[PATCH v4 0/2] Synchronize DT overlay removal with devlink removals

[PATCH v4 0/2] Synchronize DT overlay removal with devlink removals

[Herve Codina]

Hi,

In the following sequence:
  of_platform_depopulate(); /* Remove devices from a DT overlay node */
  of_overlay_remove(); /* Remove the DT overlay node itself */

Some warnings are raised by __of_changeset_entry_destroy() which  was
called from of_overlay_remove():
  ERROR: memory leak, expected refcount 1 instead of 2 ...

The issue is that, during the device devlink removals triggered from the
of_platform_depopulate(), jobs are put in a workqueue.
These jobs drop the reference to the devices. When a device is no more
referenced (refcount == 0), it is released and the reference to its
of_node is dropped by a call to of_node_put().
These operations are fully correct except that, because of the
workqueue, they are done asynchronously with respect to function calls.

In the sequence provided, the jobs are run too late, after the call to
__of_changeset_entry_destroy() and so a missing of_node_put() call is
detected by __of_changeset_entry_destroy().

This series fixes this issue introducing device_link_wait_removal() in
order to wait for the end of jobs execution (patch 1) and using this
function to synchronize the overlay removal with the end of jobs
execution (patch 2).

Compared to the previous iteration:
  https://lore.kernel.org/linux-kernel/20240229105204.720717-1-herve.codina@bootlin.com/
this v4 series:
- Uses flush_workqueue()
- Keep the of_mutex lock taken calling device_link_wait_removal()
- Perform the wait in of_changeset_destroy()

This series handles cases reported by Luca [1] and Nuno [2].
  [1]: https://lore.kernel.org/all/20231220181627.341e8789@booty/
  [2]: https://lore.kernel.org/all/20240205-fix-device-links-overlays-v2-2-5344f8c79d57@analog.com/

Best regards,
Hervé

Changes v3 -> v4
  - Patch 1
    Uses flush_workqueue() instead of drain_workqueue().

  - Patch 2
    Remove unlock/re-lock when calling device_link_wait_removal()
    Move device_link_wait_removal() call to of_changeset_destroy()
    Update commit log

Changes v2 -> v3
  - Patch 1
    No changes

  - Patch 2
    Add missing device.h

Changes v1 -> v2
  - Patch 1
    Rename the workqueue to 'device_link_wq'
    Add 'Fixes' tag and Cc stable

  - Patch 2
    Add device.h inclusion.
    Call device_link_wait_removal() later in the overlay removal
    sequence (i.e. in free_overlay_changeset() function).
    Drop of_mutex lock while calling device_link_wait_removal().
    Add	'Fixes'	tag and Cc stable

Herve Codina (2):
  driver core: Introduce device_link_wait_removal()
  of: dynamic: Synchronize of_changeset_destroy() with the devlink
    removals

 drivers/base/core.c    | 26 +++++++++++++++++++++++---
 drivers/of/dynamic.c   |  7 +++++++
 include/linux/device.h |  1 +
 3 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Herve Codina]

The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
introduces a workqueue to release the consumer and supplier devices used
in the devlink.
In the job queued, devices are release and in turn, when all the
references to these devices are dropped, the release function of the
device itself is called.

Nothing is present to provide some synchronisation with this workqueue
in order to ensure that all ongoing releasing operations are done and
so, some other operations can be started safely.

For instance, in the following sequence:
  1) of_platform_depopulate()
  2) of_overlay_remove()

During the step 1, devices are released and related devlinks are removed
(jobs pushed in the workqueue).
During the step 2, OF nodes are destroyed but, without any
synchronisation with devlink removal jobs, of_overlay_remove() can raise
warnings related to missing of_node_put():
  ERROR: memory leak, expected refcount 1 instead of 2

Indeed, the missing of_node_put() call is going to be done, too late,
from the workqueue job execution.

Introduce device_link_wait_removal() to offer a way to synchronize
operations waiting for the end of devlink removals (i.e. end of
workqueue jobs).
Also, as a flushing operation is done on the workqueue, the workqueue
used is moved from a system-wide workqueue to a local one.

Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
Cc: stable@vger.kernel.org
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c    | 26 +++++++++++++++++++++++---
 include/linux/device.h |  1 +
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d5f4e4aac09b..48b28c59c592 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
 static void __fw_devlink_link_to_consumers(struct device *dev);
 static bool fw_devlink_drv_reg_done;
 static bool fw_devlink_best_effort;
+static struct workqueue_struct *device_link_wq;
 
 /**
  * __fwnode_link_add - Create a link between two fwnode_handles.
@@ -532,12 +533,26 @@ static void devlink_dev_release(struct device *dev)
 	/*
 	 * It may take a while to complete this work because of the SRCU
 	 * 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.
+	 * supplier devices get deleted when it runs, so put it into the
+	 * dedicated workqueue.
 	 */
-	queue_work(system_long_wq, &link->rm_work);
+	queue_work(device_link_wq, &link->rm_work);
 }
 
+/**
+ * device_link_wait_removal - Wait for ongoing devlink removal jobs to terminate
+ */
+void device_link_wait_removal(void)
+{
+	/*
+	 * devlink removal jobs are queued in the dedicated work queue.
+	 * To be sure that all removal jobs are terminated, ensure that any
+	 * scheduled work has run to completion.
+	 */
+	flush_workqueue(device_link_wq);
+}
+EXPORT_SYMBOL_GPL(device_link_wait_removal);
+
 static struct class devlink_class = {
 	.name = "devlink",
 	.dev_groups = devlink_groups,
@@ -4099,9 +4114,14 @@ int __init devices_init(void)
 	sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
 	if (!sysfs_dev_char_kobj)
 		goto char_kobj_err;
+	device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
+	if (!device_link_wq)
+		goto wq_err;
 
 	return 0;
 
+ wq_err:
+	kobject_put(sysfs_dev_char_kobj);
  char_kobj_err:
 	kobject_put(sysfs_dev_block_kobj);
  block_kobj_err:
diff --git a/include/linux/device.h b/include/linux/device.h
index 1795121dee9a..d7d8305a72e8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1249,6 +1249,7 @@ void device_link_del(struct device_link *link);
 void device_link_remove(void *consumer, struct device *supplier);
 void device_links_supplier_sync_state_pause(void);
 void device_links_supplier_sync_state_resume(void);
+void device_link_wait_removal(void);
 
 /* Create alias, so I can be autoloaded. */
 #define MODULE_ALIAS_CHARDEV(major,minor) \
-- 
2.43.0

[PATCH v4 2/2] of: dynamic: Synchronize of_changeset_destroy() with the devlink removals

[Herve Codina]

In the following sequence:
  1) of_platform_depopulate()
  2) of_overlay_remove()

During the step 1, devices are destroyed and devlinks are removed.
During the step 2, OF nodes are destroyed but
__of_changeset_entry_destroy() can raise warnings related to missing
of_node_put():
  ERROR: memory leak, expected refcount 1 instead of 2 ...

Indeed, during the devlink removals performed at step 1, the removal
itself releasing the device (and the attached of_node) is done by a job
queued in a workqueue and so, it is done asynchronously with respect to
function calls.
When the warning is present, of_node_put() will be called but wrongly
too late from the workqueue job.

In order to be sure that any ongoing devlink removals are done before
the of_node destruction, synchronize the of_changeset_destroy() with the
devlink removals.

Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
Cc: stable@vger.kernel.org
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/dynamic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 3bf27052832f..169e2a9ae22f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt)	"OF: " fmt
 
+#include <linux/device.h>
 #include <linux/of.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -667,6 +668,12 @@ void of_changeset_destroy(struct of_changeset *ocs)
 {
 	struct of_changeset_entry *ce, *cen;
 
+	/*
+	 * Wait for any ongoing device link removals before destroying some of
+	 * nodes.
+	 */
+	device_link_wait_removal();
+
 	list_for_each_entry_safe_reverse(ce, cen, &ocs->entries, node)
 		__of_changeset_entry_destroy(ce);
 }
-- 
2.43.0

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Nuno Sá]

On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> introduces a workqueue to release the consumer and supplier devices used
> in the devlink.
> In the job queued, devices are release and in turn, when all the
> references to these devices are dropped, the release function of the
> device itself is called.
> 
> Nothing is present to provide some synchronisation with this workqueue
> in order to ensure that all ongoing releasing operations are done and
> so, some other operations can be started safely.
> 
> For instance, in the following sequence:
>   1) of_platform_depopulate()
>   2) of_overlay_remove()
> 
> During the step 1, devices are released and related devlinks are removed
> (jobs pushed in the workqueue).
> During the step 2, OF nodes are destroyed but, without any
> synchronisation with devlink removal jobs, of_overlay_remove() can raise
> warnings related to missing of_node_put():
>   ERROR: memory leak, expected refcount 1 instead of 2
> 
> Indeed, the missing of_node_put() call is going to be done, too late,
> from the workqueue job execution.
> 
> Introduce device_link_wait_removal() to offer a way to synchronize
> operations waiting for the end of devlink removals (i.e. end of
> workqueue jobs).
> Also, as a flushing operation is done on the workqueue, the workqueue
> used is moved from a system-wide workqueue to a local one.
> 
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

