From: Nuno Sa <[email protected]>
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 <[email protected]>
---
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/[email protected]/
---
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á
On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> From: Nuno Sa <[email protected]>
>
> 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 <[email protected]>
> ---
> 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/[email protected]/
> ---
> 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.
+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 :)
- Nuno Sá
On Wed, Jan 31, 2024 at 1:20 PM Nuno Sá <[email protected]> wrote:
>
> On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > From: Nuno Sa <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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/[email protected]/
> > ---
> > 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, 2024-01-31 at 14:30 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 1:20 PM Nuno Sá <[email protected]> wrote:
> >
> > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > From: Nuno Sa <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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/[email protected]/
> > > ---
> > > 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...
- Nuno Sá
On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <[email protected]> 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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/[email protected]/
> > > > ---
> > > > 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.
On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <[email protected]> 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á <[email protected]> wrote:
> > > >
> > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > From: Nuno Sa <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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/[email protected]/
> > > > > ---
> > > > > 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á
On Wed, Jan 31, 2024 at 3:52 PM Nuno Sá <[email protected]> wrote:
>
> 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@gmailcom> 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á <[email protected]> wrote:
> > > > >
> > > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > > From: Nuno Sa <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > > ---
> > > > > > 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/[email protected]/
> > > > > > ---
> > > > > > 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 = {
> > > > > >
> > > > > > ---
[cut]
> > 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)?
OK, I got confused somehow, sorry.
It should work.
I kind of don't like adding OF-specific code to the driver core, but
if this is fine with Greg, it can be done. It should depend on
CONFIG_OF_OVERLAY, though.
I would like a comment to be added to device_link_release_fn() to
explain why the overlay case needs synchronous execution in there.
> 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.
This sounds more complicated than the current patch.
On Wed, 2024-01-31 at 16:10 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 3:52 PM Nuno Sá <[email protected]> wrote:
> >
> > On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <[email protected]> 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á <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > > > From: Nuno Sa <[email protected]>
> > > > > > >
> > > > > > > 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 <[email protected]>
> > > > > > > ---
> > > > > > > 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/[email protected]/
> > > > > > > ---
> > > > > > > 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 = {
> > > > > > >
> > > > > > > ---
>
> [cut]
>
> > > 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)?
>
> OK, I got confused somehow, sorry.
>
> It should work.
>
> I kind of don't like adding OF-specific code to the driver core, but
> if this is fine with Greg, it can be done. It should depend on
Not perfect but I'm not seeing any other way. We need to somehow see if the node
is part of an OVERLAY and AFAIK, the only way is looking at the node flags. I'll
wait on Greg's feedback.
> CONFIG_OF_OVERLAY, though.
I guess that should be already indirectly implied. I mean if CONFIG_OF_OVERLAY
is not set, I guess there's not way for
of_node_check_flag(dev->of_node, OF_OVERLAY)) return true. But yeah, I can bail
out right away if IS_ENABLED(CONFIG_OF_OVERLAY) is not set.
> I would like a comment to be added to device_link_release_fn() to
> explain why the overlay case needs synchronous execution in there.
I do have the following comment before checking device_node_overlay_removal():
"* 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."
I can elaborate more if you prefer...
>
> > 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.
>
> This sounds more complicated than the current patch.
Agreed...
- Nuno Sá
On Wed, Jan 31, 2024 at 4:46 PM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-01-31 at 16:10 +0100, Rafael J. Wysocki wrote:
> > On Wed, Jan 31, 2024 at 3:52 PM Nuno Sá <noname.nuno@gmailcom> wrote:
> > >
> > > On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <[email protected]> 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á <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > > > > From: Nuno Sa <[email protected]>
> > > > > > > >
> > > > > > > > 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 <[email protected]>
> > > > > > > > ---
> > > > > > > > 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/[email protected]/
> > > > > > > > ---
> > > > > > > > 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 = {
> > > > > > > >
> > > > > > > > ---
> >
> > [cut]
> >
> > > > 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)?
> >
> > OK, I got confused somehow, sorry.
> >
> > It should work.
> >
> > I kind of don't like adding OF-specific code to the driver core, but
> > if this is fine with Greg, it can be done. It should depend on
>
> Not perfect but I'm not seeing any other way. We need to somehow see if the node
> is part of an OVERLAY and AFAIK, the only way is looking at the node flags. I'll
> wait on Greg's feedback.
>
> > CONFIG_OF_OVERLAY, though.
>
> I guess that should be already indirectly implied. I mean if CONFIG_OF_OVERLAY
> is not set, I guess there's not way for
> of_node_check_flag(dev->of_node, OF_OVERLAY)) return true. But yeah, I can bail
> out right away if IS_ENABLED(CONFIG_OF_OVERLAY) is not set.
>
> > I would like a comment to be added to device_link_release_fn() to
> > explain why the overlay case needs synchronous execution in there.
>
> I do have the following comment before checking device_node_overlay_removal():
>
>
> "* 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."
>
> I can elaborate more if you prefer...
No, that should suffice IMV, thanks.
Now that I think of it there is one more possibility: A dedicated
workqueue can be used for running device_link_release_fn() and the DT
overlay code can flush it after the device link deletion.
On Wed, 2024-01-31 at 17:05 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2024 at 4:46 PM Nuno Sá <[email protected]> wrote:
> >
> > On Wed, 2024-01-31 at 16:10 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Jan 31, 2024 at 3:52 PM Nuno Sá <[email protected]> wrote:
> > > >
> > > > On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> > > > > On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <[email protected]> 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á <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > > > > > From: Nuno Sa <[email protected]>
> > > > > > > > >
> > > > > > > > > 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 <[email protected]>
> > > > > > > > > ---
> > > > > > > > > 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/[email protected]/
> > > > > > > > > ---
> > > > > > > > > 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 = {
> > > > > > > > >
> > > > > > > > > ---
> > >
> > > [cut]
> > >
> > > > > 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)?
> > >
> > > OK, I got confused somehow, sorry.
> > >
> > > It should work.
> > >
> > > I kind of don't like adding OF-specific code to the driver core, but
> > > if this is fine with Greg, it can be done. It should depend on
> >
> > Not perfect but I'm not seeing any other way. We need to somehow see if the node
> > is part of an OVERLAY and AFAIK, the only way is looking at the node flags. I'll
> > wait on Greg's feedback.
> >
> > > CONFIG_OF_OVERLAY, though.
> >
> > I guess that should be already indirectly implied. I mean if CONFIG_OF_OVERLAY
> > is not set, I guess there's not way for
> > of_node_check_flag(dev->of_node, OF_OVERLAY)) return true. But yeah, I can bail
> > out right away if IS_ENABLED(CONFIG_OF_OVERLAY) is not set.
> >
> > > I would like a comment to be added to device_link_release_fn() to
> > > explain why the overlay case needs synchronous execution in there.
> >
> > I do have the following comment before checking device_node_overlay_removal():
> >
> >
> > "* 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."
> >
> > I can elaborate more if you prefer...
>
> No, that should suffice IMV, thanks.
>
> Now that I think of it there is one more possibility: A dedicated
> workqueue can be used for running device_link_release_fn() and the DT
> overlay code can flush it after the device link deletion.
Hmm, I did some experiments with that but having a work item per kobject (being that
the common thing between of_node and device) but that (even though it worked) ended
up being overly complicated.
I think I went with the above because, in theory, you can have DT waiting for
unrelated releases but that should really be a corner a case. So, if you prefer a
dedicated queue I can give that a try to see how it looks like. Thinking in
allocating a queue (maybe with the same flags as system_long_wq) in:
devlink_class_init()
Then we can have a new fwnode_link_flush_queue() helper to call from DT. Naming it
like this as DT guys might also not like much of having device specific calls in
there (and fwnode_link) actually fits IMO).
Is it something like this that you have in mind?
Thanks for helping!
- Nuno Sá
On Thu, Feb 1, 2024 at 9:24 AM Nuno Sá <[email protected]> wrote:
>
> On Wed, 2024-01-31 at 17:05 +0100, Rafael J. Wysocki wrote:
> > On Wed, Jan 31, 2024 at 4:46 PM Nuno Sá <noname.nuno@gmailcom> wrote:
> > >
> > > On Wed, 2024-01-31 at 16:10 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Jan 31, 2024 at 3:52 PM Nuno Sá <[email protected]> wrote:
> > > > >
> > > > > On Wed, 2024-01-31 at 15:28 +0100, Rafael J. Wysocki wrote:
> > > > > > On Wed, Jan 31, 2024 at 3:18 PM Nuno Sá <[email protected]> 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á <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, 2024-01-23 at 16:40 +0100, Nuno Sa via B4 Relay wrote:
> > > > > > > > > > From: Nuno Sa <[email protected]>
> > > > > > > > > >
> > > > > > > > > > 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 <[email protected]>
> > > > > > > > > > ---
> > > > > > > > > > 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/[email protected]/
> > > > > > > > > > ---
> > > > > > > > > > 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 = {
> > > > > > > > > >
> > > > > > > > > > ---
> > > >
> > > > [cut]
> > > >
> > > > > > 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)?
> > > >
> > > > OK, I got confused somehow, sorry.
> > > >
> > > > It should work.
> > > >
> > > > I kind of don't like adding OF-specific code to the driver core, but
> > > > if this is fine with Greg, it can be done. It should depend on
> > >
> > > Not perfect but I'm not seeing any other way. We need to somehow see if the node
> > > is part of an OVERLAY and AFAIK, the only way is looking at the node flags. I'll
> > > wait on Greg's feedback.
> > >
> > > > CONFIG_OF_OVERLAY, though.
> > >
> > > I guess that should be already indirectly implied. I mean if CONFIG_OF_OVERLAY
> > > is not set, I guess there's not way for
> > > of_node_check_flag(dev->of_node, OF_OVERLAY)) return true. But yeah, I can bail
> > > out right away if IS_ENABLED(CONFIG_OF_OVERLAY) is not set.
> > >
> > > > I would like a comment to be added to device_link_release_fn() to
> > > > explain why the overlay case needs synchronous execution in there.
> > >
> > > I do have the following comment before checking device_node_overlay_removal():
> > >
> > >
> > > "* 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."
> > >
> > > I can elaborate more if you prefer...
> >
> > No, that should suffice IMV, thanks.
> >
> > Now that I think of it there is one more possibility: A dedicated
> > workqueue can be used for running device_link_release_fn() and the DT
> > overlay code can flush it after the device link deletion.
>
> Hmm, I did some experiments with that but having a work item per kobject (being that
> the common thing between of_node and device) but that (even though it worked) ended
> up being overly complicated.
>
> I think I went with the above because, in theory, you can have DT waiting for
> unrelated releases but that should really be a corner a case. So, if you prefer a
> dedicated queue I can give that a try to see how it looks like. Thinking in
> allocating a queue (maybe with the same flags as system_long_wq) in:
>
> devlink_class_init()
>
> Then we can have a new fwnode_link_flush_queue() helper to call from DT. Naming it
> like this as DT guys might also not like much of having device specific calls in
> there (and fwnode_link) actually fits IMO).
>
> Is it something like this that you have in mind?
Yes, something like this.