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) \
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á
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:
- of_platform_depopulate()
- 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.
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á
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:
- of_platform_depopulate()
- 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. ;-)
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á
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:
- of_platform_depopulate()
- 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 thatany
* 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.
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á
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é
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á
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
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:
- of_platform_depopulate()
- 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
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:
- of_platform_depopulate()
- 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) \
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:
- of_platform_depopulate()
- 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+e36w...
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é
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:
- of_platform_depopulate()
- 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+e36w...
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
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:
- of_platform_depopulate()
- 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+e36w...
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
linux-stable-mirror@lists.linaro.org
-
Herve Codina -
Luca Ceresoli -
Nuno Sá -
Rafael J. Wysocki -
Saravana Kannan