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@xxxxxxxxx> 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@xxxxxxxxxx> wrote: > > > > > > > > From: Nuno Sa <nuno.sa@xxxxxxxxxx> > > > > > > > > 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á
