* REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers @ 2012-10-10 20:31 Henrik Rydberg 2012-10-10 20:34 ` Peter Stuge 2012-10-11 8:21 ` Hans de Goede 0 siblings, 2 replies; 17+ messages in thread From: Henrik Rydberg @ 2012-10-10 20:31 UTC (permalink / raw) To: Hans de Goede; +Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel Hi Hans, Alan, Greg, commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a Author: Hans de Goede <hdegoede@redhat.com> 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-10 20:31 REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers Henrik Rydberg @ 2012-10-10 20:34 ` Peter Stuge 2012-10-11 5:44 ` Henrik Rydberg 2012-10-11 8:21 ` Hans de Goede 1 sibling, 1 reply; 17+ messages in thread From: Peter Stuge @ 2012-10-10 20:34 UTC (permalink / raw) To: Henrik Rydberg Cc: Hans de Goede, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel Hej Henrik, Henrik Rydberg wrote: > commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a > Author: Hans de Goede <hdegoede@redhat.com> > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-10 20:34 ` Peter Stuge @ 2012-10-11 5:44 ` Henrik Rydberg 2012-10-11 6:50 ` Peter Stuge 0 siblings, 1 reply; 17+ messages in thread From: Henrik Rydberg @ 2012-10-11 5:44 UTC (permalink / raw) To: Peter Stuge Cc: Hans de Goede, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel On Wed, Oct 10, 2012 at 10:34:59PM +0200, Peter Stuge wrote: > Hej Henrik, > > Henrik Rydberg wrote: > > commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a > > Author: Hans de Goede <hdegoede@redhat.com> > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 5:44 ` Henrik Rydberg @ 2012-10-11 6:50 ` Peter Stuge 2012-10-11 6:57 ` Xiaofan Chen 2012-10-11 21:54 ` Henrik Rydberg 0 siblings, 2 replies; 17+ messages in thread From: Peter Stuge @ 2012-10-11 6:50 UTC (permalink / raw) To: Henrik Rydberg Cc: Hans de Goede, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 6:50 ` Peter Stuge @ 2012-10-11 6:57 ` Xiaofan Chen 2012-10-11 7:12 ` Peter Stuge 2012-10-11 21:54 ` Henrik Rydberg 1 sibling, 1 reply; 17+ messages in thread From: Xiaofan Chen @ 2012-10-11 6:57 UTC (permalink / raw) To: Henrik Rydberg, Hans de Goede, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Stuge On Thu, Oct 11, 2012 at 2:50 PM, Peter Stuge <peter@stuge.se> 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 6:57 ` Xiaofan Chen @ 2012-10-11 7:12 ` Peter Stuge 0 siblings, 0 replies; 17+ messages in thread From: Peter Stuge @ 2012-10-11 7:12 UTC (permalink / raw) To: Xiaofan Chen Cc: Henrik Rydberg, Hans de Goede, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel Xiaofan Chen wrote: > On Thu, Oct 11, 2012 at 2:50 PM, Peter Stuge <peter@stuge.se> 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 6:50 ` Peter Stuge 2012-10-11 6:57 ` Xiaofan Chen @ 2012-10-11 21:54 ` Henrik Rydberg 1 sibling, 0 replies; 17+ messages in thread From: Henrik Rydberg @ 2012-10-11 21:54 UTC (permalink / raw) To: Hans de Goede, Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-10 20:31 REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers Henrik Rydberg 2012-10-10 20:34 ` Peter Stuge @ 2012-10-11 8:21 ` Hans de Goede 2012-10-11 21:37 ` Henrik Rydberg 1 sibling, 1 reply; 17+ messages in thread From: Hans de Goede @ 2012-10-11 8:21 UTC (permalink / raw) To: Henrik Rydberg; +Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel Hi, On 10/10/2012 10:31 PM, Henrik Rydberg wrote: > Hi Hans, Alan, Greg, > > commit 3d97ff63f8997761f12c8fbe8082996c6eeaba1a > Author: Hans de Goede <hdegoede@redhat.com> > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 8:21 ` Hans de Goede @ 2012-10-11 21:37 ` Henrik Rydberg 2012-10-11 21:40 ` Peter Stuge ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Henrik Rydberg @ 2012-10-11 21:37 UTC (permalink / raw) To: Hans de Goede Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Stuge 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 <rydberg@euromail.se> 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: stable@kernel.org Signed-off-by: Henrik Rydberg <rydberg@euromail.se> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 21:37 ` Henrik Rydberg @ 2012-10-11 21:40 ` Peter Stuge 2012-10-11 21:53 ` Greg Kroah-Hartman 2012-10-12 14:25 ` Alan Stern 2 siblings, 0 replies; 17+ messages in thread From: Peter Stuge @ 2012-10-11 21:40 UTC (permalink / raw) To: linux-usb, linux-kernel 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 <rydberg@euromail.se> > 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: stable@kernel.org > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> Acked-by: Peter Stuge <peter@stuge.se> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 21:37 ` Henrik Rydberg 2012-10-11 21:40 ` Peter Stuge @ 2012-10-11 21:53 ` Greg Kroah-Hartman 2012-10-12 14:11 ` Hans de Goede 2012-10-12 14:25 ` Alan Stern 2 siblings, 1 reply; 17+ messages in thread From: Greg Kroah-Hartman @ 2012-10-11 21:53 UTC (permalink / raw) To: Henrik Rydberg Cc: Hans de Goede, Alan Stern, linux-usb, linux-kernel, Peter Stuge 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 21:53 ` Greg Kroah-Hartman @ 2012-10-12 14:11 ` Hans de Goede 2012-10-12 15:10 ` Henrik Rydberg 0 siblings, 1 reply; 17+ messages in thread From: Hans de Goede @ 2012-10-12 14:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Henrik Rydberg, Alan Stern, linux-usb, linux-kernel, Peter Stuge 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 <hdegoede@redhat.com> And stating the obvious: CC: stable@vger.kernel.org To be backported to 3.6 only Thanks & Regards, Hans ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-12 14:11 ` Hans de Goede @ 2012-10-12 15:10 ` Henrik Rydberg 0 siblings, 0 replies; 17+ messages in thread From: Henrik Rydberg @ 2012-10-12 15:10 UTC (permalink / raw) To: Hans de Goede Cc: Greg Kroah-Hartman, Alan Stern, linux-usb, linux-kernel, Peter Stuge > >>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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-11 21:37 ` Henrik Rydberg 2012-10-11 21:40 ` Peter Stuge 2012-10-11 21:53 ` Greg Kroah-Hartman @ 2012-10-12 14:25 ` Alan Stern 2012-10-12 15:08 ` Henrik Rydberg 2 siblings, 1 reply; 17+ messages in thread From: Alan Stern @ 2012-10-12 14:25 UTC (permalink / raw) To: Henrik Rydberg Cc: Hans de Goede, Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Stuge On Thu, 11 Oct 2012, Henrik Rydberg wrote: > From 40b70394747eea51fdd07cc8213dd6afd24b1b30 Mon Sep 17 00:00:00 2001 > From: Henrik Rydberg <rydberg@euromail.se> > 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: stable@kernel.org > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > --- > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-12 14:25 ` Alan Stern @ 2012-10-12 15:08 ` Henrik Rydberg 2012-10-12 15:10 ` Alan Stern 2012-10-14 15:40 ` Hans de Goede 0 siblings, 2 replies; 17+ messages in thread From: Henrik Rydberg @ 2012-10-12 15:08 UTC (permalink / raw) To: Alan Stern Cc: Hans de Goede, Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Stuge 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 <rydberg@euromail.se> 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 <rydberg@euromail.se> --- 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) { ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-12 15:08 ` Henrik Rydberg @ 2012-10-12 15:10 ` Alan Stern 2012-10-14 15:40 ` Hans de Goede 1 sibling, 0 replies; 17+ messages in thread From: Alan Stern @ 2012-10-12 15:10 UTC (permalink / raw) To: Henrik Rydberg Cc: Hans de Goede, Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Stuge 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 <rydberg@euromail.se> > 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 <rydberg@euromail.se> > --- > 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 <stern@rowland.harvard.edu> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers 2012-10-12 15:08 ` Henrik Rydberg 2012-10-12 15:10 ` Alan Stern @ 2012-10-14 15:40 ` Hans de Goede 1 sibling, 0 replies; 17+ messages in thread From: Hans de Goede @ 2012-10-14 15:40 UTC (permalink / raw) To: Henrik Rydberg Cc: Alan Stern, Greg Kroah-Hartman, linux-usb, linux-kernel, Peter Stuge 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 <rydberg@euromail.se> > 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 <rydberg@euromail.se> > --- > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-10-14 15:45 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-10-10 20:31 REGRESSION: usbdevfs: Use-scatter-gather-lists-for-large-bulk-transfers Henrik Rydberg 2012-10-10 20:34 ` Peter Stuge 2012-10-11 5:44 ` Henrik Rydberg 2012-10-11 6:50 ` Peter Stuge 2012-10-11 6:57 ` Xiaofan Chen 2012-10-11 7:12 ` Peter Stuge 2012-10-11 21:54 ` Henrik Rydberg 2012-10-11 8:21 ` Hans de Goede 2012-10-11 21:37 ` Henrik Rydberg 2012-10-11 21:40 ` Peter Stuge 2012-10-11 21:53 ` Greg Kroah-Hartman 2012-10-12 14:11 ` Hans de Goede 2012-10-12 15:10 ` Henrik Rydberg 2012-10-12 14:25 ` Alan Stern 2012-10-12 15:08 ` Henrik Rydberg 2012-10-12 15:10 ` Alan Stern 2012-10-14 15:40 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).