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

  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(-)


---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240123-fix-device-links-overlays-5422e033a09b
--

Thanks!
- Nuno Sá
  

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.
  

On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <noname.nuno@gmail.com> 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@gmailcom> 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.
> >
>
> 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.

AFAICS, this is the only way to do it and be sure that all of the
references to the link have been dropped when it is freed.