[U-Boot] [PATCH v2] usb: fix usb_stor_read/write on DM

Fri Jul 21 15:59:52 UTC 2017

On Fri, Jul 21, 2017 at 04:48:58AM -0600, Simon Glass wrote:
> +Tom for question below
> 
> Hi,
> 
> On 20 July 2017 at 03:40, Marek Vasut <marex at denx.de> wrote:
> > On 07/20/2017 11:38 AM, Bin Meng wrote:
> >> +Simon,
> >>
> >> On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut <marex at denx.de> wrote:
> >>> On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
> >>>> 2017-07-20 2:33 GMT+09:00 Marek Vasut <marex at denx.de>:
> >>>>> On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
> >>>>>> 2017-07-15 21:57 GMT+09:00 Marek Vasut <marex at denx.de>:
> >>>>>>> On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
> >>>>>>>> On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut <marex at denx.de> wrote:
> >>>>>>>>> On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote:
> >>>>>>>>>> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut <marex at denx.de> wrote:
> >>>>>>>>>>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
> >>>>>>>>>>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut <marex at denx.de>:
> >>>>>>>>>>>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
> >>>>>>>>>>>>>> Prior to DM, we could not enable different types of USB controllers
> >>>>>>>>>>>>>> at the same time.  DM was supposed to loosen the limitation.  It is
> >>>>>>>>>>>>>> true that we can compile drivers, but they do not work.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> For example, if EHCI is enabled, xHCI fails as follows:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>   => usb read 82000000 0 2000
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>   USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway.
> >>>>>>>>>>>>>>   Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401)
> >>>>>>>>>>>>>>   BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()!
> >>>>>>>>>>>>>>   BUG!
> >>>>>>>>>>>>>>   ### ERROR ### Please RESET the board ###
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> The cause of the error seems the following code:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>   #ifdef CONFIG_USB_EHCI_HCD
> >>>>>>>>>>>>>>   /*
> >>>>>>>>>>>>>>    * The U-Boot EHCI driver can handle any transfer length as long as there is
> >>>>>>>>>>>>>>    * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
> >>>>>>>>>>>>>>    * limited to 65535 blocks.
> >>>>>>>>>>>>>>    */
> >>>>>>>>>>>>>>   #define USB_MAX_XFER_BLK    65535
> >>>>>>>>>>>>>>   #else
> >>>>>>>>>>>>>>   #define USB_MAX_XFER_BLK    20
> >>>>>>>>>>>>>>   #endif
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What happens if CONFIG_BLK is not set ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> USB_MAX_XFER_BLK is chosen.
> >>>>>>>>>>>
> >>>>>>>>>>> And can we fix that even for non-CONFIG_BLK ?
> >>>>>>>>>>>
> >>>>>>>>>>>>> Why is it 20 for XHCI anyway ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> You are the maintainer.
> >>>>>>>>>>>> (I hope) you have better knowledge with this.
> >>>>>>>>>>>
> >>>>>>>>>>> Heh, way to deflect the question. I seem to remember some discussion
> >>>>>>>>>>> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML
> >>>>>>>>>>> archives myself.
> >>>>>>>>>>>
> >>>>>>>>>>>> Looks like the following commit was picked up by you.
> >>>>>>>>>>>
> >>>>>>>>>>> 5 years ago, way before DM was what it is today .
> >>>>>>>>>>
> >>>>>>>>>> And even way before the introduction of XHCI into U-Boot, which means
> >>>>>>>>>> that this 20 was targeting OHCI or proprietary HCDs, not XHCI.
> >>>>>>>>>> USB_MAX_READ_BLK was already set to 20 in the initial revision of
> >>>>>>>>>> usb_storage.c. As I said in the commit message, this 20 was certainly
> >>>>>>>>>> not optimal for these non-EHCI HCDs, but it restored the previous
> >>>>>>>>>> (i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4
> >>>>>>>>>> KiB code, which was specific to ehci-hcd.c at that time. Without
> >>>>>>>>>> knowing the rationale for the legacy 20 blocks, the safest approach
> >>>>>>>>>> for non-EHCI HCDs was to use this value in order to avoid breaking a
> >>>>>>>>>> platform or something. Looking at ohci-hcd.c, it limits the transfer
> >>>>>>>>>> size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the
> >>>>>>>>>> maximum number of transfers would depend on the MSC block size.
> >>>>>>>>>> dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to
> >>>>>>>>>> have any limit caused by these drivers. The limit with the current
> >>>>>>>>>> XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK
> >>>>>>>>>> could be set to 65535 for all HCDs but OHCI and XHCI, which require
> >>>>>>>>>> specific rules depending on the MSC block size.
> >>>>>>>>>
> >>>>>>>>> For whatever reason, something tells me that setting the block size to
> >>>>>>>>> 64k for XHCI broke things, but I cannot locate the thread. But there's
> >>>>>>>>> something in the back of my head ...
> >>>>>>>>
> >>>>>>>> Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set
> >>>>>>>> to 65535 for XHCI. With an MSC block size of blksz = 512 bytes /
> >>>>>>>> block, USB_MAX_XFER_BLK can be set to at most 1 segment *
> >>>>>>>> (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536
> >>>>>>>> bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit
> >>>>>>>> is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks.
> >>>>>>>> The buffer alignment may also have to be taken into account to adjust
> >>>>>>>> these values, which would require a USB_MAX_XFER_BLK(host_if, start,
> >>>>>>>> blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535
> >>>>>>>> regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c,
> >>>>>>>> isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
> >>>>>>>
> >>>>>>> That's probably what I was looking for, thanks.
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> So, how shall we handle this?
> >>>>>>
> >>>>>> If somebody can fix this in a correct way,
> >>>>>> I am happy to hand over this.
> >>>>>
> >>>>> Any way to fix it for !CONFIG_BLK ?
> >>>>
> >>>>
> >>>> common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
> >>>>
> >>>> IIUC, !CONFIG_BLK code will be removed after migration.
> >>>>
> >>>> Is it worthwhile to save !CONFIG_BLK case?
> >>>
> >>> Hmmmmmm, sigh. When is the migration happening, how far is it ?
> >>
> >> One idea is to force all board to switch to driver model at a preset
> >> timeline. After the deadline, boards do not switch to DM will get
> >> dropped by the mainline. I noticed that not all boards are actively
> >> maintained...
> >
> > Be my guest, there's a few which I'd like to see removed myself :-)
> 
> That makes sense although I'm not sure what the deadline should be.
> CONFIG_BLK is invasive and it is a pain to carry the #ifdefs.
> 
> Maybe end of year, or is that too short?

9 months, rounded up to next release?

-- 
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/20170721/17e9bc66/attachment.sig>