Re: [PATCH RESEND RFC] driver: core: don't queue device links removal for dt overlays

[Nuno Sá]

On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> > 
> > On Wed, 2024-01-31 at 14:30 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Jan 31, 2024 at 1:20 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> > > > 
> > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > > > 
> > > > > 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@xxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxx/
> > > > > ---
> > > > >  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.
> > > 
> > 
> > I'm trying to understand if there's no hidden issue by calling it
> > synchronously.
> > (don't think there is but this is rather core stuff :)).
> > 
> > From the DT guys, it would be helpful to get feedback on the new
> > device_node_overlay_removal() helper I'm introducing. The goal is to just do
> > the
> > sync release in case we detect a node being removed as a result of an
> > overlay
> > removal.
> > 
> > > You first need to convince yourself that you are not completely wrong.
> > 
> > I mean, the problem is definitely real and if you see the link I pasted in
> > the
> > cover, this will all lead to big splats.
> > 
> > > 
> > > > +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.
> > > 
> > 
> > Yeah, get_mantainers gives me you and Greg but I think you're the main dev
> > on
> > dev_links right?
> > 
> > > 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.
> > > 
> > 
> > So you mean adding something like adding a new
> > 
> > device_link_release(struct device_link *link) helper
> > and either call it synchronously from devlink_dev_release() or
> > asynchronously
> > from device_link_release_fn()?
> > 
> > I can drop the RFC and send a patch with the above...
> 
> No, IMV devlink_dev_release() needs to be called via
> device_link_put_kref(), but it may run device_link_release_fn()
> directly if the link is marked in a special way or something like
> this.

Sorry, I'm not totally getting this. I'm directly calling
device_link_release_fn() from  devlink_dev_release(). We should only get into
devlink_dev_release() after all the references are dropped right (being it the
release callback for the link class)?

device_node_overlay_removal() is my way to see if the link "is marked in a
special way" as you put it. This checks if one of the supplier/consumer is
marked as an OVERLAY and if it's being removed (I think that OF_DETACHED tells
us that but some feedback from DT guys will be helpful).

Alternatively, I could just check the OF_OVERLAY flag when the link is being
created and have a new variable in struct device_link to flag the synchronous
release. Disadvantage is that in this way even a sysfs unbind or module unload
(without necessarily removing the overly) would lead to a synchronous release
which can actually make sense (now that I think about it). Because if someone
does some crazy thing like "echo device > unbind" and then removes the overlay
we could still hit the overly removal path before device_link_release_fn()
completed.

- Nuno Sá