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.
I am happy to test alternatives.
Thanks,
Henrik
Hej Henrik,
Henrik Rydberg wrote:
> 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.
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?
//Peter
On Wed, Oct 10, 2012 at 10:34:59PM +0200, Peter Stuge wrote:
> Hej Henrik,
>
> Henrik Rydberg wrote:
> > 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.
>
> 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.
Indeed.
> What is the programming cable and software that uses it?
The programmer is impact, using libusbx-1.0.14-1. The device runs the
xusbdfwu firmware. The (usb 2.0) bulk endpoint says wMaxPacketSize
1x512 bytes, and bInterval 0.
The patch is pretty generic, so I am suprised the problem has not
shown up earlier.
Henrik
Henrik Rydberg wrote:
> > 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.
Depending on what distribution you are using you may or may not have
libusb-compat-0.1, without which the libusbx package isn't involved
at all.
Many distributions still ship libusb-0.1.12 or some patched variant
thereof, which of course knows nothing about the kernel changes.
> 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.
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.)
Another factor is the host controller. Is the device connected to a
USB 3.0 port or a USB 2.0 port? USB 3.0 still has some gotchas which
may or may not be what you are seeing.
Looking forward to more information
//Peter
On Thu, Oct 11, 2012 at 2:50 PM, Peter Stuge <[email protected]> wrote:
> 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.
Also it seems to me Xilinx is using libusb-1.0 API now.
http://www.xilinx.com/support/documentation/sw_manuals/xilinx14_1/pim_t_linux_cable_drivers.htm
--
Xiaofan
Xiaofan Chen wrote:
> On Thu, Oct 11, 2012 at 2:50 PM, Peter Stuge <[email protected]> wrote:
> > 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.
The configure option to completely disable logging is still present
in libusbx, and I do not know which distributions use that, if any,
so I prefer to recommend the method that is guaranteed to work.
//Peter
Hi,
On 10/10/2012 10:31 PM, Henrik Rydberg wrote:
> 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 ...
Can you please do the following:
1) unplug as many usb devices as possible
2) plug in the programmer
3) run lsusb, note the programmer bus number
4) if the programmer is one the same bus as another device (other then
a hub), try a different port
5) ideally rinse repeat until it is on a bus of its own, or atleast
a bus with a device which generate little trafic
6) Writedown the bus number, and keep using the same port for
all further tests
Then:
1) start wireshark as root, tell it to start capturing on the USB bus
you've ended up using
2) Do what you always do with the programmer
3) When finished, or when things have failed stop wireshark capture and
save the capture to a file
Repeat 2 times once with a kernel with the problematic commit, once without.
Then send me the 2 wireshark captures. Note:
1) Privacy warning: In theory I might be able to reconstruct what you're sending
over the captured USB bus when you send me these captures
2) Less is more, so if you can find some mini mini program to send to the
device you're programming that makes live easier (and should make 1 less of
an issue too).
Regards,
Hans
Hi Hans,
> 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]>
---
drivers/usb/core/devio.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ebb8a9d..6e58b59 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -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;
@@ -1344,11 +1346,12 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
sg_set_buf(&as->urb->sg[i], buf, u);
if (!is_in) {
- if (copy_from_user(buf, uurb->buffer, u)) {
+ if (copy_from_user(buf, userbuf, u)) {
ret = -EFAULT;
goto error;
}
}
+ userbuf += u;
totlen -= u;
}
} else if (uurb->buffer_length > 0) {
--
1.7.12.2
Henrik Rydberg wrote:
> Hi Hans,
>
> > 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]>
Acked-by: Peter Stuge <[email protected]>
On Thu, Oct 11, 2012 at 08:50:33AM +0200, Peter Stuge wrote:
> Henrik Rydberg wrote:
> > > 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.
I modified libusbx during testing, and yes, it is really used.
> > 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.
As it turned out, all cases of large bulk transfers, so I guess 3.6
has not been that widely tested yet. ;-)
> 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.
I looked at it for a while, it does seem correct.
Thanks,
Henrik
On Thu, Oct 11, 2012 at 11:37:07PM +0200, Henrik Rydberg wrote:
> Hi Hans,
>
> > 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?
Henrik, Very nice fix, thanks for debugging this.
Hans, any objection to me taking this?
greg k-h
Hi,
On 10/11/2012 11:53 PM, Greg Kroah-Hartman wrote:
> On Thu, Oct 11, 2012 at 11:37:07PM +0200, Henrik Rydberg wrote:
>> Hi Hans,
>>
>>> 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.
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).
Thanks for fixing this!
>> 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?
No objections please take it, this patch is:
Acked-by: Hans de Goede <[email protected]>
And stating the obvious:
CC: [email protected]
To be backported to 3.6 only
Thanks & Regards,
Hans
On Thu, 11 Oct 2012, Henrik Rydberg wrote:
> 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(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index ebb8a9d..6e58b59 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -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;
> @@ -1344,11 +1346,12 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
> sg_set_buf(&as->urb->sg[i], buf, u);
>
> if (!is_in) {
> - if (copy_from_user(buf, uurb->buffer, u)) {
> + if (copy_from_user(buf, userbuf, u)) {
> ret = -EFAULT;
> goto error;
> }
> }
> + userbuf += u;
Instead of introducing a new local variable, why not simply update
uurb->buffer? That's what we do elsewhere in the code.
Alan Stern
Hi Alan,
> 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(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ebb8a9d..6e58b59 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1349,6 +1351,7 @@
goto error;
}
}
+ uurb->buffer += u;
totlen -= u;
}
} else if (uurb->buffer_length > 0) {
> >>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).
Oh right, I should change the wording to reflect that this is for
output only. Please see the reply to Alan's comment for a (possibly)
better version.
Thanks,
Henrik
On Fri, 12 Oct 2012, Henrik Rydberg wrote:
> Hi Alan,
>
> > 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?
It's understood that for control transfers, the setup part of the
buffer remains unchanged. The data transfer starts at the eighth byte
of the buffer.
> 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(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index ebb8a9d..6e58b59 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -1349,6 +1351,7 @@
> goto error;
> }
> }
> + uurb->buffer += u;
> totlen -= u;
> }
> } else if (uurb->buffer_length > 0) {
Acked-by: Alan Stern <[email protected]>
Hi,
On 10/12/2012 05:08 PM, Henrik Rydberg wrote:
> Hi Alan,
>
>> 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(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index ebb8a9d..6e58b59 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -1349,6 +1351,7 @@
> goto error;
> }
> }
> + uurb->buffer += u;
I know you've already fixed this in the next revision, but still:
NAK, this will break large, split input transfers as it well
set as->userbuffer to the end of the buffer instead of the beginning!
Regards,
Hans