Re: [PATCH 6/9] driver core: async device shutdown infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] driver core: premature completion in kill_device() races with device_del()
- [Low] driver core: missing kernel-doc for DEV_FLAG_ASYNC_SHUTDOWN
--

commit 82979b053d4cad6e7ae5134feb8f79f9f979e23e
Author: David Jeffery <djeffery@redhat.com>

driver core: async device shutdown infrastructure

This commit introduces asynchronous device shutdown support, allowing
devices flagged with DEV_FLAG_ASYNC_SHUTDOWN to execute their shutdown
callbacks in parallel after waiting for their child and consumer
dependencies to finish shutting down.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 93605698affcc..7fee6043dd375 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -3925,6 +3932,7 @@ bool kill_device(struct device *dev)
>  	if (dev->p->dead)
>  		return false;
>  	dev->p->dead = true;
> +	complete_all(&dev->p->complete);
>  	return true;
>  }
>  EXPORT_SYMBOL_GPL(kill_device);

[Severity: High]
Does signaling dev->p->complete in kill_device() wake parent and supplier
devices too early when a child or consumer device is concurrently removed
via device_del()?

In device_del(), kill_device() is called right at the start before dropping
device_lock(dev), while the actual driver unbinding and resource release
happen later:

drivers/base/core.c:device_del() {
	device_lock(dev);
	kill_device(dev);
	device_unlock(dev);
	...
		list_for_each_entry(class_intf, &sp->interfaces, node)
			if (class_intf->remove_dev)
				class_intf->remove_dev(dev);
	...
	bus_remove_device(dev);
	device_pm_remove(dev);
	...
	devres_release_all(dev);
}

During device_shutdown(), parent and supplier devices wait for child and
consumer devices in wait_for_shutdown_dependencies() ->
wait_for_device_shutdown(), which only waits on dev->p->complete:

	if (async || wants_async_shutdown(dev))
		wait_for_completion(&dev->p->complete);

Meanwhile, __shutdown_one_device() returns immediately without waiting if
dev->p->dead is set:

	if (!dev->p || dev->p->dead)
		return;

Before this change, a deleting child or consumer stayed on
devices_kset->list until kobject_del() at the end of device_del(), and
shutdown_one_device() serialized on device_lock(dev) against
bus_remove_device() -> device_release_driver() before device_shutdown()
moved on to the parent or supplier. Similarly, for async suspend,
device_pm_remove() does not call complete_all(&dev->power.completion) until
after bus_remove_device() has completed.

By completing dev->p->complete inside kill_device(), can a parent or
supplier proceed with its ->shutdown() callback and shut down a bus or
controller while its child or consumer is still executing
class_intf->remove_dev(), ->remove(), or devres_release_all()?

[ ... ]

> diff --git a/include/linux/device.h b/include/linux/device.h
> index aee79fd6b32b4..1ebafa7984621 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -612,6 +612,7 @@ enum struct_device_flags {
>  	DEV_FLAG_OF_NODE_REUSED = 7,
>  	DEV_FLAG_OFFLINE_DISABLED = 8,
>  	DEV_FLAG_OFFLINE = 9,
> +	DEV_FLAG_ASYNC_SHUTDOWN = 10,
>  
>  	DEV_FLAG_COUNT
>  };

[Severity: Low]
enum struct_device_flags has a kernel-doc comment immediately above it (which
is parsed via the :internal: directive in
Documentation/driver-api/infrastructure.rst). Should an entry for
@DEV_FLAG_ASYNC_SHUTDOWN be added to that comment to avoid a kernel-doc
warning?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020164435.80499-1-djeffery@redhat.com?part=6