On Thu, Jan 25, 2024 at 4:31 PM 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. > > 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... No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the former replaces the latter. Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if AUTOPROBE_CONSUMER is set in there. > 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... If the original creator of the link didn't indicate either DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are expected to need the link to stay around until it is explicitly deleted. Therefore adding any of these flags for an existing link where they both are unset would be a mistake, because it would effectively cause the link to live shorter than expected by the original creator and that might lead to correctness issues. Thanks!
