Re: [PATCH v1 2/5] driver core: Add fw_devlink kernel commandline option

[Saravana Kannan <saravanak@google.com>]

On Fri, Feb 21, 2020 at 5:40 PM Saravana Kannan <saravanak@google.com> wrote:
>
> fwnode_operations.add_links allows creating device links from
> information provided by firmware.
>
> fwnode_operations.add_links is currently implemented only by
> OF/devicetree code and a specific case of efi. However, there's nothing
> preventing ACPI or other firmware types from implementing it.
>
> The OF implementation is currently controlled by a kernel commandline
> parameter called of_devlink.
>
> Since this feature is generic isn't limited to OF, add a generic
> fw_devlink kernel commandline parameter to control this feature across
> firmware types.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  .../admin-guide/kernel-parameters.txt         | 18 +++++++++++++
>  drivers/base/core.c                           | 27 ++++++++++++++++++-
>  include/linux/fwnode.h                        |  2 ++
>  3 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index dbc22d684627..29985152b66d 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1350,6 +1350,24 @@
>                         can be changed at run time by the max_graph_depth file
>                         in the tracefs tracing directory. default: 0 (no limit)
>
> +       fw_devlink=     [KNL] Create device links between consumer and supplier
> +                       devices by scanning the firmware to infer the
> +                       consumer/supplier relationships. This feature is
> +                       especially useful when drivers are loaded as modules as
> +                       it ensures proper ordering of tasks like device probing
> +                       (suppliers first, then consumers), supplier boot state
> +                       clean up (only after all consumers have probed),
> +                       suspend/resume & runtime PM (consumers first, then
> +                       suppliers).
> +                       Format: { off | permissive | on | rpm }
> +                       off --  Don't create device links from firmware info.
> +                       permissive -- Create device links from firmware info
> +                               but use it only for ordering boot state clean
> +                               up (sync_state() calls).
> +                       on --   Create device links from firmware info and use it
> +                               to enforce probe and suspend/resume ordering.
> +                       rpm --  Like "on", but also use to order runtime PM.
> +

A bit of bikeshedding myself: I could rename "on" to "enforce" and
"rpm" to "enforce-rpm".

Let me know if any of you have a strong preference on these two options.

-Saravana

>         gamecon.map[2|3]=
>                         [HW,JOY] Multisystem joystick and NES/SNES/PSX pad
>                         support via parallel port (up to 5 devices per port)
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d32a3aefff32..aeaca8a3aad9 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2345,6 +2345,31 @@ static int device_private_init(struct device *dev)
>         return 0;
>  }
>
> +u32 fw_devlink_flags;
> +static int __init fw_devlink_setup(char *arg)
> +{
> +       if (!arg)
> +               return -EINVAL;
> +
> +       if (strcmp(arg, "off") == 0) {
> +               fw_devlink_flags = 0;
> +       } else if (strcmp(arg, "permissive") == 0) {
> +               fw_devlink_flags = DL_FLAG_SYNC_STATE_ONLY;
> +       } else if (strcmp(arg, "on") == 0) {
> +               fw_devlink_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> +       } else if (strcmp(arg, "rpm") == 0) {
> +               fw_devlink_flags = DL_FLAG_AUTOPROBE_CONSUMER |
> +                                  DL_FLAG_PM_RUNTIME;
> +       }
> +       return 0;
> +}
> +early_param("fw_devlink", fw_devlink_setup);
> +
> +u32 fw_devlink_get_flags(void)
> +{
> +       return fw_devlink_flags;
> +}
> +
>  /**
>   * device_add - add device to device hierarchy.
>   * @dev: device.
> @@ -2493,7 +2518,7 @@ int device_add(struct device *dev)
>          */
>         device_link_add_missing_supplier_links();
>
> -       if (fwnode_has_op(dev->fwnode, add_links)) {
> +       if (fw_devlink_flags && fwnode_has_op(dev->fwnode, add_links)) {
>                 fw_ret = fwnode_call_int_op(dev->fwnode, add_links, dev);
>                 if (fw_ret == -ENODEV)
>                         device_link_wait_for_mandatory_supplier(dev);
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 8feeb94b8acc..e0abafbb17f8 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -170,4 +170,6 @@ struct fwnode_operations {
>         } while (false)
>  #define get_dev_from_fwnode(fwnode)    get_device((fwnode)->dev)
>
> +extern u32 fw_devlink_get_flags(void);
> +
>  #endif
> --
> 2.25.0.265.gbab2e86ba0-goog
>