The onboard_hub 'driver' consists of two drivers, a platform
driver and a USB driver. Currently when the onboard hub driver
is initialized it first registers the platform driver, then the
USB driver. This results in a race condition when the 'attach'
work is executed, which is scheduled when the platform device
is probed. The purpose of fhe 'attach' work is to bind elegible
USB hub devices to the onboard_hub USB driver. This fails if
the work runs before the USB driver has been registered.
Register the USB driver first, then the platform driver. This
increases the chances that the onboard_hub USB devices are probed
before their corresponding platform device, which the USB driver
tries to locate in _probe(). The driver already handles this
situation and defers probing if the onboard hub platform device
doesn't exist yet.
Cc: [email protected]
Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver")
Link: https://lore.kernel.org/lkml/[email protected]/T/#m0d64295f017942fd988f7c53425db302d61952b4
Reported-by: Alexander Stein <[email protected]>
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
(no changes since v1)
drivers/usb/misc/onboard_usb_hub.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 94e7966e199d..db0844b30bbd 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -433,13 +433,13 @@ static int __init onboard_hub_init(void)
{
int ret;
- ret = platform_driver_register(&onboard_hub_driver);
+ ret = usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE);
if (ret)
return ret;
- ret = usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE);
+ ret = platform_driver_register(&onboard_hub_driver);
if (ret)
- platform_driver_unregister(&onboard_hub_driver);
+ usb_deregister_device_driver(&onboard_hub_usbdev_driver);
return ret;
}
--
2.39.0.314.g84b9a713c41-goog
Currently each onboard_hub platform device owns an 'attach' work,
which is scheduled when the device probes. With this deadlocks
have been reported on a Raspberry Pi 3 B+ [1], which has nested
onboard hubs.
The flow of the deadlock is something like this (with the onboard_hub
driver built as a module) [2]:
- USB root hub is instantiated
- core hub driver calls onboard_hub_create_pdevs(), which creates the
'raw' platform device for the 1st level hub
- 1st level hub is probed by the core hub driver
- core hub driver calls onboard_hub_create_pdevs(), which creates
the 'raw' platform device for the 2nd level hub
- onboard_hub platform driver is registered
- platform device for 1st level hub is probed
- schedules 'attach' work
- platform device for 2nd level hub is probed
- schedules 'attach' work
- onboard_hub USB driver is registered
- device (and parent) lock of hub is held while the device is
re-probed with the onboard_hub driver
- 'attach' work (running in another thread) calls driver_attach(), which
blocks on one of the hub device locks
- onboard_hub_destroy_pdevs() is called by the core hub driver when one
of the hubs is detached
- destroying the pdevs invokes onboard_hub_remove(), which waits for the
'attach' work to complete
- waits forever, since the 'attach' work can't acquire the device lock
Use a single work struct for the driver instead of having a work struct
per onboard hub platform driver instance. With that it isn't necessary
to cancel the work in onboard_hub_remove(), which fixes the deadlock.
The work is only cancelled when the driver is unloaded.
[1] https://lore.kernel.org/r/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Cc: [email protected]
Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver")
Link: https://lore.kernel.org/r/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Reported-by: Stefan Wahren <[email protected]>
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- drop loop in onboard_hub_probe() to wait for an already running
'attach' work to finish. The loop can cause deadlocks and is not
needed.
Rationale for why the loop in onboard_hub_probe() isn't needed:
The idea behind the loop was: The currently running work might not take
into account the USB devices of the hub that is currently probed, which
should probe shortly after the hub was powered on.
The 'attach' work is only needed for USB devices that were previously
detached through device_release_driver() in onboard_hub_remove(). These
USB device objects only persist in the kernel if the hub is not powered
off (or put into reset) by onboard_hub_remove().
If onboard_hub_probe() is invoked and the USB device objects persisted,
then an already running 'attach' work should take them into account. If
they didn't persist the running work might miss them, but that wouldn't
be a problem since the newly created USB devices don't need to be
explicitly attached because they weren't detached previously.
drivers/usb/misc/onboard_usb_hub.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index db0844b30bbd..969c4c4f2ae9 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -27,7 +27,10 @@
#include "onboard_usb_hub.h"
+static void onboard_hub_attach_usb_driver(struct work_struct *work);
+
static struct usb_device_driver onboard_hub_usbdev_driver;
+static DECLARE_WORK(attach_usb_driver_work, onboard_hub_attach_usb_driver);
/************************** Platform driver **************************/
@@ -45,7 +48,6 @@ struct onboard_hub {
bool is_powered_on;
bool going_away;
struct list_head udev_list;
- struct work_struct attach_usb_driver_work;
struct mutex lock;
};
@@ -271,8 +273,7 @@ static int onboard_hub_probe(struct platform_device *pdev)
* This needs to be done deferred to avoid self-deadlocks on systems
* with nested onboard hubs.
*/
- INIT_WORK(&hub->attach_usb_driver_work, onboard_hub_attach_usb_driver);
- schedule_work(&hub->attach_usb_driver_work);
+ schedule_work(&attach_usb_driver_work);
return 0;
}
@@ -285,9 +286,6 @@ static int onboard_hub_remove(struct platform_device *pdev)
hub->going_away = true;
- if (&hub->attach_usb_driver_work != current_work())
- cancel_work_sync(&hub->attach_usb_driver_work);
-
mutex_lock(&hub->lock);
/* unbind the USB devices to avoid dangling references to this device */
@@ -449,6 +447,8 @@ static void __exit onboard_hub_exit(void)
{
usb_deregister_device_driver(&onboard_hub_usbdev_driver);
platform_driver_unregister(&onboard_hub_driver);
+
+ cancel_work_sync(&attach_usb_driver_work);
}
module_exit(onboard_hub_exit);
--
2.39.0.314.g84b9a713c41-goog
Hi Matthias,
Am 10.01.23 um 18:32 schrieb Matthias Kaehlcke:
> The onboard_hub 'driver' consists of two drivers, a platform
> driver and a USB driver. Currently when the onboard hub driver
> is initialized it first registers the platform driver, then the
> USB driver. This results in a race condition when the 'attach'
> work is executed, which is scheduled when the platform device
> is probed. The purpose of fhe 'attach' work is to bind elegible
> USB hub devices to the onboard_hub USB driver. This fails if
> the work runs before the USB driver has been registered.
>
> Register the USB driver first, then the platform driver. This
> increases the chances that the onboard_hub USB devices are probed
> before their corresponding platform device, which the USB driver
> tries to locate in _probe(). The driver already handles this
> situation and defers probing if the onboard hub platform device
> doesn't exist yet.
>
> Cc: [email protected]
> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver")
> Link: https://lore.kernel.org/lkml/[email protected]/T/#m0d64295f017942fd988f7c53425db302d61952b4
> Reported-by: Alexander Stein <[email protected]>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
the whole series is:
Tested-by: Stefan Wahren <[email protected]>
Thanks \o/