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

[Ulf Hansson]

On Thu, Sep 24, 2026 at 9:30 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Fri, Jun 26, 2026 at 9:33 AM Brian Masney <bmasney@xxxxxxxxxx> 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.
>
> I had already thought about this scenario when I implemented the
> framework. Your driver should just set its sync_state() callback and
> that my_drv_sync_state() will simply call the sync state helper
> functions provided by the relevant frameworks.

That would work, but would require all drivers to assign the
->sync_state() callbacks themselves. The core of each subsystem will
therefore not be able to help much, except for providing the helper
functions.

I can imagine a lot of boilerplate code, but perhaps the flexibility
is worth it.

Also note that, we need to address the situation where a child node
acts as the provider, but doesn't have a populated device for it.

>
> This link list is adding a lot of code without any clear benefit. And
> it also doesn't work for drivers that might care for a particular
> sequence of framework sync_state() callbacks. Instead your are forced
> to follow whatever order in which the frameworks are adding the sync
> state callbacks to your device.

Right, if the order is important, the "parent" driver would need to
control the order for how providers are registered. Yet, I think they
already need to do that (if it's important), although for other
reasons.

>
> I don't think this patch is a good idea.

You may very well be right, but I think we need to discuss it a bit
more. Let's chat at LPC next week.

>
> -Saravana

Kind regards
Uffe

>
> >
> > Link: https://lore.kernel.org/linux-clk/CAPx+jO9JiV16ePLk59hTQzEMnA96Va6Ns4jqJbwyZ6oTT0AjXA@xxxxxxxxxxxxxx/
> > Signed-off-by: Brian Masney <bmasney@xxxxxxxxxx>
> > Assisted-by: Claude:claude-opus-4-6
> > ---
> >  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)
> >  {
> >
> > --
> > 2.54.0
> >