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
>