With the below addressed:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/base/core.c    | 26 +++++++++++++++++++++++---
>  include/linux/device.h |  1 +
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d5f4e4aac09b..48b28c59c592 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
>  static void __fw_devlink_link_to_consumers(struct device *dev);
>  static bool fw_devlink_drv_reg_done;
>  static bool fw_devlink_best_effort;
> +static struct workqueue_struct *device_link_wq;
>  
>  /**
>   * __fwnode_link_add - Create a link between two fwnode_handles.
> @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device *dev)
>  	/*
>  	 * It may take a while to complete this work because of the SRCU
>  	 * 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.
> +	 * supplier devices get deleted when it runs, so put it into the
> +	 * dedicated workqueue.
>  	 */
> -	queue_work(system_long_wq, &link->rm_work);
> +	queue_work(device_link_wq, &link->rm_work);
>  }
>  
> +/**
> + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> terminate
> + */
> +void device_link_wait_removal(void)
> +{
> +	/*
> +	 * devlink removal jobs are queued in the dedicated work queue.
> +	 * To be sure that all removal jobs are terminated, ensure that any
> +	 * scheduled work has run to completion.
> +	 */
> +	flush_workqueue(device_link_wq);
> +}
> +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> +
>  static struct class devlink_class = {
>  	.name = "devlink",
>  	.dev_groups = devlink_groups,
> @@ -4099,9 +4114,14 @@ int __init devices_init(void)
>  	sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
>  	if (!sysfs_dev_char_kobj)
>  		goto char_kobj_err;
> +	device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> +	if (!device_link_wq)
> +		goto wq_err;
>  

I can't still agree with this. Why not doing it in devlink_class_init()? This is
devlink specific so it makes complete sense to me.

Note that this maybe the 3/4 time I'm arguing in here. If you don't agree please
tell me why and I may agree with you :).

(and sorry if you already said something about it and I missed)

- Nuno Sá
> 

Re: [PATCH v4 2/2] of: dynamic: Synchronize of_changeset_destroy() with the devlink removals

[Nuno Sá]

On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> In the following sequence:
>   1) of_platform_depopulate()
>   2) of_overlay_remove()
> 
> During the step 1, devices are destroyed and devlinks are removed.
> During the step 2, OF nodes are destroyed but
> __of_changeset_entry_destroy() can raise warnings related to missing
> of_node_put():
>   ERROR: memory leak, expected refcount 1 instead of 2 ...
> 
> Indeed, during the devlink removals performed at step 1, the removal
> itself releasing the device (and the attached of_node) is done by a job
> queued in a workqueue and so, it is done asynchronously with respect to
> function calls.
> When the warning is present, of_node_put() will be called but wrongly
> too late from the workqueue job.
> 
> In order to be sure that any ongoing devlink removals are done before
> the of_node destruction, synchronize the of_changeset_destroy() with the
> devlink removals.
> 
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

LGTM

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/of/dynamic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 3bf27052832f..169e2a9ae22f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -9,6 +9,7 @@
>  
>  #define pr_fmt(fmt)	"OF: " fmt
>  
> +#include <linux/device.h>
>  #include <linux/of.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -667,6 +668,12 @@ void of_changeset_destroy(struct of_changeset *ocs)
>  {
>  	struct of_changeset_entry *ce, *cen;
>  
> +	/*
> +	 * Wait for any ongoing device link removals before destroying some
> of
> +	 * nodes.
> +	 */
> +	device_link_wait_removal();
> +
>  	list_for_each_entry_safe_reverse(ce, cen, &ocs->entries, node)
>  		__of_changeset_entry_destroy(ce);
>  }

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Luca Ceresoli]

On Wed,  6 Mar 2024 09:50:02 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> introduces a workqueue to release the consumer and supplier devices used
> in the devlink.
> In the job queued, devices are release and in turn, when all the
> references to these devices are dropped, the release function of the
> device itself is called.
> 
> Nothing is present to provide some synchronisation with this workqueue
> in order to ensure that all ongoing releasing operations are done and
> so, some other operations can be started safely.
> 
> For instance, in the following sequence:
>   1) of_platform_depopulate()
>   2) of_overlay_remove()
> 
> During the step 1, devices are released and related devlinks are removed
> (jobs pushed in the workqueue).
> During the step 2, OF nodes are destroyed but, without any
> synchronisation with devlink removal jobs, of_overlay_remove() can raise
> warnings related to missing of_node_put():
>   ERROR: memory leak, expected refcount 1 instead of 2
> 
> Indeed, the missing of_node_put() call is going to be done, too late,
> from the workqueue job execution.
> 
> Introduce device_link_wait_removal() to offer a way to synchronize
> operations waiting for the end of devlink removals (i.e. end of
> workqueue jobs).
> Also, as a flushing operation is done on the workqueue, the workqueue
> used is moved from a system-wide workqueue to a local one.
> 
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 2/2] of: dynamic: Synchronize of_changeset_destroy() with the devlink removals

[Luca Ceresoli]

On Wed,  6 Mar 2024 09:50:03 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> In the following sequence:
>   1) of_platform_depopulate()
>   2) of_overlay_remove()
> 
> During the step 1, devices are destroyed and devlinks are removed.
> During the step 2, OF nodes are destroyed but
> __of_changeset_entry_destroy() can raise warnings related to missing
> of_node_put():
>   ERROR: memory leak, expected refcount 1 instead of 2 ...
> 
> Indeed, during the devlink removals performed at step 1, the removal
> itself releasing the device (and the attached of_node) is done by a job
> queued in a workqueue and so, it is done asynchronously with respect to
> function calls.
> When the warning is present, of_node_put() will be called but wrongly
> too late from the workqueue job.
> 
> In order to be sure that any ongoing devlink removals are done before
> the of_node destruction, synchronize the of_changeset_destroy() with the
> devlink removals.
> 
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/of/dynamic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 3bf27052832f..169e2a9ae22f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -9,6 +9,7 @@
>  
>  #define pr_fmt(fmt)	"OF: " fmt
>  
> +#include <linux/device.h>
>  #include <linux/of.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -667,6 +668,12 @@ void of_changeset_destroy(struct of_changeset *ocs)
>  {
>  	struct of_changeset_entry *ce, *cen;
>  
> +	/*
> +	 * Wait for any ongoing device link removals before destroying some of
> +	 * nodes.
> +	 */
> +	device_link_wait_removal();

Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

And no problem appeared in my tests due to the removed unlock/lock
around device_link_wait_removal().

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Rafael J. Wysocki]

On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > introduces a workqueue to release the consumer and supplier devices used
> > in the devlink.
> > In the job queued, devices are release and in turn, when all the
> > references to these devices are dropped, the release function of the
> > device itself is called.
> >
> > Nothing is present to provide some synchronisation with this workqueue
> > in order to ensure that all ongoing releasing operations are done and
> > so, some other operations can be started safely.
> >
> > For instance, in the following sequence:
> >   1) of_platform_depopulate()
> >   2) of_overlay_remove()
> >
> > During the step 1, devices are released and related devlinks are removed
> > (jobs pushed in the workqueue).
> > During the step 2, OF nodes are destroyed but, without any
> > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > warnings related to missing of_node_put():
> >   ERROR: memory leak, expected refcount 1 instead of 2
> >
> > Indeed, the missing of_node_put() call is going to be done, too late,
> > from the workqueue job execution.
> >
> > Introduce device_link_wait_removal() to offer a way to synchronize
> > operations waiting for the end of devlink removals (i.e. end of
> > workqueue jobs).
> > Also, as a flushing operation is done on the workqueue, the workqueue
> > used is moved from a system-wide workqueue to a local one.
> >
> > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
>
> With the below addressed:
>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>
> >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> >  include/linux/device.h |  1 +
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d5f4e4aac09b..48b28c59c592 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> >  static void __fw_devlink_link_to_consumers(struct device *dev);
> >  static bool fw_devlink_drv_reg_done;
> >  static bool fw_devlink_best_effort;
> > +static struct workqueue_struct *device_link_wq;
> >
> >  /**
> >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device *dev)
> >       /*
> >        * It may take a while to complete this work because of the SRCU
> >        * 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.
> > +      * supplier devices get deleted when it runs, so put it into the
> > +      * dedicated workqueue.
> >        */
> > -     queue_work(system_long_wq, &link->rm_work);
> > +     queue_work(device_link_wq, &link->rm_work);
> >  }
> >
> > +/**
> > + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> > terminate
> > + */
> > +void device_link_wait_removal(void)
> > +{
> > +     /*
> > +      * devlink removal jobs are queued in the dedicated work queue.
> > +      * To be sure that all removal jobs are terminated, ensure that any
> > +      * scheduled work has run to completion.
> > +      */
> > +     flush_workqueue(device_link_wq);
> > +}
> > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > +
> >  static struct class devlink_class = {
> >       .name = "devlink",
> >       .dev_groups = devlink_groups,
> > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> >       sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> >       if (!sysfs_dev_char_kobj)
> >               goto char_kobj_err;
> > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > +     if (!device_link_wq)
> > +             goto wq_err;
> >
>
> I can't still agree with this. Why not doing it in devlink_class_init()? This is
> devlink specific so it makes complete sense to me.

If you do that in devlink_class_init() and it fails, you essentially
cause the creation of every device link to fail.  IOW, you try to live
without device links and pretend that it is all OK.  That won't get
you very far, especially on systems where DT is used.

Doing it here, if it fails, you prevent the driver model from working
at all (because one of its necessary components is unavailable), which
arguably is a better choice.

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Rafael J. Wysocki]

