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

[Peter Mamonov <pmamonov@gmail.com>]

On Mon, 12 Oct 2015 15:44:01 +0200
Sascha Hauer <s.hauer@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@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. This only
concerns the drivers that use pollers, i.e. for example usb storage
driver is not affected, since it doesn't use pollers.

> 
> Sascha
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox