[PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[Dave Jiang]

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/68e46a09c2a07_2980100f3@dwillia2-mobl4.notmuch/
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
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);
 
 	/*
 	 * Register generic port perf numbers. The nid may not be
@@ -921,7 +923,7 @@ static void hmat_register_targets(void)
 	struct memory_target *target;
 
 	list_for_each_entry(target, &targets, node)
-		hmat_register_target(target);
+		hmat_register_target(target, true);
 }
 
 static int hmat_callback(struct notifier_block *self,
@@ -939,7 +941,7 @@ static int hmat_callback(struct notifier_block *self,
 	if (!target)
 		return NOTIFY_OK;
 
-	hmat_register_target(target);
+	hmat_register_target(target, false);
 	return NOTIFY_OK;
 }
 

base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
-- 
2.51.0

Re: [PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[dan.j.williams]

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/68e46a09c2a07_2980100f3@dwillia2-mobl4.notmuch/
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> 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);
 

...?

Re: [PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[Dave Jiang]

On 10/15/25 1:19 PM, dan.j.williams@intel.com 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/68e46a09c2a07_2980100f3@dwillia2-mobl4.notmuch/
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> 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);
>  
> 
> ...?

Re: [PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[Dave Jiang]

On 10/15/25 1:19 PM, dan.j.williams@intel.com 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/68e46a09c2a07_2980100f3@dwillia2-mobl4.notmuch/
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> 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;
> +

So this still triggers the lockdep warning. I don't think it's smart enough to know that it gets around the issue. My changes with a new flag does not trigger the lockdep.

DJ

>         for (res = target->memregions.child; res; res = res->sibling) {
>                 int target_nid = pxm_to_node(target->memory_pxm);
>  
> 
> ...?

Re: [PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[dan.j.williams]

Dave Jiang wrote:
[..]
> > 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;
> > +
> 
> So this still triggers the lockdep warning. I don't think it's smart
> enough to know that it gets around the issue. My changes with a new
> flag does not trigger the lockdep.

You have a case where target->registered is false in the
hmat_callback() path? How does that happen?

...and the splat is the same hmem_resource lock entanglement?

Re: [PATCH v2] acpi/hmat: Fix lockdep warning for hmem_register_resource()

[Dave Jiang]

On 10/16/25 12:06 PM, dan.j.williams@intel.com wrote:
> Dave Jiang wrote:
> [..]
>>> 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;
>>> +
>>
>> So this still triggers the lockdep warning. I don't think it's smart
>> enough to know that it gets around the issue. My changes with a new
>> flag does not trigger the lockdep.
> 
> You have a case where target->registered is false in the
> hmat_callback() path? How does that happen?

It seems the ELC nodes are not on CPU node and also not generic port nodes. This platform has socket 0 and 1 where the memory targets are registered. However, node 2 and 3 gets hot-plugged and does not target registered at hmat_callback() time. > 
> ...and the splat is the same hmem_resource lock entanglement?

yes

Yes.