Messages in this thread | ![/](/images/icornerl.gif) | | From | "Rafael J. Wysocki" <> | Date | Wed, 31 Jan 2024 14:30:20 +0100 | Subject | Re: [PATCH RESEND RFC] driver: core: don't queue device links removal for dt overlays |
| |
On Wed, Jan 31, 2024 at 1:20 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote: > > From: Nuno Sa <nuno.sa@analog.com> > > > > For device links, releasing the supplier/consumer devices references > > happens asynchronously in device_link_release_fn(). Hence, the possible > > release of an of_node is also asynchronous. If these nodes were added > > through overlays we have a problem because this does not respect the > > devicetree overlays assumptions that when a changeset is > > being removed in __of_changeset_entry_destroy(), it must hold the last > > reference to that node. Due to the async nature of device links that > > cannot be guaranteed. > > > > Given the above, in case one of the link consumer/supplier is part of > > an overlay node we call directly device_link_release_fn() instead of > > queueing it. Yes, it might take some significant time for > > device_link_release_fn() to complete because of synchronize_srcu() but > > we would need to, anyways, wait for all OF references to be released if > > we want to respect overlays assumptions. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > This RFC is a follow up of a previous one that I sent to the devicetree > > folks [1]. It got rejected because it was not really fixing the root > > cause of the issue (which I do agree). Please see the link where I > > fully explain what the issue is. > > > > I did also some git blaming and did saw that commit > > 80dd33cf72d1 ("drivers: base: Fix device link removal") introduced > > queue_work() as we could be releasing the last device reference and hence > > sleeping which is against SRCU callback requirements. However, that same > > commit is now making use of synchronize_srcu() which may take > > significant time (and I think that's the reason for the work item?). > > > > However, given the dt overlays requirements, I'm not seeing any > > reason to not be able to run device_link_release_fn() synchronously if we > > detect an OVERLAY node is being released. I mean, even if we come up > > (and I did some experiments in this regard) with some async mechanism to > > release the OF nodes refcounts, we still need a synchronization point > > somewhere. > > > > Anyways, I would like to have some feedback on how acceptable would this > > be or what else could I do so we can have a "clean" dt overlay removal. > > > > I'm also including dt folks so they can give some comments on the new > > device_node_overlay_removal() function. My goal is to try to detect when an > > overlay is being removed (maybe we could even have an explicit flag for > > it?) and only directly call device_link_release_fn() in that case. > > > > [1]: > > https://lore.kernel.org/linux-devicetree/20230511151047.1779841-1-nuno.sa@analog.com/ > > --- > > drivers/base/core.c | 25 ++++++++++++++++++++++++- > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > index 14d46af40f9a..31ea001f6142 100644 > > --- a/drivers/base/core.c > > +++ b/drivers/base/core.c > > @@ -497,6 +497,18 @@ static struct attribute *devlink_attrs[] = { > > }; > > ATTRIBUTE_GROUPS(devlink); > > > > +static bool device_node_overlay_removal(struct device *dev) > > +{ > > + if (!dev_of_node(dev)) > > + return false; > > + if (!of_node_check_flag(dev->of_node, OF_DETACHED)) > > + return false; > > + if (!of_node_check_flag(dev->of_node, OF_OVERLAY)) > > + return false; > > + > > + return true; > > +} > > + > > static void device_link_release_fn(struct work_struct *work) > > { > > struct device_link *link = container_of(work, struct device_link, > > rm_work); > > @@ -532,8 +544,19 @@ static void devlink_dev_release(struct device *dev) > > * synchronization in device_link_release_fn() and if the consumer or > > * supplier devices get deleted when it runs, so put it into the > > "long" > > * workqueue. > > + * > > + * However, if any of the supplier, consumer nodes is being removed > > + * through overlay removal, the expectation in > > + * __of_changeset_entry_destroy() is for the node 'kref' to be 1 > > which > > + * cannot be guaranteed with the async nature of > > + * device_link_release_fn(). Hence, do it synchronously for the > > overlay > > + * case. > > */ > > - queue_work(system_long_wq, &link->rm_work); > > + if (device_node_overlay_removal(link->consumer) || > > + device_node_overlay_removal(link->supplier)) > > + device_link_release_fn(&link->rm_work); > > + else > > + queue_work(system_long_wq, &link->rm_work); > > } > > > > static struct class devlink_class = { > > > > --- > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > > change-id: 20240123-fix-device-links-overlays-5422e033a09b > > -- > > > > Thanks! > > - Nuno Sá > > > > Hi Rafael, > > Would be nice to have your feedback on this one or if this is a complete nack... > I think calling device_link_release_fn() synchronously is ok but I might be > completely wrong.
Well, it sounds like you are expecting me to confirm that what you are doing makes sense, but I cannot do that, because I am not sufficiently familiar with DT overlays.
You first need to convince yourself that you are not completely wrong.
> +Cc Saravan as he should also be very familiar with device_links and see if the > above fairly simple solution is sane. > > I also don't want to be pushy as I know you guys are all very busy but it's (i > think) the third time I resend the patch :)
Sorry about that, I haven't realized that my input is requisite.
So the patch not only calls device_link_release_fn() synchronously, but it also calls this function directly and I, personally, wouldn't do at least the latter.
It should be fine to run it synchronously from within devlink_dev_release(), it will just take time for the SRCU synchronization, but AFAICS it is not generally safe to run it without dropping the last reference to the device link.
| ![\](/images/icornerr.gif) |