REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers

commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a

Author: Hans de Goede <[email protected]>
Date:   Wed Jul 4 09:18:03 2012 +0200

usbdevfs: Use scatter-gather lists for large bulk transfers

breaks an usb programming cable over here. The problem is reported as "bulk tranfer failed" [sic] by the tool, and bisection leads to this commit. Reverting on top of 3.6 solves it for me.

I am happy to test alternatives.

Hej Henrik,

Quoted 10 lines · Henrik Rydberg

commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a

Author: Hans de Goede <[email protected]>
Date:   Wed Jul 4 09:18:03 2012 +0200

usbdevfs: Use scatter-gather lists for large bulk transfers

breaks an usb programming cable over here. The problem is reported as "bulk tranfer failed" [sic] by the tool, and bisection leads to this commit. Reverting on top of 3.6 solves it for me.

Quoted 1 line

I am happy to test alternatives.

In order to make full use of the new kernel commit you also need changes in libusb, if the tool uses libusb, but I agree that the kernel change must under no circumstance cause existing userland software to regress.

What is the programming cable and software that uses it?

Quoted 1 line

What is the programming cable and software that uses it?

The programmer is impact, using libusbx-1.0.14-1.

The patch is pretty generic, so I am suprised the problem has not shown up earlier.

But it isn't certain that is is involved at all. If your impact version uses libusb.so like in ISE 11.1 then it will not be. One way to test is by trying to generate a libusb debug log. See
https://libusb.org/wiki/debug for instructions which are for libusb, but which work also for libusbx since the code is nearly the same. (Just replace the repository to clone from.)

Quoted 6 lines · Peter Stuge

But it isn't certain that is is involved at all. If your impact version uses libusb.so like in ISE 11.1 then it will not be. One way to test is by trying to generate a libusb debug log. See
https://libusb.org/wiki/debug for instructions which are for libusb, but which work also for libusbx since the code is nearly the same. (Just replace the repository to clone from.)

Actually this is not that correct. For libusbx, you do not need to rebuild libusbx to get the debug log at all, just need to set environment variable LIBUSB_DEBUG=4 to get the debug log.

Quoted 3 lines · Henrik Rydberg

Quoted 1 line

What is the programming cable and software that uses it?

The programmer is impact, using libusbx-1.0.14-1.

Do you know for a fact that your version calls libusb-1.0? Did you establish this with e.g. strace? ISE 11.1 impact uses only libusb.so, ie. libusb-0.1.

Quoted 2 lines

The patch is pretty generic, so I am suprised the problem has not shown up earlier.

There are several explanations. There is clearly a problem with Hans' patch(es) for some cases, but those are perhaps not so common.

I've reviewed Hans' patch that he added to libusbx and which is in libusbx-1.0.14, but I am not sure that it is correct, which is why I haven't included it in libusb yet.

Hi Hans, Alan, Greg,

commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a

Author: Hans de Goede <[email protected]>
Date:   Wed Jul 4 09:18:03 2012 +0200

usbdevfs: Use scatter-gather lists for large bulk transfers

breaks an usb programming cable over here. The problem is reported as "bulk tranfer failed" [sic] by the tool, and bisection leads to this commit. Reverting on top of 3.6 solves it for me.

Oh what fun (not). The best way to figure out what really is going on is to get some usb level traces. Note my first hunch is that what you're seeing is a device firmware bug, as this patch together with a new libusb (which you seem to also have) will make bulk transfers run slightly faster, which might be just enough to overwhelm your device ...

From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001

Hi Hans,

Quoted 6 lines

Oh what fun (not). The best way to figure out what really is going on is to get some usb level traces. Note my first hunch is that what you're seeing is a device firmware bug, as this patch together with a new libusb (which you seem to also have) will make bulk transfers run slightly faster, which might be just enough to overwhelm your device ...

Or, the large bulk transfer actually never worked in the first place. The list you gave me seemed boringly long, so I read the patch more closely instead. The fix below is the result. Greg, will you please take it through your tree?

Thanks,
Henrik

From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <[email protected]>
Date: Thu, 11 Oct 2012 23:27:04 +0200
Subject: [PATCH] usbdevfs: Fix broken scatter-gather transfer

The recently introduced handling of large bulk transfers is completely broken; the same user page is read over and over again. Fixed with this patch.

Cc [email protected]

Signed-off-by Henrik Rydberg <[email protected]>

Hi Hans,

Quoted 6 lines

