On Sat, 13 Nov 2010 23:15:16 +0100
NÃmeth MÃrton <nm127@xxxxxxxxxxx> wrote:
> The length of the isochronous packets were not computed correctly in case of memory
> mapped operation because the gaps between the isodesc data were not taken into
> account. The last data byte can be found at offset+actual_length of the
> last ISO description.
Indeed this is a bug, thanks for doing the legwork to find it. However,
I would rather describe it as "received ISO buffer has gaps and
actual_length is a length of actually transferred data segments,
not whole buffer". Your own Bugzilla entry has a better explanation:
> The second packet contains the completed URB. In the user space we see that
> there are 1000 data bytes in this URB. This data is spread between ISO blocks
> 0...5 giving 155+160+185+174+156+170=1000. ISO desc 6...12 have zero length
> each. ISO desciptors 13..23 are not completed (-EXDEV). The problem is that the
> first 1000 bytes transfered from the kernel contains 800 bytes from the isodesc
> 0 out of 155 bytes are used. The next 800 bytes are not transfered completely,
> only the first 200 bytes out of 16 bytes are used. Then isodesc 2...5 are not
> transfered at all.
I think we should just include this in the patch header.
> +++ linux-2.6.37-rc1/drivers/usb/mon/mon_bin.c 2010-11-13 22:29:09.000000000 +0100
> @@ -515,6 +515,26 @@ static void mon_bin_event(struct mon_rea
> }
>
> if (rp->mmap_active) {
> + if (usb_endpoint_xfer_isoc(epd) &&
> + ((usb_urb_dir_in(urb) && ev_type == 'C') ||
> + (usb_urb_dir_out(urb) && ev_type == 'S'))) {
> + int i;
> +
> + /* Search for the last ISO descritor with OK status
> + * and non-zero length
> + */
> + length = 0;
> + i = urb->number_of_packets - 1;
> + while (0 <= i &&
> + (urb->iso_frame_desc[i].status != 0 ||
> + urb->iso_frame_desc[i].actual_length == 0)) {
> + i--;
> + }
> + if (0 <= i) {
> + length = urb->iso_frame_desc[i].offset +
> + urb->iso_frame_desc[i].actual_length;
> + }
> + }
> offset = mon_buff_area_alloc_contiguous(rp,
> length + PKT_SIZE + lendesc);
> } else {
I am not entirely satisfied with the fix, for a couple of reasons.
First, it looks to me that the copy-out access with read(2) has exactly
the same problem as access with mmap(2), so the patch should correct
both cases.
Second, the recomputation of length is done after the anti-bursting
check, thus bypassing it.
Finally, I'm not quite certain that using actual descriptors
(urb->number_of_packets) is saner than returned descriptors
(ep->ndesc). It's not like Wireshark can use those data bytes for
anything useful without the corresponding descriptors, or does it?
Again, in Bugzilla:
> I could imagine different expected behaviours:
>
> (a) the transfered size equals to 800+800+800+800+800+170=4170 bytes, so the
> iso desc 0...4 are fully transfered and the useful data from isodesc 5
>
> (b) the transfered size equals to 800+800+800+800+800+800=4800 bytes, so the
> iso desc 0...5 are fully transfered
>
> (c) the transfered size equals to maximum possible size always, in this case
> 24*800=19200 bytes
I see you went for (a). I leaned towards (c), just for simplicity.
On the other hand, we already loop over the descriptors in
mon_bin_get_isodesc, so you're not adding an additional crash.
Let's do this: I'll rework your patch, and you review it to make
sure it works, then sign it off if it checks out. Would that work?
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
