usbmon: size of different fields?

usbmon: size of different fields?

[Németh Márton]

Hi,

I'm looking at the struct mon_bin_hdr and struct mon_bin_isodesc in file
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/usb/mon/mon_bin.c;h=44cb37b5a4dc1f9b27075e3db5346b9ebe307b22;hb=HEAD

As far as I understand u64, s64, u32 and s32 have always fixed bit lengths.

What about "unsigned char", "char", "unsigned int" and "int"? May their size in bits
differ in different architecture?

I'm asking this because I was dealing with the USB packet dissectors for Wireshark
and it is possible to capture the USB traffic on one computer and then transfer
the file to another computer.

	Márton Németh

Re: usbmon: size of different fields?

[Pete Zaitcev]

On Tue, 09 Nov 2010 07:40:36 +0100
Németh Márton <nm127@freemail.hu> wrote:

> I'm looking at the struct mon_bin_hdr and struct mon_bin_isodesc in file
> f=drivers/usb/mon/mon_bin.c

Actually you're supposed to be looking at Documentation/usb/usbmon.txt.
If there is a discrepancy between the usbmon.txt and mon_bin.c, I want
to know about it.

> As far as I understand u64, s64, u32 and s32 have always fixed bit lengths.
> 
> What about "unsigned char", "char", "unsigned int" and "int"? May their size in bits
> differ in different architecture?

No they may not. They sizes are always the same on any architecture,
as long as Linux supports it.

> I'm asking this because I was dealing with the USB packet dissectors for Wireshark
> and it is possible to capture the USB traffic on one computer and then transfer
> the file to another computer.

Do be careful here, because the struct you're talking about is a part
of API, not a network stream. Its field sizes are rigidly defined, but
the byte order is host! You MUST NOT attempt to store it in pcap files.

-- Pete

Re: usbmon: size of different fields?

[Alan Stern]

On Tue, 9 Nov 2010, [UTF-8] Németh Márton wrote:

> Hi,
> 
> I'm looking at the struct mon_bin_hdr and struct mon_bin_isodesc in file
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/usb/mon/mon_bin.c;h=44cb37b5a4dc1f9b27075e3db5346b9ebe307b22;hb=HEAD
> 
> As far as I understand u64, s64, u32 and s32 have always fixed bit lengths.

That's right.

> What about "unsigned char", "char", "unsigned int" and "int"? May their size in bits
> differ in different architecture?

char and unsigned char are always 8 bits.  int and unsigned int are 
always 32 bits.  long and unsigned long can be either 32 or 64 bits, 
depending on the architecture.

> I'm asking this because I was dealing with the USB packet dissectors for Wireshark
> and it is possible to capture the USB traffic on one computer and then transfer
> the file to another computer.

There shouldn't be any trouble with the field sizes.  The endianness
could cause a problem, though.

Alan Stern

Re: usbmon: size of different fields?

[Németh Márton]

Pete Zaitcev wrote:
> On Tue, 09 Nov 2010 07:40:36 +0100
> Németh Márton <nm127@freemail.hu> wrote:
> 
>> I'm looking at the struct mon_bin_hdr and struct mon_bin_isodesc in file
>> f=drivers/usb/mon/mon_bin.c
> 
> Actually you're supposed to be looking at Documentation/usb/usbmon.txt.
> If there is a discrepancy between the usbmon.txt and mon_bin.c, I want
> to know about it.

There is only minor differences between Documentation/usb/usbmon.txt and
drivers/usb/mon/mon_bin.c . These are as follows:
 - the busnum field is u16 in txt and "unsigned short" in c file
 - the field "length" (in txt) has different name "len_urb" (in c)

The ISO description structure is missing from the txt description but
this can be found in drivers/usb/mon/mon_bin.c .

>> As far as I understand u64, s64, u32 and s32 have always fixed bit lengths.
>>
>> What about "unsigned char", "char", "unsigned int" and "int"? May their size in bits
>> differ in different architecture?
> 
> No they may not. They sizes are always the same on any architecture,
> as long as Linux supports it.

