On Wed, 2006-04-12 at 22:56 +1000, linuxtv@xxxxxxxxxxxxxx wrote:
> Here is a patch for dst_ca.c which addresses the issue of
> messages over 128 bytes long (when asn1 length is more than 1 byte).
I ack that current code isn't correct, but this patch needs a little
revition, but it's nearly there.
>
> Ok, so apparently nobody has this problem except myself.
Indeed, of all the channels I've ever tested no one was ever near to
getting a ca_pmt >127 bytes. This said, such a case isn't impossible,
and the dst can handle up to 255 bytes.
> Ok, so I have already posted about this but it has fallen on deaf ears.
No, it hasn't fallen of deaf ears, but as no patch for it was posted no
one has yet to step up to fix the problem. (we are buzy people here)
> Ok, so I also vote Abraham off as the weakest link.
You can say whatever you like about Mr Abraham, but there are _no_
documentation for the dst whatsoever except the knowledge of this
particular person. Voting him out (Hey we are a open source community
here, not a reality show!) will leave wild guesses (like this patch
here) as the only possible choice for impoving anything with these
cards. You are free to not like this situation, but mr Abraham is not
the one to yell at. Try taking on Twinhan in stread.
Specific comments about the patch to follow:
>
> Here is it (btw it also replicates some existing patches):
> ===============================================================================
>
> --- kernel-2.6.14/linux-2.6.14/drivers/media/dvb/bt8xx/dst_ca.c.mxy 2005-10-28 10:02:08.000000000 +1000
> +++ kernel-2.6.14/linux-2.6.14/drivers/media/dvb/bt8xx/dst_ca.c 2006-04-09 11:41:33.000000000 +1000
> @@ -275,7 +275,7 @@
> return 0;
> }
>
> -static int handle_dst_tag(struct dst_state *state, struct ca_msg *p_ca_message, struct ca_msg *hw_buffer, u32 length)
> +static int handle_dst_tag(struct dst_state *state, struct ca_msg *p_ca_message, int lensize, struct ca_msg *hw_buffer, u32 length)
> {
> if (state->dst_hw_cap & DST_TYPE_HAS_SESSION) {
> hw_buffer->msg[2] = p_ca_message->msg[1]; /* MSB */
> @@ -286,17 +286,26 @@
> return -1;
> }
> hw_buffer->msg[0] = (length & 0xff) + 7;
> + /*
> + * This is probably a resource, as per EN50221 8.2.2 Table 15.
> + * type=1 2 bits
> + * class=3 (CA) 14 bits
> + * type=0 10 bits
> + * version=3 6 bits
> + */
Incorrect, this is a hardware specific commands along the lines found in
all hardware communications in dst.c and dst_ca.c
> hw_buffer->msg[1] = 0x40;
> hw_buffer->msg[2] = 0x03;
> hw_buffer->msg[3] = 0x00;
> hw_buffer->msg[4] = 0x03;
> +
> + /* But this is odd ... not asn1, and lsb,msb? */
It is just a single byte.
> hw_buffer->msg[5] = length & 0xff;
> hw_buffer->msg[6] = 0x00;
> /*
> * Need to compute length for EN50221 section 8.3.2, for the time being
> * assuming 8.3.2 is not applicable
> */
> - memcpy(&hw_buffer->msg[7], &p_ca_message->msg[4], length);
> + memcpy(&hw_buffer->msg[7], &p_ca_message->msg[3+lensize], length);
This change is the only actually needed change in the patch.
> }
> return 0;
> }
> @@ -315,11 +324,12 @@
> return 0;
> }
>
> -u32 asn_1_decode(u8 *asn_1_array)
> +u32 asn_1_decode(u8 *asn_1_array, int *plensize)
> {
> u8 length_field = 0, word_count = 0, count = 0;
> u32 length = 0;
>
> + *plensize = 1;
> length_field = asn_1_array[0];
> dprintk(verbose, DST_CA_DEBUG, 1, " Length field=[%02x]", length_field);
> if (length_field < 0x80) {
> @@ -327,8 +337,10 @@
> dprintk(verbose, DST_CA_DEBUG, 1, " Length=[%02x]\n", length);
> } else {
> word_count = length_field & 0x7f;
> + *plensize += word_count;
> for (count = 0; count < word_count; count++) {
> - length = (length | asn_1_array[count + 1]) << 8;
> + length = length << 8;
> + length += asn_1_array[count + 1];
> dprintk(verbose, DST_CA_DEBUG, 1, " Length=[%04x]", length);
> }
> }
These are all well and good, but since the dst can't cope with sizes
larger than 255 bytes checking the return value for >127 should be
enough to handle the two byte case.
> @@ -350,14 +362,17 @@
> static int ca_set_pmt(struct dst_state *state, struct ca_msg *p_ca_message, struct ca_msg *hw_buffer, u8 reply, u8 query)
> {
> u32 length = 0;
> - u8 tag_length = 8;
> + u8 tag_length = 7+1; /* 1 for checksum */
Every command sent to the dst ends with such a checksum. Try to keep
patches that fix things as small as possible to avoid confusion.
> + int lensize = 0;
>
> - length = asn_1_decode(&p_ca_message->msg[3]);
> + length = asn_1_decode(&p_ca_message->msg[3], &lensize);
> dprintk(verbose, DST_CA_DEBUG, 1, " CA Message length=[%d]", length);
> - debug_string(&p_ca_message->msg[4], length, 0); /* length is excluding tag & length */
> + if (length > sizeof(p_ca_message->msg))
> + return -1;
in addition length > (255 - 3 (tag) - lensize + 7) should be checked
for, this is because of the reformating of the capmt and the dsts limit
on 255 bytes for it's communication units.
> + debug_string(&p_ca_message->msg[3+lensize], length, 0); /* length is excluding tag & length */
this line will read out-of-buffer memory if length ==
sizeof(p_ca_message->message) or up to 3+lensize bytes shorter. (yes I
know the line you removed had this issue as well.
>
> memset(hw_buffer->msg, '\0', length);
> - handle_dst_tag(state, p_ca_message, hw_buffer, length);
> + handle_dst_tag(state, p_ca_message, lensize, hw_buffer, length);
> put_checksum(hw_buffer->msg, hw_buffer->msg[0]);
>
> debug_string(hw_buffer->msg, (length + tag_length), 0); /* tags too */
> @@ -430,7 +445,7 @@
>
> switch (command) {
> case CA_PMT:
> - dprintk(verbose, DST_CA_DEBUG, 1, "Command = SEND_CA_PMT");
> + dprintk(verbose, DST_CA_DEBUG, 1, "Command = CA_PMT");
One more needless change. The pure cosmetics can go in a separate patch.
> if ((ca_set_pmt(state, p_ca_message, hw_buffer, 0, 0)) < 0) { // code simplification started
> dprintk(verbose, DST_CA_ERROR, 1, " -->CA_PMT Failed !");
> return -1;
>
Finally, please provide a Signed-off-by line in your patch submission.
And don't mingle patch description with questions as the description
will stay in changelogs forever. Please read the patch submission
guidelines for details on these points.
Regards
Sigmund
>
> _______________________________________________
>
> linux-dvb@xxxxxxxxxxx
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
_______________________________________________
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
