On Fri, Jun 21, 2013 at 8:35 PM, Denis V. Lunev <den@xxxxxxxxxx> wrote:
> From: "Denis V. Lunev" <den@xxxxxxxxxx>
>
> From: Konstantin Filatov <kfilatov@xxxxxxxxxxxxx>
>
> The following commit
> commit 689d6eacd1b7c3677bfe6ee367766f21c3c80e26
> Author: Ming Lei <tom.leiming@xxxxxxxxx>
> Date: Thu Sep 30 20:32:44 2010 +0800
>
> USB: UHCI: add native scatter-gather support(v1)
>
> introduced an implementation of scatter-gather list for UHCI.
> This implementation has a bug in case when a non-last sg
> element was not aligned by TD's max-pkt-size. This bug was latent
> till the
> commit 2851784f4d820bc697a8cc608509f9e3975c80e5
> Author: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Date: Fri Jan 13 19:04:17 2012 +0100
>
> usb/uhci: initialize sg_table properly
>
> which initializes really initializes sg_table and enables using
> the implementation.
>
> The bug can be easily reproduced with Gadget Zero in full_speed mode
> connected to a host with UHCI controller by the standard test from
> tools/usb/ with command line
> testusb -a -t 7 -c 2000 -s 4096 -v 41
I am wondering if it is a valid test, since the built sg list has one middle sg
which size is not maxp aligned.
Did you run the test on ehci controller? and is the transfer OK on ehci
controller?
> The test crashes with SIGBUS.
IMO, the test may cause buffer crash.
>
> This patch shortens TD's packet not only for the last TD in sg list,
> but also for the last TD in sg element.
>
> Signed-off-by: Konstantin Filatov <kfilatov@xxxxxxxxxxxxx>
> Signed-off-by: Denis V. Lunev <den@xxxxxxxxxx>
> CC: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> CC: Ming Lei <tom.leiming@xxxxxxxxx>
> CC: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> CC: linux-usb@xxxxxxxxxxxxxxx
> ---
> Changes from v1:
> - added short commit descriptions as requested by Sergei Shtylyov
> - CC list extended according to the list of original authors
>
> drivers/usb/host/uhci-q.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/host/uhci-q.c b/drivers/usb/host/uhci-q.c
> index 6e6ea21..e0ebc80 100644
> --- a/drivers/usb/host/uhci-q.c
> +++ b/drivers/usb/host/uhci-q.c
> @@ -977,6 +977,9 @@ static int uhci_submit_common(struct uhci_hcd *uhci, struct urb *urb,
> for (;;) { /* Allow zero length packets */
> int pktsze = maxsze;
>
> + if (this_sg_len < pktsze) /* The short packet */
> + pktsze = this_sg_len;
This will build a short packet transfer descriptor which is not the last
one, and it isn't correct.
The attached patch may avoid the memory crash, but I am wondering if
we need to consider the situation, or a warning should be needed.
diff --git a/drivers/usb/host/uhci-q.c b/drivers/usb/host/uhci-q.c
index 041c6dd..5c98044 100644
--- a/drivers/usb/host/uhci-q.c
+++ b/drivers/usb/host/uhci-q.c
@@ -969,6 +969,13 @@ static int uhci_submit_common(struct uhci_hcd
*uhci, struct urb *urb,
pktsze = len;
if (!(urb->transfer_flags & URB_SHORT_NOT_OK))
status &= ~TD_CTRL_SPD;
+ } else {
+ /* skip the middle short transfer */
+ if (this_sg_len < maxsze) {
+ len -= this_sg_len;
+ this_sg_len = 0;
+ goto skip;
+ }
}
if (plink) {
@@ -989,6 +996,7 @@ static int uhci_submit_common(struct uhci_hcd
*uhci, struct urb *urb,
data += pktsze;
this_sg_len -= pktsze;
len -= maxsze;
+skip:
if (this_sg_len <= 0) {
if (--i <= 0 || len <= 0)
break;
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
