Re: [PATCH v8 3/4] driver core: shut down devices asynchronously

stuart hayes
2024-09-12T16:20:09Z

On 9/12/2024 9:30 AM, David Jeffery wrote:
> On Tue, Sep 10, 2024 at 8:14 PM stuart hayes <stuart.w.hayes@gmail.com> wrote:
>>
> ...
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index b69b82da8837..52d64b419c01 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -4832,6 +4832,13 @@ static void shutdown_one_device_async(void *data, async_cookie_t cookie)
>> {
>> struct device *dev = data;
>>
>> + /*
>> + * Sanity check to prevent shutdown hang in case a parent or supplier
>> + * is in devices_kset list in the wrong order
>> + */
>> + if (dev->p->shutdown_after > cookie)
>> + dev->p->shutdown_after = cookie - 1;
>> +
>> async_synchronize_cookie_domain(dev->p->shutdown_after + 1, &sd_domain);
>>
>> shutdown_one_device(dev);
>
> While the race window is really small, there is a potential race with
> this fixup. It's possible for the shutdown operation to write a new
> value to shutdown_after in the time between the if check and
> shutdown_after being re-read and used in the
> async_synchronize_cookie_domain call. Such a race would allow a too
> high value to be used.
>
> Instead, could do something like:
>
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4833,8 +4833,12 @@ static void shutdown_one_device(struct device *dev)
> static void shutdown_one_device_async(void *data, async_cookie_t cookie)
> {
> struct device *dev = data;
> + async_cookie_t wait = dev->p->shutdown_after + 1;
>
> - async_synchronize_cookie_domain(dev->p->shutdown_after + 1, &sd_domain);
> + if (wait > cookie)
> + wait = cookie;
> +
> + async_synchronize_cookie_domain(wait, &sd_domain);
>
> shutdown_one_device(dev);
> }
>
> This reads the shutdown_after value once and avoids the race window
> where its value being changed on another CPU could still cause a
> potential deadlock.
>

Good point. Really that sanity check shouldn't be needed at all. But... maybe it
would be better to just not change the shutdown_after on any device that's
already been scheduled for shutdown... this would work regardless of why the supplier
and consumer devices are in the wrong order on the devices_kset list, and would still
work if supplier/consumer devices don't get reordered for some reason other than
the devlink being sync_state only in the future. Plus, it's a bit simpler.

How does this look?

diff --git a/drivers/base/base.h b/drivers/base/base.h
index ea18aa70f151..f818a0251bb7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -105,6 +105,8 @@ struct driver_private {
* @dead - This device is currently either in the process of or has been
* removed from the system. Any asynchronous events scheduled for this
* device should exit without taking any action.
+ * @shutdown_scheduled - asynchronous shutdown of the device has already
+ * been scheduled
*
* Nothing outside of the driver core should ever touch these fields.
*/
@@ -120,6 +122,7 @@ struct device_private {
async_cookie_t shutdown_after;
struct device *device;
u8 dead:1;
+ u8 shutdown_scheduled:1;
};
#define to_device_private_parent(obj) \
container_of(obj, struct device_private, knode_parent)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index b69b82da8837..bd6bc4a3dc15 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4888,6 +4888,8 @@ void device_shutdown(void)

cookie = async_schedule_domain(shutdown_one_device_async,
dev, &sd_domain);
+ dev->p->shutdown_scheduled = 1;
+
/*
* Ensure parent & suppliers wait for this device to shut down
*/
@@ -4898,8 +4900,18 @@ void device_shutdown(void)

idx = device_links_read_lock();
list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
- device_links_read_lock_held())
- link->supplier->p->shutdown_after = cookie;
+ device_links_read_lock_held()) {
+ /*
+ * Only update cookie if device shutdown hasn't
+ * already been scheduled. Some supplier/consumer
+ * devices (sync_state only) aren't reordered on
+ * devices_kset list and don't need this, and setting
+ * this could result in a circular dependency if the
+ * supplier shutdown has already been scheduled.
+ */
+ if (!link->supplier->p->shutdown_scheduled)
+ link->supplier->p->shutdown_after = cookie;
+ }
device_links_read_unlock(idx);
put_device(dev);