On Tue, Apr 09, 2013 at 12:02:59PM +0300, Dan Carpenter wrote:
> I had a question about 4c055a5ae94c: "[media] redrat3: remove memcpys
> and fix unaligned memory access" from Feb 16, 2013.
>
> drivers/media/rc/redrat3.c
> 619 /* grab the Length and type of transfer */
> 620 pktlen = be16_to_cpu(header->length);
> 621 pkttype = be16_to_cpu(header->transfer_type);
> 622
> 623 if (pktlen > sizeof(rr3->irdata)) {
> 624 dev_warn(rr3->dev, "packet length %u too large\n", pktlen);
> 625 return;
> 626 }
> 627
> 628 switch (pkttype) {
> 629 case RR3_ERROR:
> 630 if (len >= sizeof(struct redrat3_error)) {
> 631 struct redrat3_error *error = rr3->bulk_in_buf;
> 632 unsigned fw_error = be16_to_cpu(error->fw_error);
> 633 redrat3_dump_fw_error(rr3, fw_error);
> 634 }
> 635 break;
> 636
> 637 case RR3_MOD_SIGNAL_IN:
> 638 memcpy(&rr3->irdata, rr3->bulk_in_buf, len);
> ^^^
> 639 rr3->bytes_read = len;
> ^^^
> 640 rr3_dbg(rr3->dev, "bytes_read %d, pktlen %d\n",
> 641 rr3->bytes_read, pktlen);
> ^^^^^^
> Should we be copying "pktlen" bytes on the line before? It seems
> inconsistent that it doesn't match the debug code.
The pktlen is the length of the entire packet, and here we might have
received only part of it. This debug line would have been better if
it was:
rr3_dbg(rr3->dev, "%s read of total %d\n", len, pktlen);
> My main concern is that we limit the size of "pktlen" but then we only
> use it for debug output.
pktlen is a copy of the length field of the packet. That gets memcpy'd
(line 638) into rr3->irdata where it is available as rr3->irdata.length.
It is later used to check if we've received the full packet:
689 if (rr3->bytes_read < be16_to_cpu(rr3->irdata.header.length))
690 /* we're still accumulating data */
691 return 0;
HTH and thanks for checking my changes.
Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
