Re: [PATCH 1/4] driver: core: introduce dev_add_sync_state()

From: Sverdlin, Alexander

Date: Tue Sep 22 2026 - 07:58:01 EST

Hi Brian,

On Fri, 2026-06-26 at 12:32 -0400, Brian Masney wrote:
> We have cases where a device node represents a provider for multiple
> types of resources, like clocks, power-domains, resets, etc. We
> currently have dev_set_drv_sync_state() where a framework or driver
> can set the sync_state callback for a device node, however it currently
> only supports a single sync_state callback.
>
> The pmdomain subsystem currently sets up a sync_state callback in the
> core framework, and the clk subsystem will setup it's own separate
> sync_state callback in the core framework. These can collide with each
> other on some types of devices that have multiple types of resources.
> Additionally, some clk drivers already have their own separate
> sync_state callback already defined.
>
> Let's introduce support for allowing drivers and frameworks to add their
> own sync_state callback via a new function dev_add_sync_state() so that
> multiple sync_state callbacks can coexist.
>
> Link: https://lore.kernel.org/linux-clk/CAPx+jO9JiV16ePLk59hTQzEMnA96Va6Ns4jqJbwyZ6oTT0AjXA@xxxxxxxxxxxxxx/
> Signed-off-by: Brian Masney <bmasney@xxxxxxxxxx>
> Assisted-by: Claude:claude-opus-4-6

As long as dev_add_sync_state() is used only by clk and pmdomain — which
both add to the provider from its own single-threaded ->probe() under
device_lock() — the unlocked list is safe.
But once it becomes a generic API, callers that add from other contexts
will probably need locking, e.g. two consumers registering a callback on
a shared provider concurrently would corrupt the list via racing
list_add_tail(), and a post-probe registration racing dev_sync_state()
would either double-invoke a callback (dedup TOCTOU) or walk a
half-inserted node.

Maybe the constraints to keep the list lockless want to be documented...

Other than that,
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>

> ---
> drivers/base/base.h    | 7 +++++++
> drivers/base/core.c    | 29 +++++++++++++++++++++++++++++
> include/linux/device.h | 11 +++++++++++
> 3 files changed, 47 insertions(+)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index a5b7abc10ff0..339db4afbeb4 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -178,6 +178,8 @@ static inline bool dev_has_sync_state(struct device *dev)
>
> if (!dev)
> return false;
> + if (!list_empty(&dev->sync_state_list))
> + return true;
> drv = READ_ONCE(dev->driver);
> if (drv && drv->sync_state)
> return true;
> @@ -188,10 +190,15 @@ static inline bool dev_has_sync_state(struct device *dev)
>
> static inline void dev_sync_state(struct device *dev)
> {
> + struct sync_state_entry *entry;
> +
> if (dev->bus->sync_state)
> dev->bus->sync_state(dev);
> else if (dev->driver && dev->driver->sync_state)
> dev->driver->sync_state(dev);
> +
> + list_for_each_entry(entry, &dev->sync_state_list, node)
> + entry->fn(dev);
> }
>
> int driver_add_groups(const struct device_driver *drv,
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4d026682944f..acc12f402dd3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2612,6 +2612,7 @@ EXPORT_SYMBOL_GPL(device_show_string);
> static void device_release(struct kobject *kobj)
> {
> struct device *dev = kobj_to_dev(kobj);
> + struct sync_state_entry *entry, *tmp;
> struct device_private *p = dev->p;
>
> /*
> @@ -2625,6 +2626,11 @@ static void device_release(struct kobject *kobj)
> */
> devres_release_all(dev);
>
> + list_for_each_entry_safe(entry, tmp, &dev->sync_state_list, node) {
> + list_del(&entry->node);
> + kfree(entry);
> + }
> +
> kfree(dev->dma_range_map);
> kfree(dev->driver_override.name);
>
> @@ -3239,12 +3245,35 @@ void device_initialize(struct device *dev)
> INIT_LIST_HEAD(&dev->links.consumers);
> INIT_LIST_HEAD(&dev->links.suppliers);
> INIT_LIST_HEAD(&dev->links.defer_sync);
> + INIT_LIST_HEAD(&dev->sync_state_list);
> dev->links.status = DL_DEV_NO_DRIVER;
> dev_assign_dma_coherent(dev, dma_default_coherent);
> swiotlb_dev_init(dev);
> }
> EXPORT_SYMBOL_GPL(device_initialize);
>
> +int dev_add_sync_state(struct device *dev,
> +    void (*fn)(struct device *dev))
> +{
> + struct sync_state_entry *entry;
> +
> + if (!dev || !dev->driver)
> + return 0;
> +
> + list_for_each_entry(entry, &dev->sync_state_list, node)
> + if (entry->fn == fn)
> + return 0;
> +
> + entry = kmalloc_obj(*entry);
> + if (!entry)
> + return -ENOMEM;
> +
> + entry->fn = fn;
> + list_add_tail(&entry->node, &dev->sync_state_list);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dev_add_sync_state);
> +
> struct kobject *virtual_device_parent(void)
> {
> static struct kobject *virtual_dir = NULL;
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 7b2baffdd2f5..b7a3dd4b56ed 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -642,6 +642,8 @@ enum struct_device_flags {
> * @driver_override: Driver name to force a match. Do not touch directly; use
> *      device_set_driver_override() instead.
> * @links: Links to suppliers and consumers of this device.
> + * @sync_state_list: List of sync_state callbacks added by subsystem
> + * frameworks (e.g. clk, genpd) via dev_add_sync_state().
> * @power: For device power management.
> * See Documentation/driver-api/pm/devices.rst for details.
> * @pm_domain: Provide callbacks that are executed during system suspend,
> @@ -723,6 +725,7 @@ struct device {
> */
>
> struct dev_links_info links;
> + struct list_head sync_state_list;
> struct dev_pm_info power;
> struct dev_pm_domain *pm_domain;
>
> @@ -1137,6 +1140,14 @@ static inline int dev_set_drv_sync_state(struct device *dev,
> return 0;
> }
>
> +struct sync_state_entry {
> + struct list_head node;
> + void (*fn)(struct device *dev);
> +};
> +
> +int dev_add_sync_state(struct device *dev,
> +    void (*fn)(struct device *dev));
> +
> static inline void dev_set_removable(struct device *dev,
>      enum device_removable removable)
> {

--
Alexander Sverdlin
Siemens AG
www.siemens.com