Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Saravana Kannan <saravanak@google.com>]

On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> If a device_link is previously created (eg: via
> fw_devlink_create_devlink()) before the supplier + consumer are both
> present and bound to their respective drivers, there's no way to set
> DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> DL_FLAG_AUTOREMOVE_SUPPLIER is done.

Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
Especially if fw_devlink already created the link? You are effectively
trying to delete the link fw_devlink created if any of your devices
unbind.

> While at it, make sure that we are never left with
> DL_FLAG_AUTOPROBE_CONSUMER set together with one of
> DL_FLAG_AUTOREMOVE_CONSUMER or DL_FLAG_AUTOREMOVE_SUPPLIER.

fw_devlink sets AUTOPROBE_CONSUMER. So, are you trying to clear it? Why?

I almost want to NACK this, but I'll hear more before I do.

-Saravana

>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/base/core.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..ee8a46df28e1 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -807,11 +807,15 @@ struct device_link *device_link_add(struct device *consumer,
>                  * update the existing link to stay around longer.
>                  */
>                 if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) {
> -                       if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
> -                               link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
> -                               link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
> -                       }
> -               } else if (!(flags & DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +                       link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
> +                       link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
> +                       link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
> +
> +               } else if (flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
> +                       link->flags &= ~DL_FLAG_AUTOREMOVE_SUPPLIER;
> +                       link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
> +                       link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER;
> +               } else {
>                         link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
>                                          DL_FLAG_AUTOREMOVE_SUPPLIER);
>                 }
>
> --
> 2.43.0
>