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

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

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.

> 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

Good to know... Nope, I don't care much about them being relaxed as I will still
call device_link_add() when the consumer probes and finds the supplier. The only
downside from relaxing is "loosing" AUTOPROBE_CONSUMER but that is not a big
deal.

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

Thanks! I think it's a valid bug with devlinks and overlays but it's sensible
stuff (so the RFC) so it would be nice to have some review and recommendations
how to solve it... I would definitely like to have it fixed as I see more and
more people (ab)using overlays.

- Nuno Sá