| Message ID | 20180522141227.GA118442@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote: > On Tue, 22 May 2018, martin_liu wrote: > > > 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 Thanks for the reply and more detail about the backgroud. I'd like to have a conclusion about it. Please kindly correct me if my understanding is wrong. Regarding to the "special-purpose flag", do you mean we could find a place in USB subsystem to have the flag set (not sure if it's easy to find it). Driver core would be base on the flag to decide if we need to hold the device's parent's lock. Martin -- 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
On Thu, 24 May 2018, Martin Liu wrote: > On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote: > > On Tue, 22 May 2018, martin_liu wrote: > > > > > 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 > > Thanks for the reply and more detail about the backgroud. I'd like to > have a conclusion about it. Please kindly correct me if my understanding > is wrong. Regarding to the "special-purpose flag", do you mean we could > find a place in USB subsystem to have the flag set (not sure if it's > easy to find it). Driver core would be base on the flag to decide if we > need to hold the device's parent's lock. Yes, except that the flag would not be in the USB subsystem. It would be in the device, device_type, or bus_type structure, so that the driver core could access it. 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
On Thu, May 24, 2018 at 11:02:57AM -0400, Alan Stern wrote: > On Thu, 24 May 2018, Martin Liu wrote: > > > On Tue, May 22, 2018 at 01:09:44PM -0400, Alan Stern wrote: > > > On Tue, 22 May 2018, martin_liu wrote: > > > > > > > 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 > > > > Thanks for the reply and more detail about the backgroud. I'd like to > > have a conclusion about it. Please kindly correct me if my understanding > > is wrong. Regarding to the "special-purpose flag", do you mean we could > > find a place in USB subsystem to have the flag set (not sure if it's > > easy to find it). Driver core would be base on the flag to decide if we > > need to hold the device's parent's lock. > > Yes, except that the flag would not be in the USB subsystem. It would > be in the device, device_type, or bus_type structure, so that the > driver core could access it. > > Alan Stern Thanks for the quick feedback and the suggestion. will try to figure out how it works. Martin -- 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
diff --git a/drivers/base/bus.c b/drivers/base/bus.c index ef6183306b40..6434333995d4 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -181,13 +181,15 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf, struct bus_type *bus = bus_get(drv->bus); struct device *dev; int err = -ENODEV; + bool allow_async; + allow_async = driver_allows_async_probing(drv); dev = bus_find_device_by_name(bus, NULL, buf); if (dev && dev->driver == drv) { - if (dev->parent) /* Needed for USB */ + if (dev->parent && !allow_async)/* Needed for USB */ device_lock(dev->parent); device_release_driver(dev); - if (dev->parent) + if (dev->parent && !allow_async) device_unlock(dev->parent); err = count; } @@ -208,15 +210,17 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf, struct bus_type *bus = bus_get(drv->bus); struct device *dev; int err = -ENODEV; + bool allow_async; + allow_async = driver_allows_async_probing(drv); dev = bus_find_device_by_name(bus, NULL, buf); if (dev && dev->driver == NULL && driver_match_device(drv, dev)) { - if (dev->parent) /* Needed for USB */ + if (dev->parent && !allow_async)/* Needed for USB */ device_lock(dev->parent); device_lock(dev); err = driver_probe_device(drv, dev); device_unlock(dev); - if (dev->parent) + if (dev->parent && !allow_async) device_unlock(dev->parent); if (err > 0) { @@ -769,11 +773,14 @@ EXPORT_SYMBOL_GPL(bus_rescan_devices); */ int device_reprobe(struct device *dev) { + bool allow_async; + if (dev->driver) { - if (dev->parent) /* Needed for USB */ + allow_async = driver_allows_async_probing(dev->driver); + if (dev->parent && !allow_async)/* Needed for USB */ device_lock(dev->parent); device_release_driver(dev); - if (dev->parent) + if (dev->parent && !allow_async) device_unlock(dev->parent); } return bus_rescan_devices_helper(dev, NULL); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index c9f54089429b..36aed1576c58 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -794,6 +794,7 @@ static int __driver_attach(struct device *dev, void *data) { struct device_driver *drv = data; int ret; + bool allow_async; /* * Lock device and try to bind to it. We drop the error @@ -817,13 +818,14 @@ static int __driver_attach(struct device *dev, void *data) return ret; } /* ret > 0 means positive match */ - if (dev->parent) /* Needed for USB */ + allow_async = driver_allows_async_probing(drv); + if (dev->parent && !allow_async)/* Needed for USB */ device_lock(dev->parent); device_lock(dev); if (!dev->driver) driver_probe_device(drv, dev); device_unlock(dev); - if (dev->parent) + if (dev->parent && !allow_async) device_unlock(dev->parent); return 0; @@ -851,19 +853,21 @@ EXPORT_SYMBOL_GPL(driver_attach); static void __device_release_driver(struct device *dev, struct device *parent) { struct device_driver *drv; + bool allow_async; drv = dev->driver; if (drv) { - if (driver_allows_async_probing(drv)) + allow_async = driver_allows_async_probing(drv); + if (allow_async) async_synchronize_full(); while (device_links_busy(dev)) { device_unlock(dev); - if (parent) + if (parent && !allow_async) device_unlock(parent); device_links_unbind_consumers(dev); - if (parent) + if (parent && !allow_async) device_lock(parent); device_lock(dev); @@ -919,7 +923,12 @@ void device_release_driver_internal(struct device *dev, struct device_driver *drv, struct device *parent) { - if (parent) + bool allow_async = false; + + if (drv) + allow_async = driver_allows_async_probing(drv); + + if (parent && !allow_async) device_lock(parent); device_lock(dev); @@ -927,7 +936,7 @@ void device_release_driver_internal(struct device *dev, __device_release_driver(dev, parent); device_unlock(dev); - if (parent) + if (parent && !allow_async) device_unlock(parent); }
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. drivers/base/bus.c | 19 +++++++++++++------ drivers/base/dd.c | 23 ++++++++++++++++------- 2 files changed, 29 insertions(+), 13 deletions(-)