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/[email protected]/
this v6 series:
- Add Saravana's 'Reviewed-by' tag
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/[email protected]/
Best regards,
Hervé
Changes v5 -> v6
- Patch 1
Add 'Reviewed-by: Saravana Kannan <[email protected]>'
- Patch 2
No changes
Changes v4 -> v5
- Patch 1
Remove the 'Fixes' tag
Add 'Tested-by: Luca Ceresoli <[email protected]>'
Add 'Reviewed-by: Nuno Sa <[email protected]>'
- Patch 2
Update comment as suggested
Add 'Reviewed-by: Saravana Kannan <[email protected]>'
Add 'Tested-by: Luca Ceresoli <[email protected]>'
Add 'Reviewed-by: Nuno Sa <[email protected]>'
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 | 12 ++++++++++++
include/linux/device.h | 1 +
3 files changed, 36 insertions(+), 3 deletions(-)
--
2.44.0
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: [email protected]
Signed-off-by: Herve Codina <[email protected]>
Reviewed-by: Saravana Kannan <[email protected]>
Tested-by: Luca Ceresoli <[email protected]>
Reviewed-by: Nuno Sa <[email protected]>
---
drivers/of/dynamic.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 3bf27052832f..4d57a4e34105 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,17 @@ void of_changeset_destroy(struct of_changeset *ocs)
{
struct of_changeset_entry *ce, *cen;
+ /*
+ * 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.
+ */
+ device_link_wait_removal();
+
list_for_each_entry_safe_reverse(ce, cen, &ocs->entries, node)
__of_changeset_entry_destroy(ce);
}
--
2.44.0
On Mon, Mar 25, 2024 at 04:21:24PM +0100, Herve Codina wrote:
> 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/[email protected]/
> this v6 series:
> - Add Saravana's 'Reviewed-by' tag
>
> 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/[email protected]/
>
> Best regards,
> Herv?
>
> Changes v5 -> v6
> - Patch 1
> Add 'Reviewed-by: Saravana Kannan <[email protected]>'
>
> - Patch 2
> No changes
>
> Changes v4 -> v5
> - Patch 1
> Remove the 'Fixes' tag
> Add 'Tested-by: Luca Ceresoli <[email protected]>'
> Add 'Reviewed-by: Nuno Sa <[email protected]>'
>
> - Patch 2
> Update comment as suggested
> Add 'Reviewed-by: Saravana Kannan <[email protected]>'
> Add 'Tested-by: Luca Ceresoli <[email protected]>'
> Add 'Reviewed-by: Nuno Sa <[email protected]>'
>
> 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 | 12 ++++++++++++
> include/linux/device.h | 1 +
> 3 files changed, 36 insertions(+), 3 deletions(-)
Applied, thanks!