On Sat, Jan 30, 2021 at 5:03 AM Saravana Kannan <sarava...@google.com> 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 <sarava...@google.com>
Looks reasonable to me:
Acked-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> ---
> 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
>
