On Sat, 2024-01-27 at 15:15 +0000, Jonathan Cameron wrote: > On Fri, 26 Jan 2024 15:26:08 +0100 > Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > On Fri, 2024-01-26 at 09:04 +0100, Nuno Sá wrote: > > > On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote: > > > > 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. > > > > > > > > > > Oh yes, I missed that extra if() against the DL_FLAG_AUTOREMOVE_CONSUMER > > > flag... > > > > > > > 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! > > > > > > Thanks Rafael, your last two paragraphs make it really clear what's the > > > reasoning and why this patch is wrong. > > > > > > Jonathan, if nothing else comes that I need a re-spin, can you drop this > > > patch > > > when applying? > > > > > > I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the > > > device_link_add() > > > call as it will be ignored if fw_devlinks already created the link but > > > might > > > be > > > important if the kernel command line fw_devlink is set to 'off'. > > > > > > Or maybe, as Saravan mentioned in his reply we can just pass > > > DL_FLAG_MANAGED > > > as > > > > Forget about this as I just realized DL_FLAG_MANAGED is not a proper flag we > > can > > pass... > > > > - Nuno Sá > > > > Discussion has gotten too complex - so even if no changes, send a v8 dropping > the patch (assuming that's the end conclusion!) > Dropping the patch is pretty much decided is the right thing to do. The only thing I'm still thinking is that if I should use AUTOPROBE_CONSUMER (as fw_devlinks) instead when creating the link. With that flag, any IIO consumer of the IIO backend will be automatically probed as soon as the backend is probed. It also has the advantage of keeping the link around (vs AUREMOVE_CONSUMER which deletes the link when the IIO consumer is gone) so in the re-bind case we can avoid useless EPROBE_DEFERs. It's a nitpicky thing in the end and not really that important. Moreover because I expect that in 99% of the usecases, fw_devlinks will already create our link so the flags we pass in our call don't really matter. Note that our explicit call is still important (as I explained to Saravan in another email) as we based the design with the assumption that the consumer can never be around without the backend. And in the case we have modules, we can have the links created by fw_devlinks removed unless we explicitly call device_link_add() (and that would mean our previous assumptions are no longer valid). - Nuno Sá