On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> introduces a workqueue to release the consumer and supplier devices used
> in the devlink.
> In the job queued, devices are release and in turn, when all the
> references to these devices are dropped, the release function of the
> device itself is called.
>
> Nothing is present to provide some synchronisation with this workqueue
> in order to ensure that all ongoing releasing operations are done and
> so, some other operations can be started safely.
>
> For instance, in the following sequence:
>   1) of_platform_depopulate()
>   2) of_overlay_remove()
>
> During the step 1, devices are released and related devlinks are removed
> (jobs pushed in the workqueue).
> During the step 2, OF nodes are destroyed but, without any
> synchronisation with devlink removal jobs, of_overlay_remove() can raise
> warnings related to missing of_node_put():
>   ERROR: memory leak, expected refcount 1 instead of 2
>
> Indeed, the missing of_node_put() call is going to be done, too late,
> from the workqueue job execution.
>
> Introduce device_link_wait_removal() to offer a way to synchronize
> operations waiting for the end of devlink removals (i.e. end of
> workqueue jobs).
> Also, as a flushing operation is done on the workqueue, the workqueue
> used is moved from a system-wide workqueue to a local one.
>
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")

No, it is not fixed by this patch.

In fact, the only possibly observable effect of this patch is the
failure when the allocation of device_link_wq fails AFAICS.

> Cc: stable@vger.kernel.org

So why?

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/base/core.c    | 26 +++++++++++++++++++++++---
>  include/linux/device.h |  1 +
>  2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d5f4e4aac09b..48b28c59c592 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
>  static void __fw_devlink_link_to_consumers(struct device *dev);
>  static bool fw_devlink_drv_reg_done;
>  static bool fw_devlink_best_effort;
> +static struct workqueue_struct *device_link_wq;
>
>  /**
>   * __fwnode_link_add - Create a link between two fwnode_handles.
> @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device *dev)
>         /*
>          * It may take a while to complete this work because of the SRCU
>          * 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.
> +        * supplier devices get deleted when it runs, so put it into the
> +        * dedicated workqueue.
>          */
> -       queue_work(system_long_wq, &link->rm_work);
> +       queue_work(device_link_wq, &link->rm_work);
>  }
>
> +/**
> + * device_link_wait_removal - Wait for ongoing devlink removal jobs to terminate
> + */
> +void device_link_wait_removal(void)
> +{
> +       /*
> +        * devlink removal jobs are queued in the dedicated work queue.
> +        * To be sure that all removal jobs are terminated, ensure that any
> +        * scheduled work has run to completion.
> +        */
> +       flush_workqueue(device_link_wq);
> +}
> +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> +
>  static struct class devlink_class = {
>         .name = "devlink",
>         .dev_groups = devlink_groups,
> @@ -4099,9 +4114,14 @@ int __init devices_init(void)
>         sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
>         if (!sysfs_dev_char_kobj)
>                 goto char_kobj_err;
> +       device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> +       if (!device_link_wq)
> +               goto wq_err;
>
>         return 0;
>
> + wq_err:
> +       kobject_put(sysfs_dev_char_kobj);
>   char_kobj_err:
>         kobject_put(sysfs_dev_block_kobj);
>   block_kobj_err:
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1795121dee9a..d7d8305a72e8 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1249,6 +1249,7 @@ void device_link_del(struct device_link *link);
>  void device_link_remove(void *consumer, struct device *supplier);
>  void device_links_supplier_sync_state_pause(void);
>  void device_links_supplier_sync_state_resume(void);
> +void device_link_wait_removal(void);
>
>  /* Create alias, so I can be autoloaded. */
>  #define MODULE_ALIAS_CHARDEV(major,minor) \
> --

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Nuno Sá]

On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > introduces a workqueue to release the consumer and supplier devices used
> > > in the devlink.
> > > In the job queued, devices are release and in turn, when all the
> > > references to these devices are dropped, the release function of the
> > > device itself is called.
> > > 
> > > Nothing is present to provide some synchronisation with this workqueue
> > > in order to ensure that all ongoing releasing operations are done and
> > > so, some other operations can be started safely.
> > > 
> > > For instance, in the following sequence:
> > >   1) of_platform_depopulate()
> > >   2) of_overlay_remove()
> > > 
> > > During the step 1, devices are released and related devlinks are removed
> > > (jobs pushed in the workqueue).
> > > During the step 2, OF nodes are destroyed but, without any
> > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > warnings related to missing of_node_put():
> > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > 
> > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > from the workqueue job execution.
> > > 
> > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > operations waiting for the end of devlink removals (i.e. end of
> > > workqueue jobs).
> > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > used is moved from a system-wide workqueue to a local one.
> > > 
> > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > 
> > With the below addressed:
> > 
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > 
> > >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> > >  include/linux/device.h |  1 +
> > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index d5f4e4aac09b..48b28c59c592 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > >  static void __fw_devlink_link_to_consumers(struct device *dev);
> > >  static bool fw_devlink_drv_reg_done;
> > >  static bool fw_devlink_best_effort;
> > > +static struct workqueue_struct *device_link_wq;
> > > 
> > >  /**
> > >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device *dev)
> > >       /*
> > >        * It may take a while to complete this work because of the SRCU
> > >        * 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.
> > > +      * supplier devices get deleted when it runs, so put it into the
> > > +      * dedicated workqueue.
> > >        */
> > > -     queue_work(system_long_wq, &link->rm_work);
> > > +     queue_work(device_link_wq, &link->rm_work);
> > >  }
> > > 
> > > +/**
> > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> > > terminate
> > > + */
> > > +void device_link_wait_removal(void)
> > > +{
> > > +     /*
> > > +      * devlink removal jobs are queued in the dedicated work queue.
> > > +      * To be sure that all removal jobs are terminated, ensure that any
> > > +      * scheduled work has run to completion.
> > > +      */
> > > +     flush_workqueue(device_link_wq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > +
> > >  static struct class devlink_class = {
> > >       .name = "devlink",
> > >       .dev_groups = devlink_groups,
> > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > >       sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > >       if (!sysfs_dev_char_kobj)
> > >               goto char_kobj_err;
> > > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > +     if (!device_link_wq)
> > > +             goto wq_err;
> > > 
> > 
> > I can't still agree with this. Why not doing it in devlink_class_init()?
> > This is
> > devlink specific so it makes complete sense to me.
> 
> If you do that in devlink_class_init() and it fails, you essentially
> cause the creation of every device link to fail.  IOW, you try to live
> without device links and pretend that it is all OK.  That won't get
> you very far, especially on systems where DT is used.
> 
> Doing it here, if it fails, you prevent the driver model from working
> at all (because one of its necessary components is unavailable), which
> arguably is a better choice.

That makes sense but then the only thing I still don't fully get is why we have
a separate devlink_class_init() initcall for registering the devlink class
(which can also fail)... What I take from the above is that we should fail the
driver model if one of it's fundamental components fails so I would say we
should merge devlink_class_init() with device_init() otherwise it's a bit
confusing (at least to me) and gives the idea that it's ok for the driver model
to exist without the links (unless I'm missing some other reason for the devlink
init function).

- Nuno Sá

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Rafael J. Wysocki]

On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > >
> > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > introduces a workqueue to release the consumer and supplier devices used
> > > > in the devlink.
> > > > In the job queued, devices are release and in turn, when all the
> > > > references to these devices are dropped, the release function of the
> > > > device itself is called.
> > > >
> > > > Nothing is present to provide some synchronisation with this workqueue
> > > > in order to ensure that all ongoing releasing operations are done and
> > > > so, some other operations can be started safely.
> > > >
> > > > For instance, in the following sequence:
> > > >   1) of_platform_depopulate()
> > > >   2) of_overlay_remove()
> > > >
> > > > During the step 1, devices are released and related devlinks are removed
> > > > (jobs pushed in the workqueue).
> > > > During the step 2, OF nodes are destroyed but, without any
> > > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > > warnings related to missing of_node_put():
> > > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > >
> > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > from the workqueue job execution.
> > > >
> > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > operations waiting for the end of devlink removals (i.e. end of
> > > > workqueue jobs).
> > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > used is moved from a system-wide workqueue to a local one.
> > > >
> > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > ---
> > >
> > > With the below addressed:
> > >
> > > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > >
> > > >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> > > >  include/linux/device.h |  1 +
> > > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > --- a/drivers/base/core.c
> > > > +++ b/drivers/base/core.c
> > > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > > >  static void __fw_devlink_link_to_consumers(struct device *dev);
> > > >  static bool fw_devlink_drv_reg_done;
> > > >  static bool fw_devlink_best_effort;
> > > > +static struct workqueue_struct *device_link_wq;
> > > >
> > > >  /**
> > > >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device *dev)
> > > >       /*
> > > >        * It may take a while to complete this work because of the SRCU
> > > >        * 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.
> > > > +      * supplier devices get deleted when it runs, so put it into the
> > > > +      * dedicated workqueue.
> > > >        */
> > > > -     queue_work(system_long_wq, &link->rm_work);
> > > > +     queue_work(device_link_wq, &link->rm_work);
> > > >  }
> > > >
> > > > +/**
> > > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs to
> > > > terminate
> > > > + */
> > > > +void device_link_wait_removal(void)
> > > > +{
> > > > +     /*
> > > > +      * devlink removal jobs are queued in the dedicated work queue.
> > > > +      * To be sure that all removal jobs are terminated, ensure that any
> > > > +      * scheduled work has run to completion.
> > > > +      */
> > > > +     flush_workqueue(device_link_wq);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > +
> > > >  static struct class devlink_class = {
> > > >       .name = "devlink",
> > > >       .dev_groups = devlink_groups,
> > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > >       sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > >       if (!sysfs_dev_char_kobj)
> > > >               goto char_kobj_err;
> > > > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > +     if (!device_link_wq)
> > > > +             goto wq_err;
> > > >
> > >
> > > I can't still agree with this. Why not doing it in devlink_class_init()?
> > > This is
> > > devlink specific so it makes complete sense to me.
> >
> > If you do that in devlink_class_init() and it fails, you essentially
> > cause the creation of every device link to fail.  IOW, you try to live
> > without device links and pretend that it is all OK.  That won't get
> > you very far, especially on systems where DT is used.
> >
> > Doing it here, if it fails, you prevent the driver model from working
> > at all (because one of its necessary components is unavailable), which
> > arguably is a better choice.
>
> That makes sense but then the only thing I still don't fully get is why we have
> a separate devlink_class_init() initcall for registering the devlink class
> (which can also fail)...

Well, I haven't added it. :-)

> What I take from the above is that we should fail the
> driver model if one of it's fundamental components fails so I would say we
> should merge devlink_class_init() with device_init() otherwise it's a bit
> confusing (at least to me) and gives the idea that it's ok for the driver model
> to exist without the links (unless I'm missing some other reason for the devlink
> init function).

+1

Feel free to send a patch along these lines, chances are that it will
be popular. ;-)

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Nuno Sá]

