[PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Sascha Hauer]

On Tue, Oct 13, 2015 at 01:37:33PM +0300, Peter Mamonov wrote:
> On Mon, 12 Oct 2015 15:44:01 +0200
> Sascha Hauer <s.hauer at pengutronix.de> wrote:
> 
> > On Mon, Oct 12, 2015 at 02:43:25PM +0300, Peter Mamonov wrote:
> > > On Mon, 12 Oct 2015 09:00:21 +0200
> > > Sascha Hauer <s.hauer at pengutronix.de> wrote:
> > > 
> > > > On Wed, Oct 07, 2015 at 07:52:21PM +0300, Peter Mamonov wrote:
> > > > > On Wed, 7 Oct 2015 17:40:24 +0200
> > > > > > > Non-interruptible delays are needed here to prevent ehci_*
> > > > > > > functions re-entrance. The re-entrance occurs during a usb
> > > > > > > bus scan, after detection of a usb keyboard. As soon as a
> > > > > > > USB keyboard is detected, it's driver starts the poller,
> > > > > > > which interferes with the process of usb bus scan. However
> > > > > > > that last one delay may be interruptible.
> > > > > > 
> > > > > > my problem is as soon as you start a usb control msg you block
> > > > > > everything
> > > > > > 
> > > > > > nothing else can run in barebox
> > > > > > 
> > > > > > can slow down barebox boot
> > > > > > 
> > > > > > I do think we need a real mdelay_non_interruptible feature but
> > > > > > just forbidden to recall usb control msg.
> > > > > > But the rest of barebox can run durring those 5ms
> > > > > 
> > > > > Well, I think it can be done by returning -EAGAIN on re-entrance
> > > > > detection in ehci_* functions [1], and skipping the poll in the
> > > > > keyboard driver.
> > > > 
> > > > Could you tell us what you have done to get re-entrance problems?
> > > > I have just replaced the mdelay_non_interruptible with regular
> > > > mdelay in the ehci driver and didn't notice any problems. Could
> > > > you verify the _non_interruptible is still needed?
> > > 
> > > If I revert the "usb: ehci-hcd: use mdelay_non_interruptible()"
> > > patch, while leaving the "usb: ehci-hcd: detect re-entrance"
> > > applied, I get the following messages after running the "usb"
> > > command:
> > > 
> > > barebox:/
> > > usb USB: scanning bus for
> > > devices... Bus 001 Device 001: ID 0000:0000 EHCI Host
> > > Controller Bus 001 Device 002: ID
> > > 0424:2514 Bus 001 Device 003: ID 413c:2112 Dell USB Wired
> > > Multimedia Keybo usb-keyboard usb1-0-0: USB keyboard
> > > found Bus 001 Device 004: ID 8564:1000 Mass Storage
> > > Device Using index 0 for the new
> > > disk usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > > (usb-hub:usb1) usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > > (usb-hub:usb1) usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > > (usb-keyboard:usb1-0-0) usb-keyboard usb1-0-0: submit_int_msg:
> > > re-entrance 1 (usb-keyboard:usb1-0-0) usb-keyboard usb1-0-0:
> > > submit_int_msg: re-entrance 1 (usb-hub:usb1) usb-keyboard usb1-0-0:
> > > submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0) usb-keyboard
> > > usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0) Bus
> > > 001 Device 005: ID 0424:2514 5 USB Device(s)
> > > found                                                       
> > > 
> > > According to the debug messages: submit_int_msg() is called by the
> > > usb-keyboard driver (from the poller function), while "usb-hub"
> > > driver is executing submit_control_msg() (which calls mdelay() and
> > > poller_call() subsequently). This occurs several times, until usb
> > > bus scan is completed. Probably the problem can be reproduced by
> > > adding more usb devices.
> > 
> > I wonder why this does not happen here. I finally could reproduce this
> > by adding some additional delays to the ehci driver.
> > 
> > We should probably move the reentrance detection to the generic usb
> > layer to usb_submit_int_msg, usb_control_msg and usb_bulk_msg. This
> > would avoid running into the same problems in the other usb host
> > drivers. If I understand the issue correctly we could just use regular
> > *delay in the host drivers when we detect reentrancy correctly, right?
> > I mean we don't have to print an error message when we detect
> > reentrance, but instead just consider that a case that can happen.
> 
> We can return -EAGAIN on reentrance detection in usb_* functions, but
> the usb device driver should distinguish this case and do sane
> things, i.e. it should not fail, but skip the action.

Yes, that sounds like a sane approach to me.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |