[U-Boot] [U-Boot, 5/5] dm: usb: storage: Fix broken read/write when both EHCD and xHCD are enabled

[Tom Rini]

On Thu, Sep 28, 2017 at 03:55:24AM +0200, Marek Vasut wrote:
> On 09/28/2017 03:39 AM, Tom Rini wrote:
> > On Thu, Sep 28, 2017 at 09:34:23AM +0800, Bin Meng wrote:
> >> Hi Tom,
> >>
> >> On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini <trini at konsulko.com> wrote:
> >>> On Thu, Sep 07, 2017 at 06:13:21AM -0700, Bin Meng wrote:
> >>>
> >>>> When EHCD and xHCD are enabled at the same time, USB storage device
> >>>> driver will fail to read/write from/to the storage device attached
> >>>> to the xHCI interface, due to its transfer blocks exceeds the xHCD
> >>>> driver limitation.
> >>>>
> >>>> With driver model, we have an API to get the controller's maximum
> >>>> transfer size and we can use that to determine the storage driver's
> >>>> capability of read/write.
> >>>>
> >>>> Note: the non-DM version driver is still broken with xHCD and the
> >>>> intent here is not to fix the non-DM one, since the xHCD itself is
> >>>> already broken in places like 3.0 hub support, etc.
> >>>>
> >>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> >>>> Tested-by: Stefan Roese <sr at denx.de>
> >>> [snip]
> >>>> @@ -953,6 +957,17 @@ static void usb_stor_set_max_xfer_blk(struct us_data *us)
> >>>>  #else
> >>>>       blk = 20;
> >>>>  #endif
> >>>> +#else
> >>>> +     ret = usb_get_max_xfer_size(udev, (size_t *)&size);
> >>>> +     if (ret < 0) {
> >>>> +             /* unimplemented, let's use default 20 */
> >>>> +             blk = 20;
> >>>> +     } else {
> >>>> +             if (size > USHRT_MAX * 512)
> >>>> +                     blk = USHRT_MAX;
> >>>> +             blk = size / 512;
> >>>> +     }
> >>>> +#endif
> >>>
> >>> So, Coverity saw this and found an issue (CID 167250), and I was going
> >>> to just fix it, but I'm not sure.  The problem is that we check size >
> >>> (USHRT_MAX * 512), and then assign blk.  Then, we always assign blk, so
> >>> the test above isn't used.  But my background recollection is that
> >>> there's a real issue that's being addressed here.  Can we really just
> >>> always say blk = size / 512 in this case, or did we want to be shifting
> >>> size not blk under the if?  Thanks!
> >>
> >> Did this patch applied to anywhere? I see it is in the usb tree, but
> >> not in the u-boot/master.
> >>
> >> The fix should be:
> >>
> >>              if (size > USHRT_MAX * 512)
> >>                      size = USHRT_MAX * 512;
> > 
> > It's in usb/master, and will be in master itself shortly.  Please submit
> > a patch that corrects this and has the Reported-by for Coverity.  Marek,
> > do you want to take it via your tree or should I just grab it?  Thanks!
> 
> A USB patch should always go through -usb , I believe that's an
> established practice .

Aside from when you tell me to just pick up a fix directly, for whatever
appropriate reason, yes.  Which is why I asked.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170927/1a8f07c6/attachment.sig>