On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > 
> > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > introduces a workqueue to release the consumer and supplier devices
> > > > > used
> > > > > in the devlink.
> > > > > In the job queued, devices are release and in turn, when all the
> > > > > references to these devices are dropped, the release function of the
> > > > > device itself is called.
> > > > > 
> > > > > Nothing is present to provide some synchronisation with this workqueue
> > > > > in order to ensure that all ongoing releasing operations are done and
> > > > > so, some other operations can be started safely.
> > > > > 
> > > > > For instance, in the following sequence:
> > > > >   1) of_platform_depopulate()
> > > > >   2) of_overlay_remove()
> > > > > 
> > > > > During the step 1, devices are released and related devlinks are
> > > > > removed
> > > > > (jobs pushed in the workqueue).
> > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > raise
> > > > > warnings related to missing of_node_put():
> > > > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > > > 
> > > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > > from the workqueue job execution.
> > > > > 
> > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > workqueue jobs).
> > > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > > used is moved from a system-wide workqueue to a local one.
> > > > > 
> > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > > ---
> > > > 
> > > > With the below addressed:
> > > > 
> > > > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > > > 
> > > > >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> > > > >  include/linux/device.h |  1 +
> > > > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > --- a/drivers/base/core.c
> > > > > +++ b/drivers/base/core.c
> > > > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > > > >  static void __fw_devlink_link_to_consumers(struct device *dev);
> > > > >  static bool fw_devlink_drv_reg_done;
> > > > >  static bool fw_devlink_best_effort;
> > > > > +static struct workqueue_struct *device_link_wq;
> > > > > 
> > > > >  /**
> > > > >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device
> > > > > *dev)
> > > > >       /*
> > > > >        * It may take a while to complete this work because of the SRCU
> > > > >        * 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.
> > > > > +      * supplier devices get deleted when it runs, so put it into the
> > > > > +      * dedicated workqueue.
> > > > >        */
> > > > > -     queue_work(system_long_wq, &link->rm_work);
> > > > > +     queue_work(device_link_wq, &link->rm_work);
> > > > >  }
> > > > > 
> > > > > +/**
> > > > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs
> > > > > to
> > > > > terminate
> > > > > + */
> > > > > +void device_link_wait_removal(void)
> > > > > +{
> > > > > +     /*
> > > > > +      * devlink removal jobs are queued in the dedicated work queue.
> > > > > +      * To be sure that all removal jobs are terminated, ensure that
> > > > > any
> > > > > +      * scheduled work has run to completion.
> > > > > +      */
> > > > > +     flush_workqueue(device_link_wq);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > +
> > > > >  static struct class devlink_class = {
> > > > >       .name = "devlink",
> > > > >       .dev_groups = devlink_groups,
> > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > >       sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > > >       if (!sysfs_dev_char_kobj)
> > > > >               goto char_kobj_err;
> > > > > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > +     if (!device_link_wq)
> > > > > +             goto wq_err;
> > > > > 
> > > > 
> > > > I can't still agree with this. Why not doing it in devlink_class_init()?
> > > > This is
> > > > devlink specific so it makes complete sense to me.
> > > 
> > > If you do that in devlink_class_init() and it fails, you essentially
> > > cause the creation of every device link to fail.  IOW, you try to live
> > > without device links and pretend that it is all OK.  That won't get
> > > you very far, especially on systems where DT is used.
> > > 
> > > Doing it here, if it fails, you prevent the driver model from working
> > > at all (because one of its necessary components is unavailable), which
> > > arguably is a better choice.
> > 
> > That makes sense but then the only thing I still don't fully get is why we
> > have
> > a separate devlink_class_init() initcall for registering the devlink class
> > (which can also fail)...
> 
> Well, I haven't added it. :-)
> 
> > What I take from the above is that we should fail the
> > driver model if one of it's fundamental components fails so I would say we
> > should merge devlink_class_init() with device_init() otherwise it's a bit
> > confusing (at least to me) and gives the idea that it's ok for the driver
> > model
> > to exist without the links (unless I'm missing some other reason for the
> > devlink
> > init function).
> 
> +1
> 
> Feel free to send a patch along these lines, chances are that it will
> be popular. ;-)

I was actually thinking about that but I think I encountered the reason why we
have it like this... devices_init() is called from driver_init() and there we
have:

...

devices_init();
buses_init();
classes_init();

...

So classes are initialized after devices which means we can't really do
class_register(&devlink_class) from devices_init(). Unless, of course, we re-
order things in driver_init() but that would be a questionable change at the
very least.

So, while I agree with what you've said, I'm still not sure if mixing devlink
stuff between devices_init() and devlink_class_init() is the best thing to do
given that we already have the case where devlink_class_init() can fail while
the driver model is up.

- Nuno Sá

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Rafael J. Wysocki]

