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

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

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á