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

[stuart hayes]

On 9/12/2024 9:30 AM, David Jeffery wrote:
> On Tue, Sep 10, 2024 at 8:14 PM stuart hayes <stuart.w.hayes@xxxxxxxxx> 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);