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

["Nuno Sá" <noname.nuno@gmail.com>]

Hi Saravana,

Thanks for your feedback,

On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> 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.
> 

Well, this is still useful in the modules case as the link will be relaxed after
all devices are initialized and that will already clear AUTOPROBE_CONSUMER
AFAIU. But, more importantly, if I'm not missing anything, in [1], fw_devlinks
will be dropped after the consumer + supplier are bound which means I definitely
want to create a link between my consumer and supplier. 

FWIW, I was misunderstanding things since I thought DL_FLAG_AUTOREMOVE_CONSUMER
was needed to make sure the consumer is unbound before the supplier. But for
that I think I can even pass 0 in the flags as I only need the link to be
MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.

Also note that there are more places in the kernel with
DL_FLAG_AUTOREMOVE_CONSUMER and that flag is likely being ignored in case the
link already exists.

I'm also clearing DL_FLAG_AUTOPROBE_CONSUMER as from the first check in
device_link_add(() check I realize that we can't/shouldn't have it together with
one of AUTOREMOVE_CONSUMER | AUTOREMOVE_SUPPLIER, right? At this point,
AUTOPROBE_CONSUMER is also likely not that useful anymore as both supplier and
consumer are up and I guess that's the typical case for subsystems/drivers to
call device_link_add().

And since I have your attention, it would be nice if you could look in another
sensible patch [2] that I've resended 3 times already. You're not in CC but I
see you've done quite some work in dev_links so... Completely unrelated I know
:)

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L1292
[2]: https://lore.kernel.org/all/20231213-fix-device-links-overlays-v1-1-f091b213777c@analog.com/#t

- Nuno Sá
>