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

[Rafael J. Wysocki]

On Sat, Jan 30, 2021 at 5:03 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.

That is unless they have been populated earlier.

> 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 go an 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.
>
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>

Looks reasonable to me:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>

> ---
>  drivers/base/core.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 484a942884ba..f380133df63b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -148,6 +148,20 @@ 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_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 +1168,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();
> --
> 2.30.0.365.g02bc693789-goog
>