On Fri, 17 Oct 2025 14:21:05 -0700
Dave Jiang <[email protected]> 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.
>
> Split out the target update section of hmat_register_target() so that
> hmat_callback() only envokes that section instead of attempt to register
> hmem devices that it does not need to.
>
> Fixes: cf8741ac57ed ("ACPI: NUMA: HMAT: Register "soft reserved" memory as a
> n "hmem" device")
Fix up whatever caused that line to wrap!
> notmuch/
?
> Signed-off-by: Dave Jiang <[email protected]>
The guard change seems to be unrelated to rest of the patch.
Probably shouldn't be here.
>
> ---
> v3:
> - Refactor to split out target device setup vs target update (Dan)
> ---
> drivers/acpi/numa/hmat.c | 48 ++++++++++++++++++++++------------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 1dc73d20d989..ddbdd32e79a8 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -874,28 +874,10 @@ static void hmat_register_target_devices(struct
> memory_target *target)
> }
> }
>
> -static void hmat_register_target(struct memory_target *target)
> +static void hmat_hotplug_target(struct memory_target *target)
> {
> @@ -906,7 +888,7 @@ static void hmat_register_target(struct memory_target
> *target)
> if (nid == NUMA_NO_NODE || !node_online(nid))
> return;
>
> - mutex_lock(&target_lock);
> + guard(mutex)(&target_lock);
Smells unrelated...
If you did want to do this I'd also do
if (target->registered)
return;
hmat_register_target_initiators();
etc.
> if (!target->registered) {
> hmat_register_target_initiators(target);
> hmat_register_target_cache(target);
> @@ -914,7 +896,29 @@ static void hmat_register_target(struct memory_target
> *target)
> hmat_register_target_perf(target, ACCESS_COORDINATE_CPU);
> target->registered = true;
> }
> - mutex_unlock(&target_lock);
> +}
