On Mon, 2024-01-29 at 14:31 -0800, Saravana Kannan wrote: > On Mon, Jan 29, 2024 at 12:26 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > > 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). > > I saw your reasoning, but technically there are still gaps in the > forced unbinding of consumers. If the consumer doesn't have a bus or > doesn't have an explicit driver, it won't be force unbound. But this It will never be the case for us. An IIO frontend (the consumer in here) will always be on a bus (typically spi or i2c) and have a driver. In fact, the IIO ABI should be registered in this device. > is all generic issues that need to be resolved at a driver core level. > I'd really prefer drivers/frameworks not duplicating it all over. > > How about just checking for fw_devlink=on or better and not probe your > supplier if it's not set? Or not allow unbinding your supplier if > fw_devlink=on or better isn't there? > The problem with that is that we still want our IIO converter to work even if fw_devlink is off (but if having the links is ever an issue - which shouldn't be - then I should not be using the links already). but most importantly, we would also need to put similar constrains and check the deferred timeout parameter otherwise we could not rely on the links in the modules case. I see your concern about drivers/frameworks doing unnecessary calls but, at least, in here we do have a reason to rely on it and the simplification code it gives us, really pays off. You mention we also need some fixes in the core so maybe when we are in a better state I can drop the explicit call. Also thinking in your suggestion, what I could do is not allow the IIO backend to be registered in case fw_links are off or permissive (and hence the supplier should never probe). But then, we would also need to care about the module case and I'm not seeing this checks being better than the explicit call, honestly. To sum it up, I would be fine with the constrain for the built-in case but we definitely want things to work when compiled as modules. And the checks in there would be odd (or telling users that they need to add that command line parameter) - Nuno Sá