Oh what fun (not). The best way to figure out what really is going on is to get some usb level traces. Note my first hunch is that what you're seeing is a device firmware bug, as this patch together with a new libusb (which you seem to also have) will make bulk transfers run slightly faster, which might be just enough to overwhelm your device ...

Or, the large bulk transfer actually never worked in the first place. The list you gave me seemed boringly long, so I read the patch more closely instead. The fix below is the result. Greg, will you please take it through your tree?

Quoted 10 lines · Henrik Rydberg

Hi Hans,

Quoted 6 lines

Oh what fun (not). The best way to figure out what really is going on is to get some usb level traces. Note my first hunch is that whath you're seeing is a device firmware bug, as this patch together with a new libusb (which you seem to also have) will make bulk transfers run slightly faster, which might be just enough to overwhelm your device ...

Or, the large bulk transfer actually never worked in the first place.

Quoted 3 lines

The list you gave me seemed boringly long, so I read the patch more closely instead. The fix below is the result. Greg, will you please take it through your tree?

Henrik, Very nice fix, thanks for debugging this.

Hans, any objection to me taking this?

Quoted 1 line

Quoted 1 line

Or, the large bulk transfer actually never worked in the first place.

Large input transfers certainly do, as they were part of my tests, but I must admit my test cases seem to not include large output transfers (my bad).

From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <[email protected]>
Date: Thu, 11 Oct 2012 23:27:04 +0200
Subject: [PATCH] usbdevfs: Fix broken scatter-gather transfer

The recently introduced handling of large bulk transfers is completely broken; the same user page is read over and over again. Fixed with this patch.

Cc [email protected]

Signed-off-by Henrik Rydberg <[email protected]>

---
 drivers/usb/core/devio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/core/devio.c +2 -0

index ebb8a9d..6e58b59 100644
@@ -1172,6 +1172,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 	struct usb_ctrlrequest *dr = NULL;
 	unsigned int u, totlen, isofrmlen;
 	int i, ret, is_in, num_sgs = 0, ifnum = -1;
+	void __user *userbuf;
 	void *buf;
 
 	if (uurb->flags & ~(USBDEVFS_URB_ISO_ASAP |

@@ -1333,6 +1334,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 		as->urb->num_sgs = num_sgs;
 		sg_init_table(as->urb->sg, as->urb->num_sgs);
 
+		userbuf = uurb->buffer;
 		totlen = uurb->buffer_length;
 		for (i = 0; i < as->urb->num_sgs; i++) {
 			u = (totlen > USB_SG_SIZE) ? USB_SG_SIZE : totlen;

Instead of introducing a new local variable, why not simply update uurb->buffer? That's what we do elsewhere in the code.

From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001

Hi Alan,

Quoted 2 lines

Instead of introducing a new local variable, why not simply update uurb->buffer? That's what we do elsewhere in the code.

It seemed fragile, due to these scary lines:

	if (is_in && uurb->buffer_length > 0)
		as->userbuffer = uurb->buffer;
	else
		as->userbuffer = NULL;

I suppose it works in this particular case. How is the USBDEVFS_URB_TYPE_CONTROL case supposed to work here?

I can certainly change the patch if preferred. Tested and attached below.

Thanks,
Henrik

From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <[email protected]>
Date: Thu, 11 Oct 2012 23:27:04 +0200
Subject: [PATCH v2] usbdevfs: Fix broken scatter-gather transfer

The handling of large output bulk transfers is broken; the same user page is read over and over again. Fixed with this patch.

Signed-off-by Henrik Rydberg <[email protected]>

---
 drivers/usb/core/devio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/core/devio.c +0 -0

index ebb8a9d..6e58b59 100644

Hi Alan,

Quoted 2 lines

Instead of introducing a new local variable, why not simply update uurb->buffer? That's what we do elsewhere in the code.

It seemed fragile, due to these scary lines:

	if (is_in && uurb->buffer_length > 0)
		as->userbuffer = uurb->buffer;
	else
		as->userbuffer = NULL;

I suppose it works in this particular case. How is the USBDEVFS_URB_TYPE_CONTROL case supposed to work here?

I can certainly change the patch if preferred. Tested and attached below.

Thanks,
Henrik

From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <[email protected]>
Date: Thu, 11 Oct 2012 23:27:04 +0200
Subject: [PATCH v2] usbdevfs: Fix broken scatter-gather transfer

The handling of large output bulk transfers is broken; the same u page is read over and over again. Fixed with this patch.

Signed-off-by Henrik Rydberg <[email protected]>

---
  drivers/usb/core/devio.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/core/devio.c +0 -0

index ebb8a9d..6e58b59 100644