mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Peter Mamonov <pmamonov@gmail.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()
Date: Tue, 13 Oct 2015 13:37:33 +0300	[thread overview]
Message-ID: <20151013133733.79bd1446@berta> (raw)
In-Reply-To: <20151012134400.GV7858@pengutronix.de>

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

  reply	other threads:[~2015-10-13 10:35 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-22 15:58 [PATCH v6 0/5] input: add usb keyboard driver Peter Mamonov
2015-09-22 15:58 ` [PATCH 1/5] common: clock: introduce mdelay_non_interruptible() Peter Mamonov
2015-09-22 21:15   ` Antony Pavlov
2015-09-22 21:20     ` Peter Mamonov
2015-09-22 15:58 ` [PATCH 2/5] usb: ehci-hcd: port periodic transactions implementation from the u-boot Peter Mamonov
2015-09-22 15:58 ` [PATCH 3/5] usb: ehci-hcd: detect re-entrance Peter Mamonov
2015-09-23 14:23   ` Sascha Hauer
2015-09-22 15:58 ` [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible() Peter Mamonov
2015-10-07 13:47   ` Jean-Christophe PLAGNIOL-VILLARD
2015-10-07 14:40     ` Peter Mamonov
2015-10-07 15:40       ` Jean-Christophe PLAGNIOL-VILLARD
2015-10-07 16:52         ` Peter Mamonov
2015-10-12  7:00           ` Sascha Hauer
2015-10-12 11:43             ` Peter Mamonov
2015-10-12 13:44               ` Sascha Hauer
2015-10-13 10:37                 ` Peter Mamonov [this message]
2015-10-13 10:38                   ` Sascha Hauer
2015-10-13  2:04           ` Jean-Christophe PLAGNIOL-VILLARD
2015-10-13  9:11             ` Antony Pavlov
2015-09-22 15:58 ` [PATCH 5/5] input: port usb keyboard driver from the u-boot Peter Mamonov
2015-09-23 14:34 ` [PATCH 1/2] fixup! usb: ehci-hcd: port periodic transactions implementation " Sascha Hauer
2015-09-23 14:34   ` [PATCH 2/2] fixup! input: port usb keyboard driver " Sascha Hauer
  -- strict thread matches above, loose matches on Subject: below --
2015-09-21 11:30 [RFC v5 0/2] WIP: add usb keyboard driver Peter Mamonov
2015-09-21 11:30 ` [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible() Peter Mamonov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151013133733.79bd1446@berta \
    --to=pmamonov@gmail.com \
    --cc=barebox@lists.infradead.org \
    --cc=s.hauer@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox