Re: [PATCH v2 1/3] driver core: fw_devlink: Detect supplier devices that will never be added

[Rafael J. Wysocki]

On Tue, Feb 2, 2021 at 5:33 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> During the initial parsing of firmware by fw_devlink, fw_devlink might
> infer that some supplier firmware nodes would get populated as devices.
> But the inference is not always correct. This patch tries to logically
> detect and fix such mistakes as boot progresses or more devices probe.
>
> fw_devlink makes a fundamental assumption that once a device binds to a
> driver, it will populate (i.e: add as struct devices) all the child
> firmware nodes that could be populated as devices (if they aren't
> populated already).
>
> So, whenever a device probes, we check all its child firmware nodes. If
> a child firmware node has a corresponding device populated, we don't
> modify the child node or its descendants. However, if a child firmware
> node has not been populated as a device, we delete all the fwnode links
> where the child node or its descendants are suppliers. This ensures that
> no other device is blocked on a firmware node that will never be
> populated as a device. We also mark such fwnodes as NOT_DEVICE, so that
> no new fwnode links are created with these nodes as suppliers.
>
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>

Still ACKed.

> ---
>  drivers/base/core.c    | 31 ++++++++++++++++++++++++++++---
>  include/linux/fwnode.h |  2 ++
>  2 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 484a942884ba..c95b1daabac7 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -148,6 +148,21 @@ void fwnode_links_purge(struct fwnode_handle *fwnode)
>         fwnode_links_purge_consumers(fwnode);
>  }
>
> +static void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode)
> +{
> +       struct fwnode_handle *child;
> +
> +       /* Don't purge consumer links of an added child */
> +       if (fwnode->dev)
> +               return;
> +
> +       fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
> +       fwnode_links_purge_consumers(fwnode);
> +
> +       fwnode_for_each_available_child_node(fwnode, child)
> +               fw_devlink_purge_absent_suppliers(child);
> +}
> +
>  #ifdef CONFIG_SRCU
>  static DEFINE_MUTEX(device_links_lock);
>  DEFINE_STATIC_SRCU(device_links_srcu);
> @@ -1154,12 +1169,22 @@ void device_links_driver_bound(struct device *dev)
>         LIST_HEAD(sync_list);
>
>         /*
> -        * If a device probes successfully, it's expected to have created all
> +        * If a device binds successfully, it's expected to have created all
>          * the device links it needs to or make new device links as it needs
> -        * them. So, it no longer needs to wait on any suppliers.
> +        * them. So, fw_devlink no longer needs to create device links to any
> +        * of the device's suppliers.
> +        *
> +        * Also, if a child firmware node of this bound device is not added as
> +        * a device by now, assume it is never going to be added and make sure
> +        * other devices don't defer probe indefinitely by waiting for such a
> +        * child device.
>          */
> -       if (dev->fwnode && dev->fwnode->dev == dev)
> +       if (dev->fwnode && dev->fwnode->dev == dev) {
> +               struct fwnode_handle *child;
>                 fwnode_links_purge_suppliers(dev->fwnode);
> +               fwnode_for_each_available_child_node(dev->fwnode, child)
> +                       fw_devlink_purge_absent_suppliers(child);
> +       }
>         device_remove_file(dev, &dev_attr_waiting_for_supplier);
>
>         device_links_write_lock();
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index fde4ad97564c..21082f11473f 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -19,8 +19,10 @@ struct device;
>   * fwnode link flags
>   *
>   * LINKS_ADDED: The fwnode has already be parsed to add fwnode links.
> + * NOT_DEVICE: The fwnode will never be populated as a struct device.
>   */
>  #define FWNODE_FLAG_LINKS_ADDED                BIT(0)
> +#define FWNODE_FLAG_NOT_DEVICE         BIT(1)
>
>  struct fwnode_handle {
>         struct fwnode_handle *secondary;
> --
> 2.30.0.365.g02bc693789-goog
>