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

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.

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.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/base/core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

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á
>

On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> 
> 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. 
> 

Ok, so to add a bit more on this, there are two cases:

1) Both sup and con are modules and after boot up, the link is relaxed and thus
turned into a sync_state_only link. That means the link will be deleted anyways
and AUTOPROBE_CONSUMER is already cleared by the time we try to change the link.

2) The built-in case where the link is kept as created by fw_devlink and this
patch effectively clears AUTOPROBE_CONSUMER.

Given the above, not sure what's the best option. I can think of 4:

1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER is
pretty much ignored in my call but it will turn the link in a MANAGED one and
clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;

2) Rework this patch so we can still change an existing link to accept
DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).

However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so if
flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER and
AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I think
one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...

3) Keep it as-is... This one is likely a NACK as I'm getting the feeling that
clearing stuff that might have been created by fw_devlinks is probably a no-go.

Let me know your thoughts...

- Nuno Sá

On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
>
> 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.

As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
not correct. There's almost never a good reason to drop a device link.
Even when a device/driver are unbound, we still want future probe
attempts to make use of the dependency info and block a device from
probing if the supplier hasn't probed.

If you don't want the links created by fw_devlink to be relaxed, I
think you should instead set the kernel command line param so that the
kernel doesn't time out and give up on enforcing dependencies.
deferred_probe_timeout=-1

Then you don't have to worry about creating device links.

> 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
> :)

Regarding [2], I'll try.

-Saravana

>
> [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á
> >
>

On Thu, 2024-01-25 at 16:57 -0800, Saravana Kannan wrote:
> On Thu, Jan 25, 2024 at 7:31 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> > > 
> > > 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.
> > > 
> > 
> > Ok, so to add a bit more on this, there are two cases:
> > 
> > 1) Both sup and con are modules and after boot up, the link is relaxed and
> > thus
> > turned into a sync_state_only link. That means the link will be deleted
> > anyways
> > and AUTOPROBE_CONSUMER is already cleared by the time we try to change the
> > link.
> > 
> > 2) The built-in case where the link is kept as created by fw_devlink and
> > this
> > patch effectively clears AUTOPROBE_CONSUMER.
> 
> I still don't see a good reason for you to set those flags. And if you
> see my other reply, I'm not sure you even need to make changes. Just
> use the existing command line arg.
> 
> > Given the above, not sure what's the best option. I can think of 4:
> > 
> > 1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER
> > is
> > pretty much ignored in my call but it will turn the link in a MANAGED one
> > and
> > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > 
> > 2) Rework this patch so we can still change an existing link to accept
> > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > 
> > However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so
> > if
> > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER
> > and
> > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I
> > think
> > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...
> 
> This is all way too complicated and I still see no good reason to use
> those flags in whatever case you have in mind.
> 
> And Rafael explained why your changes don't make sense. Once a link is
> created, any AUTOREMOVE flags should be set.

Yeah, Rafael reply made it clear...

- Nuno Sá

On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote:
> > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > >
> > > >
> > > > 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.
> > >
> > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> > > not correct. There's almost never a good reason to drop a device link.
> > > Even when a device/driver are unbound, we still want future probe
> > > attempts to make use of the dependency info and block a device from
> > > probing if the supplier hasn't probed.
> > >
> >
> > Yeah that makes sense and is making me thinking that I should change my call
> > (in
> > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
> > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables
> > fw_devlinks
> > then it matters.

I don't fully understand the patch series, but what exactly are you
gaining by adding device links explicitly. I'd rather that every
driver didn't explicitly create a device link. That's just a lot of
useless code in most cases and we shouldn't have useless code lying
around. If someone does fw_devlink=off and you don't create a device
link explicitly, what's the worst that's going to happen? Useless
deferred probes? fw_devlink is really only there as a debug tool at
this point -- I don't think you need to worry about people setting it
to off/permissive.

> >
>
> Yeah, just realized MANAGED is not a valid flag one can pass to
> device_link_add() :)

If you don't pass the STATELESS flag, a link is assumed to be MANAGED.
So, you can still create managed device links.

-Saravana

On Fri, 2024-01-26 at 10:09 -0800, Saravana Kannan wrote:
> On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote:
> > > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> > > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > 
> > > > > 
> > > > > 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.
> > > > 
> > > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> > > > not correct. There's almost never a good reason to drop a device link.
> > > > Even when a device/driver are unbound, we still want future probe
> > > > attempts to make use of the dependency info and block a device from
> > > > probing if the supplier hasn't probed.
> > > > 
> > > 
> > > Yeah that makes sense and is making me thinking that I should change my call
> > > (in
> > > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
> > > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables
> > > fw_devlinks
> > > then it matters.
> 
> I don't fully understand the patch series, but what exactly are you
> gaining by adding device links explicitly. I'd rather that every
> driver didn't explicitly create a device link. That's just a lot of
> useless code in most cases and we shouldn't have useless code lying
> around. If someone does fw_devlink=off and you don't create a device
> link explicitly, what's the worst that's going to happen? Useless
> deferred probes? fw_devlink is really only there as a debug tool at
> this point -- I don't think you need to worry about people setting it
> to off/permissive.
> 

There's (at least) a reasoning behind the explicit use of the links. We want to
ensure the creation of a MANAGED link so that we can be assured (i think) that the
consumer device will never be around if the supplier unbinds causing those typical
issues of a supplier going away and consumers trying to access it's code. Now, in the
HW arrangement we're trying to support there's no such thing as optional suppliers.
If the IIO backend is going away, there's no good reason for an IIO frontend to stay
around. And using the guarantee provided by the links made the code way simpler.

Note that in the first versions of the series I was also adding code (together with
dev_links) to make sure we would return error in case the consumer tried to access a
supplier callback and the supplier is no longer around. That meant a mutex for
syncing callbacks plus checking a pointer and having a kref for the backend object.

Jonathan (rightfully) complained that I was adding two ways of guaranteeing the same
thing. Thus, as we don't really have optional suppliers, we went with the explicit
links creation as it makes the code much simpler. If you have any interest, look at
[1] (and let me know if there's any wrong assumption). 

> > > 
> > 
> > Yeah, just realized MANAGED is not a valid flag one can pass to
> > device_link_add() :)
> 
> If you don't pass the STATELESS flag, a link is assumed to be MANAGED.
> So, you can still create managed device links.
> 

Yes, I realized that... Maybe even passing no flag would do the trick.

[1]: https://lore.kernel.org/linux-iio/8085910199d4b653edb61c51fc80a503ee50131d.camel@gmail.com/

- Nuno Sá