On Wed, Mar 6, 2024 at 3:08 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> > On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > >
> > > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > >
> > > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > introduces a workqueue to release the consumer and supplier devices
> > > > > > used
> > > > > > in the devlink.
> > > > > > In the job queued, devices are release and in turn, when all the
> > > > > > references to these devices are dropped, the release function of the
> > > > > > device itself is called.
> > > > > >
> > > > > > Nothing is present to provide some synchronisation with this workqueue
> > > > > > in order to ensure that all ongoing releasing operations are done and
> > > > > > so, some other operations can be started safely.
> > > > > >
> > > > > > For instance, in the following sequence:
> > > > > >   1) of_platform_depopulate()
> > > > > >   2) of_overlay_remove()
> > > > > >
> > > > > > During the step 1, devices are released and related devlinks are
> > > > > > removed
> > > > > > (jobs pushed in the workqueue).
> > > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > > raise
> > > > > > warnings related to missing of_node_put():
> > > > > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > > > >
> > > > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > > > from the workqueue job execution.
> > > > > >
> > > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > > workqueue jobs).
> > > > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > > > used is moved from a system-wide workqueue to a local one.
> > > > > >
> > > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > > > ---
> > > > >
> > > > > With the below addressed:
> > > > >
> > > > > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > > > >
> > > > > >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> > > > > >  include/linux/device.h |  1 +
> > > > > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > > --- a/drivers/base/core.c
> > > > > > +++ b/drivers/base/core.c
> > > > > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > > > > >  static void __fw_devlink_link_to_consumers(struct device *dev);
> > > > > >  static bool fw_devlink_drv_reg_done;
> > > > > >  static bool fw_devlink_best_effort;
> > > > > > +static struct workqueue_struct *device_link_wq;
> > > > > >
> > > > > >  /**
> > > > > >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct device
> > > > > > *dev)
> > > > > >       /*
> > > > > >        * It may take a while to complete this work because of the SRCU
> > > > > >        * 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.
> > > > > > +      * supplier devices get deleted when it runs, so put it into the
> > > > > > +      * dedicated workqueue.
> > > > > >        */
> > > > > > -     queue_work(system_long_wq, &link->rm_work);
> > > > > > +     queue_work(device_link_wq, &link->rm_work);
> > > > > >  }
> > > > > >
> > > > > > +/**
> > > > > > + * device_link_wait_removal - Wait for ongoing devlink removal jobs
> > > > > > to
> > > > > > terminate
> > > > > > + */
> > > > > > +void device_link_wait_removal(void)
> > > > > > +{
> > > > > > +     /*
> > > > > > +      * devlink removal jobs are queued in the dedicated work queue.
> > > > > > +      * To be sure that all removal jobs are terminated, ensure that
> > > > > > any
> > > > > > +      * scheduled work has run to completion.
> > > > > > +      */
> > > > > > +     flush_workqueue(device_link_wq);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > > +
> > > > > >  static struct class devlink_class = {
> > > > > >       .name = "devlink",
> > > > > >       .dev_groups = devlink_groups,
> > > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > >       sysfs_dev_char_kobj = kobject_create_and_add("char", dev_kobj);
> > > > > >       if (!sysfs_dev_char_kobj)
> > > > > >               goto char_kobj_err;
> > > > > > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > > +     if (!device_link_wq)
> > > > > > +             goto wq_err;
> > > > > >
> > > > >
> > > > > I can't still agree with this. Why not doing it in devlink_class_init()?
> > > > > This is
> > > > > devlink specific so it makes complete sense to me.
> > > >
> > > > If you do that in devlink_class_init() and it fails, you essentially
> > > > cause the creation of every device link to fail.  IOW, you try to live
> > > > without device links and pretend that it is all OK.  That won't get
> > > > you very far, especially on systems where DT is used.
> > > >
> > > > Doing it here, if it fails, you prevent the driver model from working
> > > > at all (because one of its necessary components is unavailable), which
> > > > arguably is a better choice.
> > >
> > > That makes sense but then the only thing I still don't fully get is why we
> > > have
> > > a separate devlink_class_init() initcall for registering the devlink class
> > > (which can also fail)...
> >
> > Well, I haven't added it. :-)
> >
> > > What I take from the above is that we should fail the
> > > driver model if one of it's fundamental components fails so I would say we
> > > should merge devlink_class_init() with device_init() otherwise it's a bit
> > > confusing (at least to me) and gives the idea that it's ok for the driver
> > > model
> > > to exist without the links (unless I'm missing some other reason for the
> > > devlink
> > > init function).
> >
> > +1
> >
> > Feel free to send a patch along these lines, chances are that it will
> > be popular. ;-)
>
> I was actually thinking about that but I think I encountered the reason why we
> have it like this... devices_init() is called from driver_init() and there we
> have:
>
> ...
>
> devices_init();
> buses_init();
> classes_init();
>
> ...
>
> So classes are initialized after devices which means we can't really do
> class_register(&devlink_class) from devices_init(). Unless, of course, we re-
> order things in driver_init() but that would be a questionable change at the
> very least.
>
> So, while I agree with what you've said, I'm still not sure if mixing devlink
> stuff between devices_init() and devlink_class_init() is the best thing to do
> given that we already have the case where devlink_class_init() can fail while
> the driver model is up.

So why don't you make devlink_class_init() do a BUG() on failure
instead of returning an error?  IMO crashing early is better than
crashing later or otherwise failing in a subtle way due to a missed
dependency.

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Nuno Sá]

On Wed, 2024-03-06 at 15:37 +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 6, 2024 at 3:08 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> > > On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > 
> > > > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > > 
> > > > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > introduces a workqueue to release the consumer and supplier
> > > > > > > devices
> > > > > > > used
> > > > > > > in the devlink.
> > > > > > > In the job queued, devices are release and in turn, when all the
> > > > > > > references to these devices are dropped, the release function of
> > > > > > > the
> > > > > > > device itself is called.
> > > > > > > 
> > > > > > > Nothing is present to provide some synchronisation with this
> > > > > > > workqueue
> > > > > > > in order to ensure that all ongoing releasing operations are done
> > > > > > > and
> > > > > > > so, some other operations can be started safely.
> > > > > > > 
> > > > > > > For instance, in the following sequence:
> > > > > > >   1) of_platform_depopulate()
> > > > > > >   2) of_overlay_remove()
> > > > > > > 
> > > > > > > During the step 1, devices are released and related devlinks are
> > > > > > > removed
> > > > > > > (jobs pushed in the workqueue).
> > > > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > > > raise
> > > > > > > warnings related to missing of_node_put():
> > > > > > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > > > > > 
> > > > > > > Indeed, the missing of_node_put() call is going to be done, too
> > > > > > > late,
> > > > > > > from the workqueue job execution.
> > > > > > > 
> > > > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > > > workqueue jobs).
> > > > > > > Also, as a flushing operation is done on the workqueue, the
> > > > > > > workqueue
> > > > > > > used is moved from a system-wide workqueue to a local one.
> > > > > > > 
> > > > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > > > > ---
> > > > > > 
> > > > > > With the below addressed:
> > > > > > 
> > > > > > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > > > > > 
> > > > > > >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> > > > > > >  include/linux/device.h |  1 +
> > > > > > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > > > --- a/drivers/base/core.c
> > > > > > > +++ b/drivers/base/core.c
> > > > > > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > > > > > >  static void __fw_devlink_link_to_consumers(struct device *dev);
> > > > > > >  static bool fw_devlink_drv_reg_done;
> > > > > > >  static bool fw_devlink_best_effort;
> > > > > > > +static struct workqueue_struct *device_link_wq;
> > > > > > > 
> > > > > > >  /**
> > > > > > >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > > > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct
> > > > > > > device
> > > > > > > *dev)
> > > > > > >       /*
> > > > > > >        * It may take a while to complete this work because of the
> > > > > > > SRCU
> > > > > > >        * 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.
> > > > > > > +      * supplier devices get deleted when it runs, so put it into
> > > > > > > the
> > > > > > > +      * dedicated workqueue.
> > > > > > >        */
> > > > > > > -     queue_work(system_long_wq, &link->rm_work);
> > > > > > > +     queue_work(device_link_wq, &link->rm_work);
> > > > > > >  }
> > > > > > > 
> > > > > > > +/**
> > > > > > > + * device_link_wait_removal - Wait for ongoing devlink removal
> > > > > > > jobs
> > > > > > > to
> > > > > > > terminate
> > > > > > > + */
> > > > > > > +void device_link_wait_removal(void)
> > > > > > > +{
> > > > > > > +     /*
> > > > > > > +      * devlink removal jobs are queued in the dedicated work
> > > > > > > queue.
> > > > > > > +      * To be sure that all removal jobs are terminated, ensure
> > > > > > > that
> > > > > > > any
> > > > > > > +      * scheduled work has run to completion.
> > > > > > > +      */
> > > > > > > +     flush_workqueue(device_link_wq);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > > > +
> > > > > > >  static struct class devlink_class = {
> > > > > > >       .name = "devlink",
> > > > > > >       .dev_groups = devlink_groups,
> > > > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > > >       sysfs_dev_char_kobj = kobject_create_and_add("char",
> > > > > > > dev_kobj);
> > > > > > >       if (!sysfs_dev_char_kobj)
> > > > > > >               goto char_kobj_err;
> > > > > > > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > > > +     if (!device_link_wq)
> > > > > > > +             goto wq_err;
> > > > > > > 
> > > > > > 
> > > > > > I can't still agree with this. Why not doing it in
> > > > > > devlink_class_init()?
> > > > > > This is
> > > > > > devlink specific so it makes complete sense to me.
> > > > > 
> > > > > If you do that in devlink_class_init() and it fails, you essentially
> > > > > cause the creation of every device link to fail.  IOW, you try to live
> > > > > without device links and pretend that it is all OK.  That won't get
> > > > > you very far, especially on systems where DT is used.
> > > > > 
> > > > > Doing it here, if it fails, you prevent the driver model from working
> > > > > at all (because one of its necessary components is unavailable), which
> > > > > arguably is a better choice.
> > > > 
> > > > That makes sense but then the only thing I still don't fully get is why
> > > > we
> > > > have
> > > > a separate devlink_class_init() initcall for registering the devlink
> > > > class
> > > > (which can also fail)...
> > > 
> > > Well, I haven't added it. :-)
> > > 
> > > > What I take from the above is that we should fail the
> > > > driver model if one of it's fundamental components fails so I would say
> > > > we
> > > > should merge devlink_class_init() with device_init() otherwise it's a
> > > > bit
> > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > driver
> > > > model
> > > > to exist without the links (unless I'm missing some other reason for the
> > > > devlink
> > > > init function).
> > > 
> > > +1
> > > 
> > > Feel free to send a patch along these lines, chances are that it will
> > > be popular. ;-)
> > 
> > I was actually thinking about that but I think I encountered the reason why
> > we
> > have it like this... devices_init() is called from driver_init() and there
> > we
> > have:
> > 
> > ...
> > 
> > devices_init();
> > buses_init();
> > classes_init();
> > 
> > ...
> > 
> > So classes are initialized after devices which means we can't really do
> > class_register(&devlink_class) from devices_init(). Unless, of course, we
> > re-
> > order things in driver_init() but that would be a questionable change at the
> > very least.
> > 
> > So, while I agree with what you've said, I'm still not sure if mixing
> > devlink
> > stuff between devices_init() and devlink_class_init() is the best thing to
> > do
> > given that we already have the case where devlink_class_init() can fail
> > while
> > the driver model is up.
> 
> So why don't you make devlink_class_init() do a BUG() on failure
> instead of returning an error?  IMO crashing early is better than
> crashing later or otherwise failing in a subtle way due to a missed
> dependency.

Well, I do agree with that... Maybe that's something that Herve can sneak in
this patch? Otherwise, I can later (after this one is applied) send a patch for
it.

- Nuno Sá

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Herve Codina]

