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. 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@xxxxxxxxxx/#t - Nuno Sá >
