Re: [PATCH v2 1/9] driver core: Enable suppliers to implement fine grained sync_state support

Danilo Krummrich
2026-04-18T11:23:50Z

On Fri Apr 10, 2026 at 12:40 PM CEST, Ulf Hansson wrote:
> The common sync_state support isn't fine grained enough for some types of
> suppliers, like power domains for example. Especially when a supplier
> provides multiple independent power domains, each with their own set of
> consumers. In these cases we need to wait for all consumers for all the
> provided power domains before invoking the supplier's ->sync_state().
>
> To allow a more fine grained sync_state support to be implemented on per
> supplier's driver basis, let's add a new optional callback. As soon as
> there is an update worth to consider in regards to managing sync_state for
> a supplier device, __device_links_queue_sync_state() invokes the callback.
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/core.c | 7 ++++++-
> include/linux/device/driver.h | 7 +++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 09b98f02f559..4085a011d8ca 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1106,7 +1106,9 @@ int device_links_check_suppliers(struct device *dev)
> * Queues a device for a sync_state() callback when the device links write lock
> * isn't held. This allows the sync_state() execution flow to use device links
> * APIs. The caller must ensure this function is called with
> - * device_links_write_lock() held.
> + * device_links_write_lock() held. Note, if the optional queue_sync_state()
> + * callback has been assigned too, it gets called for every update to allowing a

s/allowing/allow/

> + * more fine grained support to be implemented on per supplier basis.
> *
> * This function does a get_device() to make sure the device is not freed while
> * on this list.
> @@ -1126,6 +1128,9 @@ static void __device_links_queue_sync_state(struct device *dev,
> if (dev->state_synced)
> return;
>
> + if (dev->driver && dev->driver->queue_sync_state)
> + dev->driver->queue_sync_state(dev);

This seems to be called without the device lock being held, which seems to allow
the queue_sync_state() callback to execute concurrently with remove(). This
opens the door for all kinds of UAF conditions in drivers.

This also made me aware that the above dev_has_sync_state() is probably broken,
as it also performs the following check without the device lock being held.

dev->driver && dev->driver->sync_state

I think nothing prevents dev->driver to become NULL concurrently; in this case
READ_ONCE() should be sufficient though as it doesn't execute the callback.

I will send a patch for this.

> +
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> if (!device_link_test(link, DL_FLAG_MANAGED))
> continue;
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index bbc67ec513ed..bc9ae1cbe03c 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -68,6 +68,12 @@ enum probe_type {
> * be called at late_initcall_sync level. If the device has
> * consumers that are never bound to a driver, this function
> * will never get called until they do.
> + * @queue_sync_state: Similar to the ->sync_state() callback, but called to
> + * allow syncing device state to software state in a more fine
> + * grained way. It is called when there is an updated state that
> + * may be worth to consider for any of the consumers linked to
> + * this device. If implemented, the ->sync_state() callback is
> + * required too.

What happens if this is not the case? Maybe worth to check and warn about this
in driver_register().

> * @remove: Called when the device is removed from the system to
> * unbind a device from this driver.
> * @shutdown: Called at shut-down time to quiesce the device.
> @@ -110,6 +116,7 @@ struct device_driver {
>
> int (*probe) (struct device *dev);
> void (*sync_state)(struct device *dev);
> + void (*queue_sync_state)(struct device *dev);
> int (*remove) (struct device *dev);
> void (*shutdown) (struct device *dev);
> int (*suspend) (struct device *dev, pm_message_t state);
> --
> 2.43.0