Re: [PATCH v3 2/3] driver core: fw_devlink: Add support for FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD

["Rafael J. Wysocki" <rafael@kernel.org>]

On Wed, Sep 15, 2021 at 7:09 PM Saravana Kannan <saravanak@google.com> wrote:
>
> If a parent device is also a supplier to a child device, fw_devlink=on by
> design delays the probe() of the child device until the probe() of the
> parent finishes successfully.
>
> However, some drivers of such parent devices (where parent is also a
> supplier) expect the child device to finish probing successfully as soon as
> they are added using device_add() and before the probe() of the parent
> device has completed successfully. One example of such a case is discussed
> in the link mentioned below.
>
> Add a flag to make fw_devlink=on not enforce these supplier-consumer
> relationships, so these drivers can continue working.
>
> Link: https://lore.kernel.org/netdev/CAGETcx_uj0V4DChME-gy5HGKTYnxLBX=TH2rag29f_p=UcG+Tg@mail.gmail.com/
> Fixes: ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/base/core.c    | 19 +++++++++++++++++++
>  include/linux/fwnode.h | 11 ++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 316df6027093..21d4cb5d3767 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1722,6 +1722,25 @@ static int fw_devlink_create_devlink(struct device *con,
>         struct device *sup_dev;
>         int ret = 0;
>
> +       /*
> +        * In some cases, a device P might also be a supplier to its child node
> +        * C. However, this would defer the probe of C until the probe of P
> +        * completes successfully. This is perfectly fine in the device driver
> +        * model. device_add() doesn't guarantee probe completion of the device
> +        * by the time it returns.

That's right.

However, I don't quite see a point in device links where the supplier
is a direct ancestor of the consumer.  From the PM perspective they
are simply redundant and I'm not sure about any other use cases where
they aren't.

So what's the reason to add them in the first place?

> +        *
> +        * However, there are a few drivers that assume C will finish probing
> +        * as soon as it's added and before P finishes probing. So, we provide
> +        * a flag to let fw_devlink know not to delay the probe of C until the
> +        * probe of P completes successfully.

Well, who's expected to set that flag and when?  This needs to be
mentioned here IMO.

> +        *
> +        * When such a flag is set, we can't create device links where P is the
> +        * supplier of C as that would delay the probe of C.
> +        */
> +       if (sup_handle->flags & FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD &&
> +           fwnode_is_ancestor_of(sup_handle, con->fwnode))
> +               return -EINVAL;
> +
>         sup_dev = get_dev_from_fwnode(sup_handle);
>         if (sup_dev) {
>                 /*
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 59828516ebaf..9f4ad719bfe3 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -22,10 +22,15 @@ struct device;
>   * LINKS_ADDED:        The fwnode has already be parsed to add fwnode links.
>   * NOT_DEVICE: The fwnode will never be populated as a struct device.
>   * INITIALIZED: The hardware corresponding to fwnode has been initialized.
> + * NEEDS_CHILD_BOUND_ON_ADD: For this fwnode/device to probe successfully, its
> + *                          driver needs its child devices to be bound with
> + *                          their respective drivers as soon as they are
> + *                          added.

The fact that this requires so much comment text here is a clear
band-aid indication to me.

>   */
> -#define FWNODE_FLAG_LINKS_ADDED                BIT(0)
> -#define FWNODE_FLAG_NOT_DEVICE         BIT(1)
> -#define FWNODE_FLAG_INITIALIZED                BIT(2)
> +#define FWNODE_FLAG_LINKS_ADDED                        BIT(0)
> +#define FWNODE_FLAG_NOT_DEVICE                 BIT(1)
> +#define FWNODE_FLAG_INITIALIZED                        BIT(2)
> +#define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD   BIT(3)
>
>  struct fwnode_handle {
>         struct fwnode_handle *secondary;
> --
> 2.33.0.309.g3052b89438-goog
>