On 10/15/25 1:19 PM, [email protected] wrote:
> 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:
That works. I was tunnel visioned in trying to not mess up the current code
flow.
DJ>
> 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);
>
>
> ...?