Hi Nuno,

On Wed, 06 Mar 2024 15:50:44 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

...
> > > > > 
> > > > > That makes sense but then the only thing I still don't fully get is why
> > > > > we
> > > > > have
> > > > > a separate devlink_class_init() initcall for registering the devlink
> > > > > class
> > > > > (which can also fail)...  
> > > > 
> > > > Well, I haven't added it. :-)
> > > >   
> > > > > What I take from the above is that we should fail the
> > > > > driver model if one of it's fundamental components fails so I would say
> > > > > we
> > > > > should merge devlink_class_init() with device_init() otherwise it's a
> > > > > bit
> > > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > > driver
> > > > > model
> > > > > to exist without the links (unless I'm missing some other reason for the
> > > > > devlink
> > > > > init function).  
> > > > 
> > > > +1
> > > > 
> > > > Feel free to send a patch along these lines, chances are that it will
> > > > be popular. ;-)  
> > > 
> > > I was actually thinking about that but I think I encountered the reason why
> > > we
> > > have it like this... devices_init() is called from driver_init() and there
> > > we
> > > have:
> > > 
> > > ...
> > > 
> > > devices_init();
> > > buses_init();
> > > classes_init();
> > > 
> > > ...
> > > 
> > > So classes are initialized after devices which means we can't really do
> > > class_register(&devlink_class) from devices_init(). Unless, of course, we
> > > re-
> > > order things in driver_init() but that would be a questionable change at the
> > > very least.
> > > 
> > > So, while I agree with what you've said, I'm still not sure if mixing
> > > devlink
> > > stuff between devices_init() and devlink_class_init() is the best thing to
> > > do
> > > given that we already have the case where devlink_class_init() can fail
> > > while
> > > the driver model is up.  
> > 
> > So why don't you make devlink_class_init() do a BUG() on failure
> > instead of returning an error?  IMO crashing early is better than
> > crashing later or otherwise failing in a subtle way due to a missed
> > dependency.  
> 
> Well, I do agree with that... Maybe that's something that Herve can sneak in
> this patch? Otherwise, I can later (after this one is applied) send a patch for
> it.

Well, I don't thing that this have to be part of this current series.
It is an other topic and should be handled out of this current series.

Hervé

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Nuno Sá]

On Wed, 2024-03-06 at 16:01 +0100, Herve Codina wrote:
> Hi Nuno,
> 
> On Wed, 06 Mar 2024 15:50:44 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> ...
> > > > > > 
> > > > > > That makes sense but then the only thing I still don't fully get is
> > > > > > why
> > > > > > we
> > > > > > have
> > > > > > a separate devlink_class_init() initcall for registering the devlink
> > > > > > class
> > > > > > (which can also fail)...  
> > > > > 
> > > > > Well, I haven't added it. :-)
> > > > >   
> > > > > > What I take from the above is that we should fail the
> > > > > > driver model if one of it's fundamental components fails so I would
> > > > > > say
> > > > > > we
> > > > > > should merge devlink_class_init() with device_init() otherwise it's
> > > > > > a
> > > > > > bit
> > > > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > > > driver
> > > > > > model
> > > > > > to exist without the links (unless I'm missing some other reason for
> > > > > > the
> > > > > > devlink
> > > > > > init function).  
> > > > > 
> > > > > +1
> > > > > 
> > > > > Feel free to send a patch along these lines, chances are that it will
> > > > > be popular. ;-)  
> > > > 
> > > > I was actually thinking about that but I think I encountered the reason
> > > > why
> > > > we
> > > > have it like this... devices_init() is called from driver_init() and
> > > > there
> > > > we
> > > > have:
> > > > 
> > > > ...
> > > > 
> > > > devices_init();
> > > > buses_init();
> > > > classes_init();
> > > > 
> > > > ...
> > > > 
> > > > So classes are initialized after devices which means we can't really do
> > > > class_register(&devlink_class) from devices_init(). Unless, of course,
> > > > we
> > > > re-
> > > > order things in driver_init() but that would be a questionable change at
> > > > the
> > > > very least.
> > > > 
> > > > So, while I agree with what you've said, I'm still not sure if mixing
> > > > devlink
> > > > stuff between devices_init() and devlink_class_init() is the best thing
> > > > to
> > > > do
> > > > given that we already have the case where devlink_class_init() can fail
> > > > while
> > > > the driver model is up.  
> > > 
> > > So why don't you make devlink_class_init() do a BUG() on failure
> > > instead of returning an error?  IMO crashing early is better than
> > > crashing later or otherwise failing in a subtle way due to a missed
> > > dependency.  
> > 
> > Well, I do agree with that... Maybe that's something that Herve can sneak in
> > this patch? Otherwise, I can later (after this one is applied) send a patch
> > for
> > it.
> 
> Well, I don't thing that this have to be part of this current series.
> It is an other topic and should be handled out of this current series.
> 

Yeah, fair enough... IMHO, then I would say that we should still have the
workqueue moved to devlink_class_init() and I can then follow up with a patch to
do BUG_ON(ret) in it.

Alternatively I can also just move it in the follow up patch but I don't think
it makes much sense.

- Nuno Sá

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Herve Codina]

Hi Rafael,

On Wed, 6 Mar 2024 13:48:37 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > introduces a workqueue to release the consumer and supplier devices used
> > in the devlink.
> > In the job queued, devices are release and in turn, when all the
> > references to these devices are dropped, the release function of the
> > device itself is called.
> >
> > Nothing is present to provide some synchronisation with this workqueue
> > in order to ensure that all ongoing releasing operations are done and
> > so, some other operations can be started safely.
> >
> > For instance, in the following sequence:
> >   1) of_platform_depopulate()
> >   2) of_overlay_remove()
> >
> > During the step 1, devices are released and related devlinks are removed
> > (jobs pushed in the workqueue).
> > During the step 2, OF nodes are destroyed but, without any
> > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > warnings related to missing of_node_put():
> >   ERROR: memory leak, expected refcount 1 instead of 2
> >
> > Indeed, the missing of_node_put() call is going to be done, too late,
> > from the workqueue job execution.
> >
> > Introduce device_link_wait_removal() to offer a way to synchronize
> > operations waiting for the end of devlink removals (i.e. end of
> > workqueue jobs).
> > Also, as a flushing operation is done on the workqueue, the workqueue
> > used is moved from a system-wide workqueue to a local one.
> >
> > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")  
> 
> No, it is not fixed by this patch.

Was explicitly asked by Saravana on v1 review:
https://lore.kernel.org/linux-kernel/CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com/

The commit 80dd33cf72d1 introduces the workqueue and so some asynchronous tasks
on removal.
This patch and the next one allows to re-sync execution waiting for jobs in
the workqueue when it is needed.

