All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: martin_liu <liumartin@google.com>
Cc: gregkh@linuxfoundation.org, <linux-usb@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [RFC] driver core: don't hold dev's parent lock when using async probe
Date: Tue, 22 May 2018 13:09:44 -0400 (EDT)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1805221250480.1466-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <20180522141227.GA118442@google.com>

On Tue, 22 May 2018, martin_liu wrote:

> SOC have internal I/O buses that can't be probed for devices. The
> devices on the buses can be accessed directly without additinal
> configuration required. This type of bus is represented as
> "simple-bus". In some platforms, we name "soc" with "simple-bus"
> attribute and many devices are hooked under and desribe them in DT
> (device tree).
> 
> In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's
> parent's lock during probe and remove")' to solve USB subsystem
> lock sequence since usb device's characteristic. Thus "soc"
> needs to be locked whenever a device and driver's probing
> happen under "soc" bus. During this period, an async driver
> tries to probe a device which is under the "soc" bus would be
> blocked until previous driver finish the probing and release "soc"
> lock. And the next probing under the "soc" bus need to wait for
> async finish. Because of that, driver's async probe for init
> time improvement will be shadowed.
> 
> Since many devices don't have USB devices' characteristic, they
> actually don't need parent's lock. However, in order to control
> the risk and minimize the impact, we don't request parent's lock
> only when a driver requests async probe.
> 
> Async probe could have more benefit after we have this patch.
> 
> Signed-off-by: martin_liu <liumartin@google.com>
> ---
> This RFC is asked to get some feedback since it involed driver
> core and USB subsystem. I'm not familiar with USB subsystem and
> not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the
> device's parent's lock during probe and remove")' since it has
> been there over 10 years. If we still need it and hard to fix it
> , the simple way is to find a place not to allow USB subsystem
> drivers to have async probe capability. Any suggestion is welcome.

I don't think the "allows_async_probing" attribute is the best way to 
attack this.  Some other approach, like a special-purpose flag, might 
be better.

Yes, USB still needs to have parent's locks held during probing.  
Here's the reason.  A USB device can have multiple interfaces, each
bound to its own driver.  A driver may sometimes need to issue a reset,
but in USB there's no way to reset a single interface.  Only the entire
device can be reset, and of course this affects all the interfaces.  
Therefore a driver needs to acquire the device lock before it can issue
a reset.