So to summarize, the following table is valid on all architectures. Right?

  type in Linux  | size in bits
  ---------------+---------------
  unsigned char  | 8bit
  char           | 8bit
  unsigned int   | 32bit
  int            | 32bit

>> I'm asking this because I was dealing with the USB packet dissectors for Wireshark
>> and it is possible to capture the USB traffic on one computer and then transfer
>> the file to another computer.
> 
> Do be careful here, because the struct you're talking about is a part
> of API, not a network stream. Its field sizes are rigidly defined, but
> the byte order is host! You MUST NOT attempt to store it in pcap files.

OK, that's clear, the byte order of the API structure fields are in "host endian"
order. The API structures are already saved by Wireshark into file for quite some
time. There is already a discussion on endianness topic together with ISO descritors:

  Wireshark Bug 5370 - Add support for USB isochronous
  https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5370

There is an other problem which I found about capturing ISO USB packets with
mmap, this problem seems to be originated from Linux kernel:

  Kernel Bug Tracker Bug 22182 - usbmon: completed ISO packet content is not fully arriving with mmap
  https://bugzilla.kernel.org/show_bug.cgi?id=22182

	Márton Németh

Re: [Wireshark-dev] usbmon: size of different fields?

[Guy Harris]

On Nov 9, 2010, at 12:05 PM, Németh Márton wrote:

> OK, that's clear, the byte order of the API structure fields are in "host endian"
> order. The API structures are already saved by Wireshark into file for quite some
> time.

...and tcpdump.  Support for capturing on USB on Linux has been in libpcap since at least libpcap 1.0.

When reading a pcap file, or a pcap-ng file section, written on a machine with a byte order opposite from that of the machine reading the file, libpcap and Wireshark's Wiretap library byte-swap most host-byte-order fields into the byte order of the host reading the file; the exceptions are the iso_rec structure and the isochronous descriptors.

Re: usbmon: size of different fields?

[Pete Zaitcev]

On Tue, 9 Nov 2010 13:23:28 -0800
Guy Harris <guy@alum.mit.edu> wrote:
> On Nov 9, 2010, at 12:05 PM, Németh Márton wrote:
> 
> > OK, that's clear, the byte order of the API structure fields are in "host endian"
> > order. The API structures are already saved by Wireshark into file for quite some
> > time.
> 
> ...and tcpdump.  Support for capturing on USB on Linux has been in
> libpcap since at least libpcap 1.0.

I imagined that Nemeth wanted to implement an alternative to that.
Surely he knows how libpcap works. In that case a new, host-independent
format may be introduced. But userland is mysterious to me.

I'll make sure to document ISO descriptors properly. I was sure
I had done that, sorry.

-- Pete

Re: usbmon: size of different fields?

[Guy Harris]

On Nov 10, 2010, at 7:21 AM, Pete Zaitcev wrote:

> On Tue, 9 Nov 2010 13:23:28 -0800
> Guy Harris <guy@alum.mit.edu> wrote:
>> On Nov 9, 2010, at 12:05 PM, Németh Márton wrote:
>> 
>>> OK, that's clear, the byte order of the API structure fields are in "host endian"
>>> order. The API structures are already saved by Wireshark into file for quite some
>>> time.
>> 
>> ...and tcpdump.  Support for capturing on USB on Linux has been in
>> libpcap since at least libpcap 1.0.
> 
> I imagined that Nemeth wanted to implement an alternative to that.

I hadn't heard him propose that.

It might be a good idea...

> Surely he knows how libpcap works. In that case a new, host-independent
> format may be introduced.

...and, if done, it would be ideal if it were also designed to be platform-dependent, so that it didn't have Linux implementation details leaking through; that could let it be used if other platforms offer a way to watch USB operations.

Are the formats of the USB header and the isochronous descriptors guaranteed never to change?  If not, a new format should definitely be introduced, as, for example, with the mmapped buffer, we just pass to the capture callback a pointer to the item in that buffer.  However, given that the capture callback is just passed a single pointer to the packet data, access to the mmapped buffer would have to be done by constructing the new header in a mallocated buffer *AND* all the packet data will have to be copied to that buffer, so a lot more data copying will be done.

[PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

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.

This patch fixes the problem described at https://bugzilla.kernel.org/show_bug.cgi?id=22182 .

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--- linux-2.6.37-rc1/drivers/usb/mon/mon_bin.c.orig	2010-10-20 22:30:22.000000000 +0200
+++ 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 {

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Pete Zaitcev]

On Sat, 13 Nov 2010 23:15:16 +0100
Németh Márton <nm127@freemail.hu> 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

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Németh Márton]

Pete Zaitcev wrote:
> On Sat, 13 Nov 2010 23:15:16 +0100
> Németh Márton <nm127@freemail.hu> 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.

Feel free to use any description you think worth including.

> 
>> +++ 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.

I'm happy that you can point out things points which I haven't see for the first time.

> 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.

The (c) solution would work also, it has the drawback that in that way
the kernel gives away the most uninitialized buffer content. Normally
it only contains remaining bytes from the previous URB data and not
leaking out any sensitive information.

> 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?

Sure, I'm happy as long as I can capture the whole data content of the ISO
packets.

	Márton Németh

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Pete Zaitcev]

On Sun, 14 Nov 2010 21:24:46 +0100
Németh Márton <nm127@freemail.hu> wrote:

> >> (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.
> 
> The (c) solution would work also, it has the drawback that in that way
> the kernel gives away the most uninitialized buffer content. Normally
> it only contains remaining bytes from the previous URB data and not
> leaking out any sensitive information.

I do not think the leakage in this case is a particular concern,
because any program that can do mmap() can scan the whole buffer.
The reported offsets are purely advisory. Moreover, the program
that merely reads can read your keyboard input. Therefore, leaking
a bit more is no worse than before. Access to usbmon must always be
protected by permissions in /dev.

-- Pete

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Pete Zaitcev]

On Sun, 14 Nov 2010 21:24:46 +0100
Németh Márton <nm127@freemail.hu> wrote:

> Sure, I'm happy as long as I can capture the whole data content of the ISO
> packets.

Great. So, what do you think about the attached? It differs from your
code in one important aspect: output buffer sizes are not adjusted.
Your patch attempts that, but uses actual_length, which is not
defined during submission events. So I skipped that.

-- Pete

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index 44cb37b..15c5e46 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -437,6 +437,28 @@ static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
 	return length;
 }
 
+/*
+ * This is the look-ahead pass in case of 'Ci', when actual_length cannot
+ * be used to determine the length of the whole contiguous buffer.
+ */
+static unsigned int mon_bin_collate_isodesc(const struct mon_reader_bin *rp,
+    struct urb *urb, unsigned int ndesc)
+{
+	struct usb_iso_packet_descriptor *fp;
+	unsigned int length;
+
+	length = 0;
+	fp = urb->iso_frame_desc;
+	while (ndesc-- != 0) {
+		if (fp->status == 0 && fp->actual_length != 0) {
+			if (fp->offset + fp->actual_length > length)
+				length = fp->offset + fp->actual_length;
+		}
+		fp++;
+	}
+	return length;
+}
+
 static void mon_bin_get_isodesc(const struct mon_reader_bin *rp,
     unsigned int offset, struct urb *urb, char ev_type, unsigned int ndesc)
 {
@@ -479,6 +501,10 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
 	/*
 	 * Find the maximum allowable length, then allocate space.
 	 */
+	urb_length = (ev_type == 'S') ?
+	    urb->transfer_buffer_length : urb->actual_length;
+	length = urb_length;
+
 	if (usb_endpoint_xfer_isoc(epd)) {
 		if (urb->number_of_packets < 0) {
 			ndesc = 0;
@@ -487,14 +513,16 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
 		} else {
 			ndesc = urb->number_of_packets;
 		}
+		if (ev_type == 'C' && usb_urb_dir_in(urb))
+			length = mon_bin_collate_isodesc(rp, urb, ndesc);
 	} else {
 		ndesc = 0;
 	}
 	lendesc = ndesc*sizeof(struct mon_bin_isodesc);
 
-	urb_length = (ev_type == 'S') ?
-	    urb->transfer_buffer_length : urb->actual_length;
-	length = urb_length;
+	/* not an issue unless there's a subtle bug in a HCD somewhere */
+	if (length >= urb->transfer_buffer_length)
+		length = urb->transfer_buffer_length;
 
 	if (length >= rp->b_size/5)
 		length = rp->b_size/5;

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Alan Stern]