> 
> In fact, the only possibly observable effect of this patch is the
> failure when the allocation of device_link_wq fails AFAICS.
> 
> > Cc: stable@vger.kernel.org  
> 
> So why?

Cc:stable is needed as this patch is a prerequisite of patch 2 (needed
to fix the asynchronous workqueue task issue).

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Rafael J. Wysocki]

On Wed, Mar 6, 2024 at 4:24 PM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Rafael,
>
> On Wed, 6 Mar 2024 13:48:37 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > introduces a workqueue to release the consumer and supplier devices used
> > > in the devlink.
> > > In the job queued, devices are release and in turn, when all the
> > > references to these devices are dropped, the release function of the
> > > device itself is called.
> > >
> > > Nothing is present to provide some synchronisation with this workqueue
> > > in order to ensure that all ongoing releasing operations are done and
> > > so, some other operations can be started safely.
> > >
> > > For instance, in the following sequence:
> > >   1) of_platform_depopulate()
> > >   2) of_overlay_remove()
> > >
> > > During the step 1, devices are released and related devlinks are removed
> > > (jobs pushed in the workqueue).
> > > During the step 2, OF nodes are destroyed but, without any
> > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > warnings related to missing of_node_put():
> > >   ERROR: memory leak, expected refcount 1 instead of 2
> > >
> > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > from the workqueue job execution.
> > >
> > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > operations waiting for the end of devlink removals (i.e. end of
> > > workqueue jobs).
> > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > used is moved from a system-wide workqueue to a local one.
> > >
> > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> >
> > No, it is not fixed by this patch.
>
> Was explicitly asked by Saravana on v1 review:
> https://lore.kernel.org/linux-kernel/CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com/

Well, I don't think this is a valid request, sorry.

> The commit 80dd33cf72d1 introduces the workqueue and so some asynchronous tasks
> on removal.
> This patch and the next one allows to re-sync execution waiting for jobs in
> the workqueue when it is needed.

I get this, but still, this particular individual patch by itself
doesn't fix anything.  Do you agree with this?

If somebody applies this patch without the next one in the series,
they will not get any change in behavior, so the tag is at least
misleading.

You can claim that the next patch on top of this one fixes things, so
adding a Fixes tag to the other patch would be fine.

There is an explicit dependency between them (the second patch is not
even applicable without the first one, or if it is, the resulting code
won't compile anyway), but you can make a note to the maintainer that
they need to go to -stable together.

> >
> > In fact, the only possibly observable effect of this patch is the
> > failure when the allocation of device_link_wq fails AFAICS.
> >
> > > Cc: stable@vger.kernel.org
> >
> > So why?
>
> Cc:stable is needed as this patch is a prerequisite of patch 2 (needed
> to fix the asynchronous workqueue task issue).

Dependencies like this can be expressed in "Cc: stable" tags.
Personally, I'd do it like this:

Cc: stable@vger.kernel.org # 5.13: Depends on the first patch in the series

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Saravana Kannan]