The problem is that the driver's thread may already hold the device
lock.  During a normal probe sequence, for example, the interfaces get
probed by the hub driver while it owns the device lock.  But for probes
under other circumstances (for example, if the user writes to the
driver's "bind" attribute in sysfs), the device lock might not be held.

A driver cannot tell these two cases apart.  The only way to make it
work all the time is to have the caller _always_ hold the device lock
while the driver is probed (or the removed, for that matter).

Alan Stern

WARNING: multiple messages have this Message-ID (diff)
From: Alan Stern <stern@rowland.harvard.edu>
To: martin_liu <liumartin@google.com>
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: [RFC] driver core: don't hold dev's parent lock when using async probe
Date: Tue, 22 May 2018 13:09:44 -0400 (EDT)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1805221250480.1466-100000@iolanthe.rowland.org> (raw)

On Tue, 22 May 2018, martin_liu wrote:

> SOC have internal I/O buses that can't be probed for devices. The
> devices on the buses can be accessed directly without additinal
> configuration required. This type of bus is represented as
> "simple-bus". In some platforms, we name "soc" with "simple-bus"
> attribute and many devices are hooked under and desribe them in DT
> (device tree).
> 
> In commit 'bf74ad5bc417 introduce ("[PATCH] Hold the device's
> parent's lock during probe and remove")' to solve USB subsystem
> lock sequence since usb device's characteristic. Thus "soc"
> needs to be locked whenever a device and driver's probing
> happen under "soc" bus. During this period, an async driver
> tries to probe a device which is under the "soc" bus would be
> blocked until previous driver finish the probing and release "soc"
> lock. And the next probing under the "soc" bus need to wait for
> async finish. Because of that, driver's async probe for init
> time improvement will be shadowed.
> 
> Since many devices don't have USB devices' characteristic, they
> actually don't need parent's lock. However, in order to control
> the risk and minimize the impact, we don't request parent's lock
> only when a driver requests async probe.
> 
> Async probe could have more benefit after we have this patch.
> 
> Signed-off-by: martin_liu <liumartin@google.com>
> ---
> This RFC is asked to get some feedback since it involed driver
> core and USB subsystem. I'm not familiar with USB subsystem and
> not sure if we still need 'bf74ad5bc417 ("[PATCH] Hold the
> device's parent's lock during probe and remove")' since it has
> been there over 10 years. If we still need it and hard to fix it
> , the simple way is to find a place not to allow USB subsystem
> drivers to have async probe capability. Any suggestion is welcome.

I don't think the "allows_async_probing" attribute is the best way to 
attack this.  Some other approach, like a special-purpose flag, might 
be better.

Yes, USB still needs to have parent's locks held during probing.  
Here's the reason.  A USB device can have multiple interfaces, each
bound to its own driver.  A driver may sometimes need to issue a reset,
but in USB there's no way to reset a single interface.  Only the entire
device can be reset, and of course this affects all the interfaces.  
Therefore a driver needs to acquire the device lock before it can issue
a reset.

The problem is that the driver's thread may already hold the device
lock.  During a normal probe sequence, for example, the interfaces get
probed by the hub driver while it owns the device lock.  But for probes
under other circumstances (for example, if the user writes to the
driver's "bind" attribute in sysfs), the device lock might not be held.

A driver cannot tell these two cases apart.  The only way to make it
work all the time is to have the caller _always_ hold the device lock
while the driver is probed (or the removed, for that matter).

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2018-05-22 17:09 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-22 14:12 [RFC] driver core: don't hold dev's parent lock when using async probe martin_liu
2018-05-22 14:12 ` martin_liu
2018-05-22 17:09 ` Alan Stern [this message]
2018-05-22 17:09   ` Alan Stern
2018-05-24 14:00   ` Martin Liu
2018-05-24 14:00     ` martin_liu
2018-05-24 15:02     ` Alan Stern
2018-05-24 15:02       ` Alan Stern
2018-05-24 16:05       ` Martin Liu
2018-05-24 16:05         ` martin_liu
2018-05-29  7:07       ` [RFC PATCH v2] driver core: hold dev's parent lock when needed martin_liu
2018-05-29  7:07         ` [RFC,v2] " martin_liu
2018-05-29  7:47         ` [RFC PATCH v2] " Greg KH
2018-05-29  7:47           ` [RFC,v2] " Greg Kroah-Hartman
2018-05-29 14:07         ` [RFC PATCH v2] " Alan Stern
2018-05-29 14:07           ` [RFC,v2] " Alan Stern
2018-05-29 16:34           ` [RFC PATCH v3] " Martin Liu
2018-05-29 16:34             ` [RFC,v3] " martin_liu
2018-05-29 16:59             ` [RFC PATCH v3] " Greg KH
2018-05-29 16:59               ` [RFC,v3] " Greg Kroah-Hartman
2018-05-29 17:08             ` [RFC PATCH v3] " Andy Shevchenko
2018-05-29 17:08               ` [RFC,v3] " Andy Shevchenko
2018-05-29 18:49             ` [RFC PATCH v3] " Alan Stern
2018-05-29 18:49               ` [RFC,v3] " Alan Stern
2018-05-30 16:31               ` [PATCH v4] " Martin Liu
2018-05-30 16:31                 ` [v4] " martin_liu
2018-05-30 17:21                 ` [PATCH v4] " Alan Stern
2018-05-30 17:21                   ` [v4] " Alan Stern
2018-05-31  6:31                   ` [PATCH v4] " Greg KH
2018-05-31  6:31                     ` [v4] " Greg Kroah-Hartman
2018-05-31  7:27                     ` [PATCH v4] " Martin Liu
2018-05-31  7:27                       ` [v4] " martin_liu
2018-05-31 18:55                       ` [PATCH v4] " Andy Shevchenko
2018-05-31 18:55                         ` [v4] " Andy Shevchenko
2018-05-29 15:28         ` [RFC PATCH v2] " Andy Shevchenko
2018-05-29 15:28           ` [RFC,v2] " Andy Shevchenko

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=Pine.LNX.4.44L0.1805221250480.1466-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=liumartin@google.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.