On Sun, 14 Nov 2010, Pete Zaitcev wrote:

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index 44cb37b..15c5e46 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -437,6 +437,28 @@ static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
>  	return length;
>  }
>  
> +/*
> + * This is the look-ahead pass in case of 'Ci', when actual_length cannot
> + * be used to determine the length of the whole contiguous buffer.
> + */

Shouldn't the comment say "Cz" instead of "Ci"?

> +static unsigned int mon_bin_collate_isodesc(const struct mon_reader_bin *rp,
> +    struct urb *urb, unsigned int ndesc)
> +{
> +	struct usb_iso_packet_descriptor *fp;
> +	unsigned int length;
> +
> +	length = 0;
> +	fp = urb->iso_frame_desc;
> +	while (ndesc-- != 0) {
> +		if (fp->status == 0 && fp->actual_length != 0) {

I'd leave out the fp->status == 0 test.  It's not relevant; a buffer 
can contain valid data even if the final status isn't 0.

Alan Stern

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Pete Zaitcev]

On Sun, 14 Nov 2010 22:01:58 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> > + * This is the look-ahead pass in case of 'Ci', when actual_length cannot
> > + * be used to determine the length of the whole contiguous buffer.
> 
> Shouldn't the comment say "Cz" instead of "Ci"?

Turned out it's actually "C Zi".

> > +	while (ndesc-- != 0) {
> > +		if (fp->status == 0 && fp->actual_length != 0) {
> 
> I'd leave out the fp->status == 0 test.  It's not relevant; a buffer 
> can contain valid data even if the final status isn't 0.

That's a good point, however is actual_length filled in such case?
I thought it was either one or the other.

-- Pete

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Németh Márton]

Hi Pete,
Pete Zaitcev írta:
> On Sun, 14 Nov 2010 21:24:46 +0100
> Németh Márton <nm127@freemail.hu> wrote:
> 
>> Sure, I'm happy as long as I can capture the whole data content of the ISO
>> packets.
> 
> Great. So, what do you think about the attached? It differs from your
> code in one important aspect: output buffer sizes are not adjusted.
> Your patch attempts that, but uses actual_length, which is not
> defined during submission events. So I skipped that.

I think there are four cases for ISO communication URBs:

ev_type == 'S' && usb_urb_dir_in(urb)  ---> we don't need the data
ev_type == 'C' && usb_urb_dir_in(urb)  ---> data is available, we'll need it
ev_type == 'S' && usb_urb_dir_out(urb)  ---> data is available, we'll need it
ev_type == 'C' && usb_urb_dir_out(urb)  ---> we don't need the data

I cannot say for sure which field contains the length in case of ISO out submit
URBs.

I'll test the patch later in my environment.

> 
> -- Pete
> 
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index 44cb37b..15c5e46 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -437,6 +437,28 @@ static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
>  	return length;
>  }
>  
> +/*
> + * This is the look-ahead pass in case of 'Ci', when actual_length cannot
> + * be used to determine the length of the whole contiguous buffer.
> + */
> +static unsigned int mon_bin_collate_isodesc(const struct mon_reader_bin *rp,
> +    struct urb *urb, unsigned int ndesc)
> +{
> +	struct usb_iso_packet_descriptor *fp;
> +	unsigned int length;
> +
> +	length = 0;
> +	fp = urb->iso_frame_desc;
> +	while (ndesc-- != 0) {
> +		if (fp->status == 0 && fp->actual_length != 0) {
> +			if (fp->offset + fp->actual_length > length)
> +				length = fp->offset + fp->actual_length;

I don't know whether the compiler will optimize the two times computing the
expression (fp->offset + fp->actual_length). Maybe I would use a local variable
to compute the sum before the "if" and then use just the local variable.

> +		}
> +		fp++;
> +	}
> +	return length;
> +}
> +
>  static void mon_bin_get_isodesc(const struct mon_reader_bin *rp,
>      unsigned int offset, struct urb *urb, char ev_type, unsigned int ndesc)
>  {
> @@ -479,6 +501,10 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
>  	/*
>  	 * Find the maximum allowable length, then allocate space.
>  	 */
> +	urb_length = (ev_type == 'S') ?
> +	    urb->transfer_buffer_length : urb->actual_length;
> +	length = urb_length;
> +
>  	if (usb_endpoint_xfer_isoc(epd)) {
>  		if (urb->number_of_packets < 0) {
>  			ndesc = 0;
> @@ -487,14 +513,16 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
>  		} else {
>  			ndesc = urb->number_of_packets;
>  		}
> +		if (ev_type == 'C' && usb_urb_dir_in(urb))
> +			length = mon_bin_collate_isodesc(rp, urb, ndesc);
>  	} else {
>  		ndesc = 0;
>  	}
>  	lendesc = ndesc*sizeof(struct mon_bin_isodesc);
>  
> -	urb_length = (ev_type == 'S') ?
> -	    urb->transfer_buffer_length : urb->actual_length;
> -	length = urb_length;
> +	/* not an issue unless there's a subtle bug in a HCD somewhere */
> +	if (length >= urb->transfer_buffer_length)
> +		length = urb->transfer_buffer_length;
>  
>  	if (length >= rp->b_size/5)
>  		length = rp->b_size/5;
> 
> 

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Pete Zaitcev]

On Mon, 15 Nov 2010 06:48:40 +0100
Németh Márton <nm127@freemail.hu> wrote:

> ev_type == 'S' && usb_urb_dir_out(urb)  ---> data is available, we'll need it

The write submission case should be covered by the transfer_buffer_length,
I think. Is there a driver that only sets the ISO descriptors but not
the overall length?

-- Pete

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Alan Stern]

On Sun, 14 Nov 2010, Pete Zaitcev wrote:

> > > +	while (ndesc-- != 0) {
> > > +		if (fp->status == 0 && fp->actual_length != 0) {
> > 
> > I'd leave out the fp->status == 0 test.  It's not relevant; a buffer 
> > can contain valid data even if the final status isn't 0.
> 
> That's a good point, however is actual_length filled in such case?
> I thought it was either one or the other.

No, it's possible to have both.  This is less likely with isochronous 
than with the other transfer types, but it can happen.  For example, 
consider a high-bandwidth high-speed transfer where 3072 bytes are 
received in three packets during a single microframe.  You might 
receive two of the three packets, giving actual_length = 2048, and then 
not receive the third, giving status = -EPROTO.

Alan Stern

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Alan Stern]

On Sun, 14 Nov 2010, Pete Zaitcev wrote:

> On Mon, 15 Nov 2010 06:48:40 +0100
> Németh Márton <nm127@freemail.hu> wrote:
> 
> > ev_type == 'S' && usb_urb_dir_out(urb)  ---> data is available, we'll need it
> 
> The write submission case should be covered by the transfer_buffer_length,
> I think. Is there a driver that only sets the ISO descriptors but not
> the overall length?

If there is, it's a bug.  usb_submit_urb() could check for that sort of 
thing -- although so far nobody has complained of problems, so checking 
doesn't seem necessary.

Other things usb_submit_urb() could check for, but currently doesn't, 
include:

	Make sure the packet offsets are non-decreasing;

	Make sure the packet don't overlap in the buffer;

	Make sure the spacing between two consecutive packets
	in the buffer doesn't exceed some upper limit (needed
	by ehci-hcd).

Alan Stern

Re: [PATCH, RFC] usbmon: correct computing of the ISO packets with mmap

[Németh Márton]

Németh Márton wrote:
> Hi Pete,
> Pete Zaitcev wrote:
>> On Sun, 14 Nov 2010 21:24:46 +0100
>> Németh Márton <nm127@freemail.hu> wrote:
>>
>>> Sure, I'm happy as long as I can capture the whole data content of the ISO
>>> packets.
>> Great. So, what do you think about the attached? It differs from your
>> code in one important aspect: output buffer sizes are not adjusted.
>> Your patch attempts that, but uses actual_length, which is not
>> defined during submission events. So I skipped that.
> 
> I think there are four cases for ISO communication URBs:
> 
> ev_type == 'S' && usb_urb_dir_in(urb)  ---> we don't need the data
> ev_type == 'C' && usb_urb_dir_in(urb)  ---> data is available, we'll need it
> ev_type == 'S' && usb_urb_dir_out(urb)  ---> data is available, we'll need it
> ev_type == 'C' && usb_urb_dir_out(urb)  ---> we don't need the data
> 
> I cannot say for sure which field contains the length in case of ISO out submit
> URBs.
> 
> I'll test the patch later in my environment.

I tested this patch with ISO in traffic and it works for me.

>> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
>> index 44cb37b..15c5e46 100644
>> --- a/drivers/usb/mon/mon_bin.c
>> +++ b/drivers/usb/mon/mon_bin.c
>> @@ -437,6 +437,28 @@ static unsigned int mon_bin_get_data(const struct mon_reader_bin *rp,
>>  	return length;
>>  }
>>  
>> +/*
>> + * This is the look-ahead pass in case of 'Ci', when actual_length cannot
>> + * be used to determine the length of the whole contiguous buffer.
>> + */
>> +static unsigned int mon_bin_collate_isodesc(const struct mon_reader_bin *rp,
>> +    struct urb *urb, unsigned int ndesc)
>> +{
>> +	struct usb_iso_packet_descriptor *fp;
>> +	unsigned int length;
>> +
>> +	length = 0;
>> +	fp = urb->iso_frame_desc;
>> +	while (ndesc-- != 0) {
>> +		if (fp->status == 0 && fp->actual_length != 0) {
>> +			if (fp->offset + fp->actual_length > length)
>> +				length = fp->offset + fp->actual_length;
> 
> I don't know whether the compiler will optimize the two times computing the
> expression (fp->offset + fp->actual_length). Maybe I would use a local variable
> to compute the sum before the "if" and then use just the local variable.
> 
>> +		}
>> +		fp++;
>> +	}
>> +	return length;
>> +}
>> +
>>  static void mon_bin_get_isodesc(const struct mon_reader_bin *rp,
>>      unsigned int offset, struct urb *urb, char ev_type, unsigned int ndesc)
>>  {
>> @@ -479,6 +501,10 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
>>  	/*
>>  	 * Find the maximum allowable length, then allocate space.
>>  	 */
>> +	urb_length = (ev_type == 'S') ?
>> +	    urb->transfer_buffer_length : urb->actual_length;
>> +	length = urb_length;
>> +
>>  	if (usb_endpoint_xfer_isoc(epd)) {
>>  		if (urb->number_of_packets < 0) {
>>  			ndesc = 0;
>> @@ -487,14 +513,16 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
>>  		} else {
>>  			ndesc = urb->number_of_packets;
>>  		}
>> +		if (ev_type == 'C' && usb_urb_dir_in(urb))
>> +			length = mon_bin_collate_isodesc(rp, urb, ndesc);
>>  	} else {
>>  		ndesc = 0;
>>  	}
>>  	lendesc = ndesc*sizeof(struct mon_bin_isodesc);
>>  
>> -	urb_length = (ev_type == 'S') ?
>> -	    urb->transfer_buffer_length : urb->actual_length;
>> -	length = urb_length;
>> +	/* not an issue unless there's a subtle bug in a HCD somewhere */
>> +	if (length >= urb->transfer_buffer_length)
>> +		length = urb->transfer_buffer_length;
>>  
>>  	if (length >= rp->b_size/5)
>>  		length = rp->b_size/5;
>>
>>
> 
>