Re: [PATCH v2 3/4] dax: Update hmem resource/device registration

["Fontenot, Nathan" <nafonten@amd.com>]

On 1/23/2025 10:01 AM, Fontenot, Nathan wrote:
> On 1/21/2025 5:14 PM, Ira Weiny wrote:
>> Fontenot, Nathan wrote:
>>> On 1/16/2025 4:28 PM, Ira Weiny wrote:
>>>> Nathan Fontenot wrote:
>>>>> In order to handle registering hmem devices for SOFT RESERVE reources
>>>>                                                               ^^^^^^^^^
>>>>                                                               resources
>>>>
>>>>> that are added late in boot update the hmem_register_resource(),
>>>>> hmem_register_device(), and walk_hmem_resources() interfaces.
>>>>>
>>>>> Remove the target_nid arg to hmem_register_resource(). The target nid
>>>>> value is calculated from the resource start address and not used until
>>>>> registering a device for the resource. Move the target nid calculation
>>>>> to hmem_register_device().
>>>>>
>>>>> To allow for registering hmem devices outside of the hmem dax driver
>>>>> probe routine save the dax hmem platform driver during probe. The
>>>>> hmem_register_device() interface can then drop the host and target
>>>>> nid parameters.
>>>>>
>>>>> There should be no functional changes.
>>>>>
>>>>> Signed-off-by: Nathan Fontenot <nathan.fontenot@amd.com>

[ snip ]

>>>>> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
>>>>> index 5e7c53f18491..088f4060d4d5 100644
>>>>> --- a/drivers/dax/hmem/hmem.c
>>>>> +++ b/drivers/dax/hmem/hmem.c
>>>>> @@ -9,6 +9,8 @@
>>>>>  static bool region_idle;
>>>>>  module_param_named(region_idle, region_idle, bool, 0644);
>>>>>  
>>>>> +static struct platform_device *dax_hmem_pdev;
>>>>
>>>> I don't think you can assume there is only ever 1 hmem platform device.
>>>>
>>>> hmat_register_target_devices() in particular iterates multiple memory
>>>> regions and will create more than one.
>>>>
>>>> What am I missing?
>>>
>>> You may be correct that there can be more than one hmem platform device.
>>> I was making this change based on a comment from Dan that it may not matter
>>> which platform device these are created against.
>>
>> If that is true I think there should be a big comment around this code
>> explaining why it is ok to have the platform device being allocated in
>> this call unregistered when a different platform device (host) is
>> released.
>>
>> IOW hmem_register_device() calls two devm_*() functions using host as the
>> device used to trigger an action.  It is not entirely clear to me why that
>> change is safe here.
>>
>>>
>>> I could be wrong in that assumption. If so we'll need to figure lout how to
>>> determine which platform device a soft reserve resource would be created
>>> against when they are added later in boot from a notification by the
>>> srmem notification chain.
>>
>> I see that it would be more difficult to track.  And I'm ok if it really
>> does work.  But just looking at the commit message and code I don't see
>> how this does not at least introduce a functional change.
> 
> I'm going to go back and take a look at this again. I went this direction
> using the approach of having the srmem notification chain. The dax driver
> then adds soft reserves outside of a probe routine and don't have a
> platform device associated with them.
> 

Digging back into this, the dax driver only creates one platform device.
During hmem_register_resource (which is what hmat_register_target_devices()
calls for each resource) the dax hmem driver will only create a platform
device when the first resource is registered.

Each resource that is passed to hmem_register_resource() is added to a
resource tree internal to the dax/hmem driver that is eventually walked
by the dax hmem driver probe routine.

Now that I understand this better I am confident that saving a pointer
to the dax hmem platform device is safe. I'll include this information
in the commit log for the next version of the patch.

-Nathan