On Fri, 21 Jun 2013, Ming Lei wrote:
> On Fri, Jun 21, 2013 at 11:19 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Fri, 21 Jun 2013, Konstantin Filatov wrote:
> >
> >> > --- 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.
> >> Why do you think so? Could you please refer to the specification of UHCI.
> >
> > It _isn't_ correct. But the fault lies in the driver that submitted
> > the SG transfer in the first place, not in uhci-hcd. This is the right
> > thing to do.
> >
> > Also, its the same as what ehci-hcd does. Oddly enough, it looks like
> > nobody ever added SG support to ohci-hcd.
> >
> > By the way, the patch could be improved if it removed the line
> >
> > pktsze = len;
> >
> > This won't be needed, because the code is careful to guarantee that
> > this_sg_len <= len always.
> >
> >> Your patch skips a chunk of data to transfer. This is a corruption of
> >> data. It's unacceptable. We can return an error code if we treat the
> >> request as malformed, but we can't transfer data selectively.
> >
> > I agree. Fix up the patch description as Sergei suggested, and you can
> > add
> >
> > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>
> But the patch violates USB spec, doesn't it?
No, the client driver violates the kernel's requirements by submitting
an SG transfer that can't be carried out. Although it probably isn't
documented anywhere, the USB stack requires that the size of each SG
element except the last one must be a multiple of the endpoint's
maxpacket value.
(The wireless USB stack may have a weaker requirement here. I'm
talking about wired USB only.)
We could fail the transfer request if the SG element length violates
this requirement. That would be acceptable. But then ehci-hcd and
xhci-hcd should be updated to do the same thing.
> Also it sets the later packet size of TD as the small one, is it correct?
I don't understand the question. TDs don't have a "later" or an
"earlier" packet size. Are you referring to the ActLen and MaxLen
fields in the TD? And what do you mean by "the small one"?
Alan Stern
--
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
