| Message ID | 20240123-iio-backend-v7-4-1bff236b8693@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add new backend framework | expand |
On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > From: Nuno Sa <nuno.sa@analog.com> > > 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. > While at it, make sure that we are never left with > DL_FLAG_AUTOPROBE_CONSUMER set together with one of > DL_FLAG_AUTOREMOVE_CONSUMER or DL_FLAG_AUTOREMOVE_SUPPLIER. fw_devlink sets AUTOPROBE_CONSUMER. So, are you trying to clear it? Why? I almost want to NACK this, but I'll hear more before I do. -Saravana > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/base/core.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 14d46af40f9a..ee8a46df28e1 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -807,11 +807,15 @@ struct device_link *device_link_add(struct device *consumer, > * update the existing link to stay around longer. > */ > if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) { > - if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) { > - link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER; > - link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER; > - } > - } else if (!(flags & DL_FLAG_AUTOREMOVE_CONSUMER)) { > + link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER; > + link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER; > + link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER; > + > + } else if (flags & DL_FLAG_AUTOREMOVE_CONSUMER) { > + link->flags &= ~DL_FLAG_AUTOREMOVE_SUPPLIER; > + link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER; > + link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER; > + } else { > link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER | > DL_FLAG_AUTOREMOVE_SUPPLIER); > } > > -- > 2.43.0 >
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@kernel.org> wrote: > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > 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@analog.com/#t - Nuno Sá >
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@kernel.org> wrote: > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > 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... 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... - Nuno Sá
On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > 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!
On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > 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. As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is not correct. There's almost never a good reason to drop a device link. Even when a device/driver are unbound, we still want future probe attempts to make use of the dependency info and block a device from probing if the supplier hasn't probed. If you don't want the links created by fw_devlink to be relaxed, I think you should instead set the kernel command line param so that the kernel doesn't time out and give up on enforcing dependencies. deferred_probe_timeout=-1 Then you don't have to worry about creating device links. > 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 > :) Regarding [2], I'll try. -Saravana > > [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@analog.com/#t > > - Nuno Sá > > >
On Thu, Jan 25, 2024 at 7:31 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > 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
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@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > 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 having the link around is useful in case we re-probe so we don't need to call the consumer probe function (just to EPROBE_DEFER) without the supplier being already there... - Nuno Sá
On Thu, 2024-01-25 at 16:57 -0800, Saravana Kannan wrote: > On Thu, Jan 25, 2024 at 7:31 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > 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. Yeah, Rafael reply made it clear... - Nuno Sá
On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote: > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > 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. > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is > not correct. There's almost never a good reason to drop a device link. > Even when a device/driver are unbound, we still want future probe > attempts to make use of the dependency info and block a device from > probing if the supplier hasn't probed. > Yeah that makes sense and is making me thinking that I should change my call (in patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure, AUTOREMOVE_CONSUMER won't matter most cases but if someone disables fw_devlinks then it matters. > If you don't want the links created by fw_devlink to be relaxed, I > think you should instead set the kernel command line param so that the > kernel doesn't time out and give up on enforcing dependencies. > deferred_probe_timeout=-1 Good to know... Nope, I don't care much about them being relaxed as I will still call device_link_add() when the consumer probes and finds the supplier. The only downside from relaxing is "loosing" AUTOPROBE_CONSUMER but that is not a big deal. > > Then you don't have to worry about creating device links. > > > 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 > > :) > > Regarding [2], I'll try. > Thanks! I think it's a valid bug with devlinks and overlays but it's sensible stuff (so the RFC) so it would be nice to have some review and recommendations how to solve it... I would definitely like to have it fixed as I see more and more people (ab)using overlays. - Nuno Sá
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@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > 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á
On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote: > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote: > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > 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. > > > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is > > not correct. There's almost never a good reason to drop a device link. > > Even when a device/driver are unbound, we still want future probe > > attempts to make use of the dependency info and block a device from > > probing if the supplier hasn't probed. > > > > Yeah that makes sense and is making me thinking that I should change my call > (in > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure, > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables > fw_devlinks > then it matters. > Yeah, just realized MANAGED is not a valid flag one can pass to device_link_add() :) - Nuno Sá >
On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@gmail.com> wrote: > > On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote: > > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote: > > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > 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. > > > > > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is > > > not correct. There's almost never a good reason to drop a device link. > > > Even when a device/driver are unbound, we still want future probe > > > attempts to make use of the dependency info and block a device from > > > probing if the supplier hasn't probed. > > > > > > > Yeah that makes sense and is making me thinking that I should change my call > > (in > > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure, > > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables > > fw_devlinks > > then it matters. I don't fully understand the patch series, but what exactly are you gaining by adding device links explicitly. I'd rather that every driver didn't explicitly create a device link. That's just a lot of useless code in most cases and we shouldn't have useless code lying around. If someone does fw_devlink=off and you don't create a device link explicitly, what's the worst that's going to happen? Useless deferred probes? fw_devlink is really only there as a debug tool at this point -- I don't think you need to worry about people setting it to off/permissive. > > > > Yeah, just realized MANAGED is not a valid flag one can pass to > device_link_add() :) If you don't pass the STATELESS flag, a link is assumed to be MANAGED. So, you can still create managed device links. -Saravana
On Fri, 2024-01-26 at 10:09 -0800, Saravana Kannan wrote: > On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@gmail.com> wrote: > > > > On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote: > > > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote: > > > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > > > 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. > > > > > > > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is > > > > not correct. There's almost never a good reason to drop a device link. > > > > Even when a device/driver are unbound, we still want future probe > > > > attempts to make use of the dependency info and block a device from > > > > probing if the supplier hasn't probed. > > > > > > > > > > Yeah that makes sense and is making me thinking that I should change my call > > > (in > > > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure, > > > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables > > > fw_devlinks > > > then it matters. > > I don't fully understand the patch series, but what exactly are you > gaining by adding device links explicitly. I'd rather that every > driver didn't explicitly create a device link. That's just a lot of > useless code in most cases and we shouldn't have useless code lying > around. If someone does fw_devlink=off and you don't create a device > link explicitly, what's the worst that's going to happen? Useless > deferred probes? fw_devlink is really only there as a debug tool at > this point -- I don't think you need to worry about people setting it > to off/permissive. > There's (at least) a reasoning behind the explicit use of the links. We want to ensure the creation of a MANAGED link so that we can be assured (i think) that the consumer device will never be around if the supplier unbinds causing those typical issues of a supplier going away and consumers trying to access it's code. Now, in the HW arrangement we're trying to support there's no such thing as optional suppliers. If the IIO backend is going away, there's no good reason for an IIO frontend to stay around. And using the guarantee provided by the links made the code way simpler. Note that in the first versions of the series I was also adding code (together with dev_links) to make sure we would return error in case the consumer tried to access a supplier callback and the supplier is no longer around. That meant a mutex for syncing callbacks plus checking a pointer and having a kref for the backend object. Jonathan (rightfully) complained that I was adding two ways of guaranteeing the same thing. Thus, as we don't really have optional suppliers, we went with the explicit links creation as it makes the code much simpler. If you have any interest, look at [1] (and let me know if there's any wrong assumption). > > > > > > > Yeah, just realized MANAGED is not a valid flag one can pass to > > device_link_add() :) > > If you don't pass the STATELESS flag, a link is assumed to be MANAGED. > So, you can still create managed device links. > Yes, I realized that... Maybe even passing no flag would do the trick. [1]: https://lore.kernel.org/linux-iio/8085910199d4b653edb61c51fc80a503ee50131d.camel@gmail.com/ - Nuno Sá
On Fri, 26 Jan 2024 15:26:08 +0100 Nuno Sá <noname.nuno@gmail.com> 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@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > > > 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!) Jonathan
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@gmail.com> 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@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > > > > > 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á
On Mon, Jan 29, 2024 at 12:26 AM Nuno Sá <noname.nuno@gmail.com> 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@gmail.com> 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@gmail.com> 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@kernel.org> wrote: > > > > > > > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > > > > > > > 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 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? -Saravana
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@gmail.com> 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@gmail.com> 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@gmail.com> > > > > > > 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@kernel.org> wrote: > > > > > > > > > > > > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > > > > > > > > > > > > > > > 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á
diff --git a/drivers/base/core.c b/drivers/base/core.c index 14d46af40f9a..ee8a46df28e1 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -807,11 +807,15 @@ struct device_link *device_link_add(struct device *consumer, * update the existing link to stay around longer. */ if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) { - if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) { - link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER; - link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER; - } - } else if (!(flags & DL_FLAG_AUTOREMOVE_CONSUMER)) { + link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER; + link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER; + link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER; + + } else if (flags & DL_FLAG_AUTOREMOVE_CONSUMER) { + link->flags &= ~DL_FLAG_AUTOREMOVE_SUPPLIER; + link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER; + link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER; + } else { link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER | DL_FLAG_AUTOREMOVE_SUPPLIER); }