On Wed, Mar 6, 2024 at 7:56 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Mar 6, 2024 at 4:24 PM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Rafael,
> >
> > On Wed, 6 Mar 2024 13:48:37 +0100
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > > On Wed, Mar 6, 2024 at 9:51 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > > >
> > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > introduces a workqueue to release the consumer and supplier devices used
> > > > in the devlink.
> > > > In the job queued, devices are release and in turn, when all the
> > > > references to these devices are dropped, the release function of the
> > > > device itself is called.
> > > >
> > > > Nothing is present to provide some synchronisation with this workqueue
> > > > in order to ensure that all ongoing releasing operations are done and
> > > > so, some other operations can be started safely.
> > > >
> > > > For instance, in the following sequence:
> > > >   1) of_platform_depopulate()
> > > >   2) of_overlay_remove()
> > > >
> > > > During the step 1, devices are released and related devlinks are removed
> > > > (jobs pushed in the workqueue).
> > > > During the step 2, OF nodes are destroyed but, without any
> > > > synchronisation with devlink removal jobs, of_overlay_remove() can raise
> > > > warnings related to missing of_node_put():
> > > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > >
> > > > Indeed, the missing of_node_put() call is going to be done, too late,
> > > > from the workqueue job execution.
> > > >
> > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > operations waiting for the end of devlink removals (i.e. end of
> > > > workqueue jobs).
> > > > Also, as a flushing operation is done on the workqueue, the workqueue
> > > > used is moved from a system-wide workqueue to a local one.
> > > >
> > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > >
> > > No, it is not fixed by this patch.
> >
> > Was explicitly asked by Saravana on v1 review:
> > https://lore.kernel.org/linux-kernel/CAGETcx9uP86EHyKJNifBMd23oCsA+KpMa+e36wJEEnHDve+Avg@mail.gmail.com/
>
> Well, I don't think this is a valid request, sorry.
>
> > The commit 80dd33cf72d1 introduces the workqueue and so some asynchronous tasks
> > on removal.
> > This patch and the next one allows to re-sync execution waiting for jobs in
> > the workqueue when it is needed.
>
> I get this, but still, this particular individual patch by itself
> doesn't fix anything.  Do you agree with this?
>
> If somebody applies this patch without the next one in the series,
> they will not get any change in behavior, so the tag is at least
> misleading.
>
> You can claim that the next patch on top of this one fixes things, so
> adding a Fixes tag to the other patch would be fine.
>
> There is an explicit dependency between them (the second patch is not
> even applicable without the first one, or if it is, the resulting code
> won't compile anyway), but you can make a note to the maintainer that
> they need to go to -stable together.
>
> > >
> > > In fact, the only possibly observable effect of this patch is the
> > > failure when the allocation of device_link_wq fails AFAICS.
> > >
> > > > Cc: stable@vger.kernel.org
> > >
> > > So why?
> >
> > Cc:stable is needed as this patch is a prerequisite of patch 2 (needed
> > to fix the asynchronous workqueue task issue).
>
> Dependencies like this can be expressed in "Cc: stable" tags.
> Personally, I'd do it like this:
>
> Cc: stable@vger.kernel.org # 5.13: Depends on the first patch in the series

I'm okay with this too. I personally think it's better to list "Fixes:
xyz" in all the patches that are needed to fix xyz (especially when
there's no compile time dependency on earlier patches), but it's not a
hill I'll die on. And if Rafael's suggestion is the expected norm,
then I'll remember to follow that in the future.

-Saravana

Re: [PATCH v4 1/2] driver core: Introduce device_link_wait_removal()

[Saravana Kannan]

On Wed, Mar 6, 2024 at 6:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Wed, 2024-03-06 at 15:37 +0100, Rafael J. Wysocki wrote:
> > On Wed, Mar 6, 2024 at 3:08 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > >
> > > On Wed, 2024-03-06 at 14:05 +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Mar 6, 2024 at 2:01 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > >
> > > > > On Wed, 2024-03-06 at 13:43 +0100, Rafael J. Wysocki wrote:
> > > > > > On Wed, Mar 6, 2024 at 10:17 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, 2024-03-06 at 09:50 +0100, Herve Codina wrote:
> > > > > > > > The commit 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > > introduces a workqueue to release the consumer and supplier
> > > > > > > > devices
> > > > > > > > used
> > > > > > > > in the devlink.
> > > > > > > > In the job queued, devices are release and in turn, when all the
> > > > > > > > references to these devices are dropped, the release function of
> > > > > > > > the
> > > > > > > > device itself is called.
> > > > > > > >
> > > > > > > > Nothing is present to provide some synchronisation with this
> > > > > > > > workqueue
> > > > > > > > in order to ensure that all ongoing releasing operations are done
> > > > > > > > and
> > > > > > > > so, some other operations can be started safely.
> > > > > > > >
> > > > > > > > For instance, in the following sequence:
> > > > > > > >   1) of_platform_depopulate()
> > > > > > > >   2) of_overlay_remove()
> > > > > > > >
> > > > > > > > During the step 1, devices are released and related devlinks are
> > > > > > > > removed
> > > > > > > > (jobs pushed in the workqueue).
> > > > > > > > During the step 2, OF nodes are destroyed but, without any
> > > > > > > > synchronisation with devlink removal jobs, of_overlay_remove() can
> > > > > > > > raise
> > > > > > > > warnings related to missing of_node_put():
> > > > > > > >   ERROR: memory leak, expected refcount 1 instead of 2
> > > > > > > >
> > > > > > > > Indeed, the missing of_node_put() call is going to be done, too
> > > > > > > > late,
> > > > > > > > from the workqueue job execution.
> > > > > > > >
> > > > > > > > Introduce device_link_wait_removal() to offer a way to synchronize
> > > > > > > > operations waiting for the end of devlink removals (i.e. end of
> > > > > > > > workqueue jobs).
> > > > > > > > Also, as a flushing operation is done on the workqueue, the
> > > > > > > > workqueue
> > > > > > > > used is moved from a system-wide workqueue to a local one.
> > > > > > > >
> > > > > > > > Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > > > > > ---
> > > > > > >
> > > > > > > With the below addressed:
> > > > > > >
> > > > > > > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > > > > > >
> > > > > > > >  drivers/base/core.c    | 26 +++++++++++++++++++++++---
> > > > > > > >  include/linux/device.h |  1 +
> > > > > > > >  2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > > > > index d5f4e4aac09b..48b28c59c592 100644
> > > > > > > > --- a/drivers/base/core.c
> > > > > > > > +++ b/drivers/base/core.c
> > > > > > > > @@ -44,6 +44,7 @@ static bool fw_devlink_is_permissive(void);
> > > > > > > >  static void __fw_devlink_link_to_consumers(struct device *dev);
> > > > > > > >  static bool fw_devlink_drv_reg_done;
> > > > > > > >  static bool fw_devlink_best_effort;
> > > > > > > > +static struct workqueue_struct *device_link_wq;
> > > > > > > >
> > > > > > > >  /**
> > > > > > > >   * __fwnode_link_add - Create a link between two fwnode_handles.
> > > > > > > > @@ -532,12 +533,26 @@ static void devlink_dev_release(struct
> > > > > > > > device
> > > > > > > > *dev)
> > > > > > > >       /*
> > > > > > > >        * It may take a while to complete this work because of the
> > > > > > > > SRCU
> > > > > > > >        * 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.
> > > > > > > > +      * supplier devices get deleted when it runs, so put it into
> > > > > > > > the
> > > > > > > > +      * dedicated workqueue.
> > > > > > > >        */
> > > > > > > > -     queue_work(system_long_wq, &link->rm_work);
> > > > > > > > +     queue_work(device_link_wq, &link->rm_work);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * device_link_wait_removal - Wait for ongoing devlink removal
> > > > > > > > jobs
> > > > > > > > to
> > > > > > > > terminate
> > > > > > > > + */
> > > > > > > > +void device_link_wait_removal(void)
> > > > > > > > +{
> > > > > > > > +     /*
> > > > > > > > +      * devlink removal jobs are queued in the dedicated work
> > > > > > > > queue.
> > > > > > > > +      * To be sure that all removal jobs are terminated, ensure
> > > > > > > > that
> > > > > > > > any
> > > > > > > > +      * scheduled work has run to completion.
> > > > > > > > +      */
> > > > > > > > +     flush_workqueue(device_link_wq);
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL_GPL(device_link_wait_removal);
> > > > > > > > +
> > > > > > > >  static struct class devlink_class = {
> > > > > > > >       .name = "devlink",
> > > > > > > >       .dev_groups = devlink_groups,
> > > > > > > > @@ -4099,9 +4114,14 @@ int __init devices_init(void)
> > > > > > > >       sysfs_dev_char_kobj = kobject_create_and_add("char",
> > > > > > > > dev_kobj);
> > > > > > > >       if (!sysfs_dev_char_kobj)
> > > > > > > >               goto char_kobj_err;
> > > > > > > > +     device_link_wq = alloc_workqueue("device_link_wq", 0, 0);
> > > > > > > > +     if (!device_link_wq)
> > > > > > > > +             goto wq_err;
> > > > > > > >
> > > > > > >
> > > > > > > I can't still agree with this. Why not doing it in
> > > > > > > devlink_class_init()?
> > > > > > > This is
> > > > > > > devlink specific so it makes complete sense to me.
> > > > > >
> > > > > > If you do that in devlink_class_init() and it fails, you essentially
> > > > > > cause the creation of every device link to fail.  IOW, you try to live
> > > > > > without device links and pretend that it is all OK.  That won't get
> > > > > > you very far, especially on systems where DT is used.
> > > > > >
> > > > > > Doing it here, if it fails, you prevent the driver model from working
> > > > > > at all (because one of its necessary components is unavailable), which
> > > > > > arguably is a better choice.
> > > > >
> > > > > That makes sense but then the only thing I still don't fully get is why
> > > > > we
> > > > > have
> > > > > a separate devlink_class_init() initcall for registering the devlink
> > > > > class
> > > > > (which can also fail)...
> > > >
> > > > Well, I haven't added it. :-)
> > > >
> > > > > What I take from the above is that we should fail the
> > > > > driver model if one of it's fundamental components fails so I would say
> > > > > we
> > > > > should merge devlink_class_init() with device_init() otherwise it's a
> > > > > bit
> > > > > confusing (at least to me) and gives the idea that it's ok for the
> > > > > driver
> > > > > model
> > > > > to exist without the links (unless I'm missing some other reason for the
> > > > > devlink
> > > > > init function).
> > > >
> > > > +1
> > > >
> > > > Feel free to send a patch along these lines, chances are that it will
> > > > be popular. ;-)
> > >
> > > I was actually thinking about that but I think I encountered the reason why
> > > we
> > > have it like this... devices_init() is called from driver_init() and there
> > > we
> > > have:
> > >
> > > ...
> > >
> > > devices_init();
> > > buses_init();
> > > classes_init();
> > >
> > > ...
> > >
> > > So classes are initialized after devices which means we can't really do
> > > class_register(&devlink_class) from devices_init(). Unless, of course, we
> > > re-
> > > order things in driver_init() but that would be a questionable change at the
> > > very least.
> > >
> > > So, while I agree with what you've said, I'm still not sure if mixing
> > > devlink
> > > stuff between devices_init() and devlink_class_init() is the best thing to
> > > do
> > > given that we already have the case where devlink_class_init() can fail
> > > while
> > > the driver model is up.
> >
> > So why don't you make devlink_class_init() do a BUG() on failure
> > instead of returning an error?  IMO crashing early is better than
> > crashing later or otherwise failing in a subtle way due to a missed
> > dependency.
>
> Well, I do agree with that... Maybe that's something that Herve can sneak in
> this patch? Otherwise, I can later (after this one is applied) send a patch for
> it.

I'll happily Ack the patch if you want to add a BUG(), but the way
it's written is still pedantically better than putting it in
devices_init(). All errors from devices_init() are ignored and not
even logged by the caller. At least any error from
devlink_class_init() would be logged if initcall_debug is enabled :-)

Oh, btw, I wrote devlink_class_init() as a separate initcall because
it's just another class like any other class that's being registered.

All that said, I think this whole discussion is a pedantic waste of time.

-Saravana

Re: [PATCH v4 2/2] of: dynamic: Synchronize of_changeset_destroy() with the devlink removals

[Saravana Kannan]

On Wed, Mar 6, 2024 at 12:51 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> In the following sequence:
>   1) of_platform_depopulate()
>   2) of_overlay_remove()
>
> During the step 1, devices are destroyed and devlinks are removed.
> During the step 2, OF nodes are destroyed but
> __of_changeset_entry_destroy() can raise warnings related to missing
> of_node_put():
>   ERROR: memory leak, expected refcount 1 instead of 2 ...
>
> Indeed, during the devlink removals performed at step 1, the removal
> itself releasing the device (and the attached of_node) is done by a job
> queued in a workqueue and so, it is done asynchronously with respect to
> function calls.
> When the warning is present, of_node_put() will be called but wrongly
> too late from the workqueue job.
>
> In order to be sure that any ongoing devlink removals are done before
> the of_node destruction, synchronize the of_changeset_destroy() with the
> devlink removals.
>
> Fixes: 80dd33cf72d1 ("drivers: base: Fix device link removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/of/dynamic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 3bf27052832f..169e2a9ae22f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -9,6 +9,7 @@
>
>  #define pr_fmt(fmt)    "OF: " fmt
>
> +#include <linux/device.h>
>  #include <linux/of.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -667,6 +668,12 @@ void of_changeset_destroy(struct of_changeset *ocs)
>  {
>         struct of_changeset_entry *ce, *cen;
>
> +       /*
> +        * Wait for any ongoing device link removals before destroying some of
> +        * nodes.
> +        */

Not going to ask you to revise this patch just for this, but this
comment isn't very useful. It's telling what you are doing. Not why.
And the function name is already clear on what you are doing.

Maybe something like this would be better at describing the "why"?
Free free to reword it.

When a device is deleted, the device links to/from it are also queued
for deletion. Until these device links are freed, the devices
themselves aren't freed. If the device being deleted is due to an
overlay change, this device might be holding a reference to a device
node that will be freed. So, wait until all already pending device
links are deleted before freeing a device node. This ensures we don't
free any device node that has a non-zero reference count.

Reviewed-by: Saravana Kannan <saravanak@google.com>

-Saravana

> +       device_link_wait_removal();
> +
>         list_for_each_entry_safe_reverse(ce, cen, &ocs->entries, node)
>                 __of_changeset_entry_destroy(ce);
>  }
> --
> 2.43.0
>