Dave Jiang wrote:
> The following lockdep splat was observed while kernel auto-online a CXL
> memory region:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.17.0djtest+ #53 Tainted: G W
> ------------------------------------------------------
> systemd-udevd/3334 is trying to acquire lock:
> ffffffff90346188 (hmem_resource_lock){+.+.}-{4:4}, at:
> hmem_register_resource+0x31/0x50
>
> but task is already holding lock:
> ffffffff90338890 ((node_chain).rwsem){++++}-{4:4}, at:
> blocking_notifier_call_chain+0x2e/0x70
>
> which lock already depends on the new lock.
> [..]
> Chain exists of:
> hmem_resource_lock --> mem_hotplug_lock --> (node_chain).rwsem
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> rlock((node_chain).rwsem);
> lock(mem_hotplug_lock);
> lock((node_chain).rwsem);
> lock(hmem_resource_lock);
>
> The lock ordering can cause potential deadlock. There are instances
> where hmem_resource_lock is taken after (node_chain).rwsem, and vice
> versa.
>
> Remove registering of target devices from the hmat_callback(). By the
> time the hmat hotplug notifier is being called, there should not be
> hmem targets that still need to be registered.
>
> Fixes: cf8741ac57ed ("ACPI: NUMA: HMAT: Register "soft reserved" memory as an
> "hmem" device")
> Link:
> https://lore.kernel.org/nvdimm/[email protected]/
> Suggested-by: Dan Williams <[email protected]>
> Signed-off-by: Dave Jiang <[email protected]>
> ---
> v2:
> - Drop target registering in hmat_callback instead. (Dan)
> ---
> drivers/acpi/numa/hmat.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 5a36d57289b4..5084ae1688f6 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -874,7 +874,8 @@ static void hmat_register_target_devices(struct
> memory_target *target)
> }
> }
>
> -static void hmat_register_target(struct memory_target *target)
> +static void hmat_register_target(struct memory_target *target,
> + bool register_devices)
> {
> int nid = pxm_to_node(target->memory_pxm);
>
> @@ -882,7 +883,8 @@ static void hmat_register_target(struct memory_target
> *target)
> * Devices may belong to either an offline or online
> * node, so unconditionally add them.
> */
> - hmat_register_target_devices(target);
> + if (register_devices)
> + hmat_register_target_devices(target);
Why a new flag to pass around and not something like:
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 5a36d57289b4..9f9f09480765 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -867,6 +867,9 @@ static void hmat_register_target_devices(struct
memory_target *target)
if (!IS_ENABLED(CONFIG_DEV_DAX_HMEM))
return;
+ if (target->registered)
+ return;
+
for (res = target->memregions.child; res; res = res->sibling) {
int target_nid = pxm_to_node(target->memory_pxm);
...?
