Re: [PATCH 3/7] component: Introduce struct aggregate_device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Replace 'struct master' with 'struct aggregate_device' and then rename
> 'master' to 'adev' everywhere in the code. While we're here, put a
> struct device inside the aggregate device so that we can register it
> with a bus_type in the next patch.
>
> The diff is large but that's because this is mostly a rename, where
> sometimes 'master' is replaced with 'adev' and other times it is
> replaced with 'parent' to indicate that the struct device that was being
> used is actually the parent of the aggregate device and driver.
>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
> Cc: Saravana Kannan <saravanak@xxxxxxxxxx>
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>  drivers/base/component.c  | 249 ++++++++++++++++++++------------------
>  include/linux/component.h |   2 +-
>  2 files changed, 134 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 5e79299f6c3f..55e30e0b0952 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -9,6 +9,7 @@
>   */
>  #include <linux/component.h>
>  #include <linux/device.h>
> +#include <linux/idr.h>
>  #include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> @@ -58,18 +59,21 @@ struct component_match {
>         struct component_match_array *compare;
>  };
>
> -struct master {
> +struct aggregate_device {
>         struct list_head node;
>         bool bound;
>
>         const struct component_master_ops *ops;
>         struct device *parent;
> +       struct device dev;
>         struct component_match *match;
> +
> +       int id;
>  };
>
>  struct component {
>         struct list_head node;
> -       struct master *master;
> +       struct aggregate_device *adev;
>         bool bound;
>
>         const struct component_ops *ops;
> @@ -79,7 +83,9 @@ struct component {
>
>  static DEFINE_MUTEX(component_mutex);
>  static LIST_HEAD(component_list);
> -static LIST_HEAD(masters);
> +static LIST_HEAD(aggregate_devices);
> +
> +static DEFINE_IDA(aggregate_ida);
>
>  #ifdef CONFIG_DEBUG_FS
>
> @@ -87,12 +93,12 @@ static struct dentry *component_debugfs_dir;
>
>  static int component_devices_show(struct seq_file *s, void *data)
>  {
> -       struct master *m = s->private;
> +       struct aggregate_device *m = s->private;
>         struct component_match *match = m->match;
>         size_t i;
>
>         mutex_lock(&component_mutex);
> -       seq_printf(s, "%-40s %20s\n", "master name", "status");
> +       seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
>         seq_puts(s, "-------------------------------------------------------------\n");
>         seq_printf(s, "%-40s %20s\n\n",
>                    dev_name(m->parent), m->bound ? "bound" : "not bound");
> @@ -122,46 +128,46 @@ static int __init component_debug_init(void)
>
>  core_initcall(component_debug_init);
>
> -static void component_master_debugfs_add(struct master *m)
> +static void component_master_debugfs_add(struct aggregate_device *m)
>  {
>         debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
>                             &component_devices_fops);
>  }
>
> -static void component_master_debugfs_del(struct master *m)
> +static void component_master_debugfs_del(struct aggregate_device *m)
>  {
>         debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
>  }
>
>  #else
>
> -static void component_master_debugfs_add(struct master *m)
> +static void component_master_debugfs_add(struct aggregate_device *m)
>  { }
>
> -static void component_master_debugfs_del(struct master *m)
> +static void component_master_debugfs_del(struct aggregate_device *m)
>  { }
>
>  #endif
>
> -static struct master *__master_find(struct device *parent,
> +static struct aggregate_device *__aggregate_find(struct device *parent,
>         const struct component_master_ops *ops)
>  {
> -       struct master *m;
> +       struct aggregate_device *m;
>
> -       list_for_each_entry(m, &masters, node)
> +       list_for_each_entry(m, &aggregate_devices, node)
>                 if (m->parent == parent && (!ops || m->ops == ops))
>                         return m;
>
>         return NULL;
>  }
>
> -static struct component *find_component(struct master *master,
> +static struct component *find_component(struct aggregate_device *adev,
>         struct component_match_array *mc)
>  {
>         struct component *c;
>
>         list_for_each_entry(c, &component_list, node) {
> -               if (c->master && c->master != master)
> +               if (c->adev && c->adev != adev)
>                         continue;
>
>                 if (mc->compare && mc->compare(c->dev, mc->data))
> @@ -175,101 +181,102 @@ static struct component *find_component(struct master *master,
>         return NULL;
>  }
>
> -static int find_components(struct master *master)
> +static int find_components(struct aggregate_device *adev)
>  {
> -       struct component_match *match = master->match;
> +       struct component_match *match = adev->match;
>         size_t i;
>         int ret = 0;
>
>         /*
>          * Scan the array of match functions and attach
> -        * any components which are found to this master.
> +        * any components which are found to this adev.
>          */
>         for (i = 0; i < match->num; i++) {
>                 struct component_match_array *mc = &match->compare[i];
>                 struct component *c;
>
> -               dev_dbg(master->parent, "Looking for component %zu\n", i);
> +               dev_dbg(adev->parent, "Looking for component %zu\n", i);
>
>                 if (match->compare[i].component)
>                         continue;
>
> -               c = find_component(master, mc);
> +               c = find_component(adev, mc);
>                 if (!c) {
>                         ret = -ENXIO;
>                         break;
>                 }
>
> -               dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
> +               dev_dbg(adev->parent, "found component %s, duplicate %u\n",
> +                       dev_name(c->dev), !!c->adev);
>
> -               /* Attach this component to the master */
> -               match->compare[i].duplicate = !!c->master;
> +               /* Attach this component to the adev */
> +               match->compare[i].duplicate = !!c->adev;
>                 match->compare[i].component = c;
> -               c->master = master;
> +               c->adev = adev;
>         }
>         return ret;
>  }
>
>  /* Detach component from associated master */
> -static void remove_component(struct master *master, struct component *c)
> +static void remove_component(struct aggregate_device *adev, struct component *c)
>  {
>         size_t i;
>
> -       /* Detach the component from this master. */
> -       for (i = 0; i < master->match->num; i++)
> -               if (master->match->compare[i].component == c)
> -                       master->match->compare[i].component = NULL;
> +       /* Detach the component from this adev. */
> +       for (i = 0; i < adev->match->num; i++)
> +               if (adev->match->compare[i].component == c)
> +                       adev->match->compare[i].component = NULL;
>  }
>
>  /*
> - * Try to bring up a master.  If component is NULL, we're interested in
> - * this master, otherwise it's a component which must be present to try
> - * and bring up the master.
> + * Try to bring up an aggregate device.  If component is NULL, we're interested
> + * in this aggregate device, otherwise it's a component which must be present
> + * to try and bring up the aggregate device.
>   *
>   * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
>   */
> -static int try_to_bring_up_master(struct master *master,
> +static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
>         struct component *component)
>  {
>         int ret;
>
> -       dev_dbg(master->parent, "trying to bring up master\n");
> +       dev_dbg(adev->parent, "trying to bring up adev\n");
>
> -       if (find_components(master)) {
> -               dev_dbg(master->parent, "master has incomplete components\n");
> +       if (find_components(adev)) {
> +               dev_dbg(adev->parent, "master has incomplete components\n");
>                 return 0;
>         }
>
> -       if (component && component->master != master) {
> -               dev_dbg(master->parent, "master is not for this component (%s)\n",
> +       if (component && component->adev != adev) {
> +               dev_dbg(adev->parent, "master is not for this component (%s)\n",
>                         dev_name(component->dev));
>                 return 0;
>         }
>
> -       if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> +       if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
>                 return -ENOMEM;
>
>         /* Found all components */
> -       ret = master->ops->bind(master->parent);
> +       ret = adev->ops->bind(adev->parent);
>         if (ret < 0) {
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>                 if (ret != -EPROBE_DEFER)
> -                       dev_info(master->parent, "master bind failed: %d\n", ret);
> +                       dev_info(adev->parent, "adev bind failed: %d\n", ret);
>                 return ret;
>         }
>
> -       master->bound = true;
> +       adev->bound = true;
>         return 1;
>  }
>
>  static int try_to_bring_up_masters(struct component *component)
>  {
> -       struct master *m;
> +       struct aggregate_device *adev;
>         int ret = 0;
>
> -       list_for_each_entry(m, &masters, node) {
> -               if (!m->bound) {
> -                       ret = try_to_bring_up_master(m, component);
> +       list_for_each_entry(adev, &aggregate_devices, node) {
> +               if (!adev->bound) {
> +                       ret = try_to_bring_up_aggregate_device(adev, component);
>                         if (ret != 0)
>                                 break;
>                 }
> @@ -278,12 +285,12 @@ static int try_to_bring_up_masters(struct component *component)
>         return ret;
>  }
>
> -static void take_down_master(struct master *master)
> +static void take_down_aggregate_device(struct aggregate_device *adev)
>  {
> -       if (master->bound) {
> -               master->ops->unbind(master->parent);
> -               devres_release_group(master->parent, NULL);
> -               master->bound = false;
> +       if (adev->bound) {
> +               adev->ops->unbind(adev->parent);
> +               devres_release_group(adev->parent, NULL);
> +               adev->bound = false;
>         }
>  }
>
> @@ -324,7 +331,7 @@ static int component_match_realloc(struct component_match *match, size_t num)
>         return 0;
>  }
>
> -static void __component_match_add(struct device *master,
> +static void __component_match_add(struct device *parent,
>         struct component_match **matchptr,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *),
> @@ -344,7 +351,7 @@ static void __component_match_add(struct device *master,
>                         return;
>                 }
>
> -               devres_add(master, match);
> +               devres_add(parent, match);
>
>                 *matchptr = match;
>         }
> @@ -370,13 +377,13 @@ static void __component_match_add(struct device *master,
>
>  /**
>   * component_match_add_release - add a component match entry with release callback
> - * @master: device with the aggregate driver
> + * @parent: parent device of the aggregate driver
>   * @matchptr: pointer to the list of component matches
>   * @release: release function for @compare_data
>   * @compare: compare function to match against all components
>   * @compare_data: opaque pointer passed to the @compare function
>   *
> - * Adds a new component match to the list stored in @matchptr, which the @master
> + * Adds a new component match to the list stored in @matchptr, which the
>   * aggregate driver needs to function. The list of component matches pointed to
>   * by @matchptr must be initialized to NULL before adding the first match. This
>   * only matches against components added with component_add().
> @@ -388,19 +395,19 @@ static void __component_match_add(struct device *master,
>   *
>   * See also component_match_add() and component_match_add_typed().
>   */
> -void component_match_add_release(struct device *master,
> +void component_match_add_release(struct device *parent,
>         struct component_match **matchptr,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *), void *compare_data)
>  {
> -       __component_match_add(master, matchptr, release, compare, NULL,
> +       __component_match_add(parent, matchptr, release, compare, NULL,
>                               compare_data);
>  }
>  EXPORT_SYMBOL(component_match_add_release);
>
>  /**
>   * component_match_add_typed - add a component match entry for a typed component
> - * @master: device with the aggregate driver
> + * @parent: parent device of the aggregate driver
>   * @matchptr: pointer to the list of component matches
>   * @compare_typed: compare function to match against all typed components
>   * @compare_data: opaque pointer passed to the @compare function
> @@ -415,32 +422,33 @@ EXPORT_SYMBOL(component_match_add_release);
>   *
>   * See also component_match_add_release() and component_match_add_typed().
>   */
> -void component_match_add_typed(struct device *master,
> +void component_match_add_typed(struct device *parent,
>         struct component_match **matchptr,
>         int (*compare_typed)(struct device *, int, void *), void *compare_data)
>  {
> -       __component_match_add(master, matchptr, NULL, NULL, compare_typed,
> +       __component_match_add(parent, matchptr, NULL, NULL, compare_typed,
>                               compare_data);
>  }
>  EXPORT_SYMBOL(component_match_add_typed);
>
> -static void free_master(struct master *master)
> +static void free_aggregate_device(struct aggregate_device *adev)
>  {
> -       struct component_match *match = master->match;
> +       struct component_match *match = adev->match;
>         int i;
>
> -       component_master_debugfs_del(master);
> -       list_del(&master->node);
> +       component_master_debugfs_del(adev);
> +       list_del(&adev->node);
>
>         if (match) {
>                 for (i = 0; i < match->num; i++) {
>                         struct component *c = match->compare[i].component;
>                         if (c)
> -                               c->master = NULL;
> +                               c->adev = NULL;
>                 }
>         }
>
> -       kfree(master);
> +       ida_free(&aggregate_ida, adev->id);
> +       kfree(adev);
>  }
>
>  /**
> @@ -459,31 +467,40 @@ int component_master_add_with_match(struct device *parent,
>         const struct component_master_ops *ops,
>         struct component_match *match)
>  {
> -       struct master *master;
> -       int ret;
> +       struct aggregate_device *adev;
> +       int ret, id;
>
>         /* Reallocate the match array for its true size */
>         ret = component_match_realloc(match, match->num);
>         if (ret)
>                 return ret;
>
> -       master = kzalloc(sizeof(*master), GFP_KERNEL);
> -       if (!master)
> +       adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +       if (!adev)
>                 return -ENOMEM;
>
> -       master->parent = parent;
> -       master->ops = ops;
> -       master->match = match;
> +       id = ida_alloc(&aggregate_ida, GFP_KERNEL);
> +       if (id < 0) {
> +               kfree(adev);
> +               return id;
> +       }
> +
> +       adev->id = id;
> +       adev->parent = parent;
> +       adev->dev.parent = parent;

Don't set adev->dev.parent. You are creating a functional 1-1
dependency where none exists. The real dependencies are the 1-many
dependencies between the aggregate and the components. Use device
links to capture that and enforce proper suspend/resume and runtime PM
ordering.

-Saravana

> +       adev->ops = ops;
> +       adev->match = match;
> +       dev_set_name(&adev->dev, "aggregate%d", id);
>
> -       component_master_debugfs_add(master);
> -       /* Add to the list of available masters. */
> +       component_master_debugfs_add(adev);
> +       /* Add to the list of available aggregate devices. */
>         mutex_lock(&component_mutex);
> -       list_add(&master->node, &masters);
> +       list_add(&adev->node, &aggregate_devices);
>
> -       ret = try_to_bring_up_master(master, NULL);
> +       ret = try_to_bring_up_aggregate_device(adev, NULL);
>
>         if (ret < 0)
> -               free_master(master);
> +               free_aggregate_device(adev);
>
>         mutex_unlock(&component_mutex);
>
> @@ -503,25 +520,25 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
>  void component_master_del(struct device *parent,
>         const struct component_master_ops *ops)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>
>         mutex_lock(&component_mutex);
> -       master = __master_find(parent, ops);
> -       if (master) {
> -               take_down_master(master);
> -               free_master(master);
> +       adev = __aggregate_find(parent, ops);
> +       if (adev) {
> +               take_down_aggregate_device(adev);
> +               free_aggregate_device(adev);
>         }
>         mutex_unlock(&component_mutex);
>  }
>  EXPORT_SYMBOL_GPL(component_master_del);
>
>  static void component_unbind(struct component *component,
> -       struct master *master, void *data)
> +       struct aggregate_device *adev, void *data)
>  {
>         WARN_ON(!component->bound);
>
>         if (component->ops && component->ops->unbind)
> -               component->ops->unbind(component->dev, master->parent, data);
> +               component->ops->unbind(component->dev, adev->parent, data);
>         component->bound = false;
>
>         /* Release all resources claimed in the binding of this component */
> @@ -539,26 +556,26 @@ static void component_unbind(struct component *component,
>   */
>  void component_unbind_all(struct device *parent, void *data)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>         struct component *c;
>         size_t i;
>
>         WARN_ON(!mutex_is_locked(&component_mutex));
>
> -       master = __master_find(parent, NULL);
> -       if (!master)
> +       adev = __aggregate_find(parent, NULL);
> +       if (!adev)
>                 return;
>
>         /* Unbind components in reverse order */
> -       for (i = master->match->num; i--; )
> -               if (!master->match->compare[i].duplicate) {
> -                       c = master->match->compare[i].component;
> -                       component_unbind(c, master, data);
> +       for (i = adev->match->num; i--; )
> +               if (!adev->match->compare[i].duplicate) {
> +                       c = adev->match->compare[i].component;
> +                       component_unbind(c, adev, data);
>                 }
>  }
>  EXPORT_SYMBOL_GPL(component_unbind_all);
>
> -static int component_bind(struct component *component, struct master *master,
> +static int component_bind(struct component *component, struct aggregate_device *adev,
>         void *data)
>  {
>         int ret;
> @@ -568,7 +585,7 @@ static int component_bind(struct component *component, struct master *master,
>          * This allows us to roll-back a failed component without
>          * affecting anything else.
>          */
> -       if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> +       if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
>                 return -ENOMEM;
>
>         /*
> @@ -577,14 +594,14 @@ static int component_bind(struct component *component, struct master *master,
>          * at the appropriate moment.
>          */
>         if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>                 return -ENOMEM;
>         }
>
> -       dev_dbg(master->parent, "binding %s (ops %ps)\n",
> +       dev_dbg(adev->parent, "binding %s (ops %ps)\n",
>                 dev_name(component->dev), component->ops);
>
> -       ret = component->ops->bind(component->dev, master->parent, data);
> +       ret = component->ops->bind(component->dev, adev->parent, data);
>         if (!ret) {
>                 component->bound = true;
>
> @@ -595,16 +612,16 @@ static int component_bind(struct component *component, struct master *master,
>                  * can clean those resources up independently.
>                  */
>                 devres_close_group(component->dev, NULL);
> -               devres_remove_group(master->parent, NULL);
> +               devres_remove_group(adev->parent, NULL);
>
> -               dev_info(master->parent, "bound %s (ops %ps)\n",
> +               dev_info(adev->parent, "bound %s (ops %ps)\n",
>                          dev_name(component->dev), component->ops);
>         } else {
>                 devres_release_group(component->dev, NULL);
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>
>                 if (ret != -EPROBE_DEFER)
> -                       dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
> +                       dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
>                                 dev_name(component->dev), component->ops, ret);
>         }
>
> @@ -622,31 +639,31 @@ static int component_bind(struct component *component, struct master *master,
>   */
>  int component_bind_all(struct device *parent, void *data)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>         struct component *c;
>         size_t i;
>         int ret = 0;
>
>         WARN_ON(!mutex_is_locked(&component_mutex));
>
> -       master = __master_find(parent, NULL);
> -       if (!master)
> +       adev = __aggregate_find(parent, NULL);
> +       if (!adev)
>                 return -EINVAL;
>
>         /* Bind components in match order */
> -       for (i = 0; i < master->match->num; i++)
> -               if (!master->match->compare[i].duplicate) {
> -                       c = master->match->compare[i].component;
> -                       ret = component_bind(c, master, data);
> +       for (i = 0; i < adev->match->num; i++)
> +               if (!adev->match->compare[i].duplicate) {
> +                       c = adev->match->compare[i].component;
> +                       ret = component_bind(c, adev, data);
>                         if (ret)
>                                 break;
>                 }
>
>         if (ret != 0) {
>                 for (; i > 0; i--)
> -                       if (!master->match->compare[i - 1].duplicate) {
> -                               c = master->match->compare[i - 1].component;
> -                               component_unbind(c, master, data);
> +                       if (!adev->match->compare[i - 1].duplicate) {
> +                               c = adev->match->compare[i - 1].component;
> +                               component_unbind(c, adev, data);
>                         }
>         }
>
> @@ -675,8 +692,8 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
>
>         ret = try_to_bring_up_masters(component);
>         if (ret < 0) {
> -               if (component->master)
> -                       remove_component(component->master, component);
> +               if (component->adev)
> +                       remove_component(component->adev, component);
>                 list_del(&component->node);
>
>                 kfree(component);
> @@ -757,9 +774,9 @@ void component_del(struct device *dev, const struct component_ops *ops)
>                         break;
>                 }
>
> -       if (component && component->master) {
> -               take_down_master(component->master);
> -               remove_component(component->master, component);
> +       if (component && component->adev) {
> +               take_down_aggregate_device(component->adev);
> +               remove_component(component->adev, component);
>         }
>
>         mutex_unlock(&component_mutex);
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 16de18f473d7..71bfc3862633 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -41,7 +41,7 @@ void component_del(struct device *, const struct component_ops *);
>  int component_bind_all(struct device *master, void *master_data);
>  void component_unbind_all(struct device *master, void *master_data);
>
> -struct master;
> +struct aggregate_device;
>
>  /**
>   * struct component_master_ops - callback for the aggregate driver
> --
> https://chromeos.dev
>



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux