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

[stuart hayes <stuart.w.hayes@gmail.com>]

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);