Re: [TEST PATCH v1] driver: core: Make fw_devlink=on more forgiving

[Saravana Kannan]

On Thu, Jan 21, 2021 at 12:22 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> This patch is for test purposes only and pretty experimental. Code might
> not be optimized, clean, formatted properly, etc.
>
> Please review it only for functional bugs like locking bugs, wrong
> logic, etc.
>
> It's basically trying to figure out which devices will never probe and
> ignore them. Might not always work.
>
> Marek, Geert, Marc,
>
> Can you please try this patch INSTEAD of the other workarounds we found?

Oh and can you please also try with the CONFIG_MODULES enabled vs
disabled? Or have it disabled but fix the patch so the condition
always evaluates to true.

Thanks,
Saravana

>
> Jon, Michael,
>
> I'm explicitly not including you in the "To" because this patch won't
> work for your issues.
>
> Cc: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> ---
>  drivers/base/base.h |   3 ++
>  drivers/base/core.c | 117 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/base/dd.c   |  24 +++++++++
>  3 files changed, 142 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index f5600a83124f..8d5fd95fa147 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -106,6 +106,9 @@ struct device_private {
>  #define to_device_private_class(obj)   \
>         container_of(obj, struct device_private, knode_class)
>
> +bool fw_devlink_is_permissive(void);
> +bool fw_devlink_unblock_probe(struct device *dev);
> +
>  /* initialisation functions */
>  extern int devices_init(void);
>  extern int buses_init(void);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index e61e62b624ce..8528704bbb40 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -49,7 +49,6 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
>  static LIST_HEAD(deferred_sync);
>  static unsigned int defer_sync_state_count = 1;
>  static DEFINE_MUTEX(fwnode_link_lock);
> -static bool fw_devlink_is_permissive(void);
>
>  /**
>   * fwnode_link_add - Create a link between two fwnode_handles.
> @@ -1481,7 +1480,7 @@ u32 fw_devlink_get_flags(void)
>         return fw_devlink_flags;
>  }
>
> -static bool fw_devlink_is_permissive(void)
> +bool fw_devlink_is_permissive(void)
>  {
>         return fw_devlink_flags == FW_DEVLINK_FLAGS_PERMISSIVE;
>  }
> @@ -1552,6 +1551,120 @@ static int fw_devlink_relax_cycle(struct device *con, void *sup)
>         return ret;
>  }
>
> +static int __device_links_suppliers_available(struct device *dev)
> +{
> +       struct device_link *link;
> +       int ret = 0;
> +
> +       if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
> +           !fw_devlink_is_permissive()) {
> +               return -EPROBE_DEFER;
> +       }
> +
> +       list_for_each_entry(link, &dev->links.suppliers, c_node) {
> +               if (!(link->flags & DL_FLAG_MANAGED))
> +                       continue;
> +
> +               if (link->status != DL_STATE_AVAILABLE &&
> +                   !(link->flags & DL_FLAG_SYNC_STATE_ONLY)) {
> +                       ret = -EPROBE_DEFER;
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +bool fw_devlink_unblock_probe(struct device *dev)
> +{
> +       struct fwnode_link *link, *tmp;
> +       struct device_link *dev_link, *dev_ln;
> +       struct fwnode_handle *fwnode = dev->fwnode;
> +       bool unblocked = false;
> +
> +       if (!fw_devlink_get_flags() || fw_devlink_is_permissive())
> +               return false;
> +
> +       if (!fwnode)
> +               return false;
> +
> +       mutex_lock(&fwnode_link_lock);
> +
> +       /* Delete questionable fwnode links */
> +       list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
> +               struct device *par_dev;
> +               struct fwnode_handle *par;
> +               bool bound;
> +
> +               /*
> +                * Walk up fwnode tree of supplier till we find a parent device
> +                * that has been added or a parent fwnode that has fwnode links
> +                * (this is a firmware node that is expected to be added as a
> +                * device in the future).
> +                */
> +               par = fwnode_get_parent(link->supplier);
> +               while (par && list_empty(&par->suppliers) && !par->dev)
> +                       par = fwnode_get_next_parent(par);
> +
> +               /* Supplier is waiting on parent device to be added. */
> +               if (par && !par->dev) {
> +                       fwnode_handle_put(par);
> +                       continue;
> +               }
> +
> +               if (par && par->dev) {
> +                       par_dev = get_dev_from_fwnode(fwnode);
> +                       device_lock(par_dev);
> +                       bound = device_is_bound(par_dev);
> +                       device_unlock(par_dev);
> +                       put_device(par_dev);
> +
> +                       /* Supplier is waiting on parent device to be bound. */
> +                       if (!bound)
> +                               continue;
> +               }
> +
> +               /*
> +                * Supplier has no parent or the immediate parent device has
> +                * been bound to a device. It should have been added by now.
> +                * So, this link is spurious. Delete it.
> +                */
> +               dev_info(dev, "Deleting fwnode link to %pfwP\n",
> +                        link->supplier);
> +               list_del(&link->s_hook);
> +               list_del(&link->c_hook);
> +               kfree(link);
> +               unblocked = true;
> +       }
> +
> +       if (IS_ENABLED(CONFIG_MODULES))
> +               goto out;
> +
> +       device_links_write_lock();
> +
> +       list_for_each_entry_safe(dev_link, dev_ln, &dev->links.suppliers,
> +                                c_node) {
> +               if (!(dev_link->flags & DL_FLAG_INFERRED) ||
> +                   dev_link->flags & DL_FLAG_SYNC_STATE_ONLY ||
> +                   dev_link->status != DL_STATE_DORMANT)
> +                       continue;
> +
> +               /* This supplier should have probed by now. */
> +               if (!__device_links_suppliers_available(dev_link->supplier)) {
> +                       dev_info(dev, "Deleting dev link to %s\n",
> +                                dev_name(dev_link->supplier));
> +                       device_link_drop_managed(dev_link);
> +                       unblocked = true;
> +               }
> +       }
> +
> +       device_links_write_unlock();
> +
> +out:
> +       mutex_unlock(&fwnode_link_lock);
> +       return unblocked;
> +}
> +
>  /**
>   * fw_devlink_create_devlink - Create a device link from a consumer to fwnode
>   * @con - Consumer device for the device link
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 2f32f38a11ed..d4ccd2a2b6a4 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -301,6 +301,25 @@ static void deferred_probe_timeout_work_func(struct work_struct *work)
>  }
>  static DECLARE_DELAYED_WORK(deferred_probe_timeout_work, deferred_probe_timeout_work_func);
>
> +static bool deferred_probe_fw_devlink_unblock(void)
> +{
> +       struct device *dev;
> +       struct device_private *private;
> +       bool unblocked = false;
> +
> +       if (!fw_devlink_get_flags() || fw_devlink_is_permissive())
> +               return false;
> +
> +       mutex_lock(&deferred_probe_mutex);
> +       list_for_each_entry(private, &deferred_probe_pending_list, deferred_probe) {
> +               dev = private->device;
> +               unblocked |= fw_devlink_unblock_probe(dev);
> +       }
> +       mutex_unlock(&deferred_probe_mutex);
> +
> +       return unblocked;
> +}
> +
>  /**
>   * deferred_probe_initcall() - Enable probing of deferred devices
>   *
> @@ -317,6 +336,11 @@ static int deferred_probe_initcall(void)
>         driver_deferred_probe_trigger();
>         /* Sort as many dependencies as possible before exiting initcalls */
>         flush_work(&deferred_probe_work);
> +
> +       while (deferred_probe_fw_devlink_unblock()) {
> +               driver_deferred_probe_trigger();
> +               flush_work(&deferred_probe_work);
> +       }
>         initcalls_done = true;
>
>         /*
> --
> 2.30.0.296.g2bfb1c46d8-goog
>