On Thu, Jan 25, 2024 at 7:31 AM Nuno Sá <noname.nuno@xxxxxxxxx> 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@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. > > > > 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. > 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... Basically drop this patch. I don't see a point in this. -Saravana
