Re: [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  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 
>  Tested-by: Stefan Roese 
> >>> [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 *));
>  + 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


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Marek Vasut]

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  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 
 Tested-by: Stefan Roese 
>>> [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 *));
 + 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 .

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 09:34:23AM +0800, Bin Meng wrote:
> Hi Tom,
> 
> On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini  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 
> >> Tested-by: Stefan Roese 
> > [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 *));
> >> + 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!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Bin Meng]

Hi Tom,

On Thu, Sep 28, 2017 at 12:34 AM, Tom Rini  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 
>> Tested-by: Stefan Roese 
> [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 *));
>> + 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;

Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 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 
> Tested-by: Stefan Roese 
[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 *));
> + 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!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot