Re: [PATCH v4 6/7] dax/hmem: Save the DAX HMEM platform device pointer

[Nathan Fontenot <nathan.fontenot@amd.com>]

On 6/6/2025 3:11 AM, Zhijian Li (Fujitsu) wrote:
> 
> 
> On 06/06/2025 00:54, Dave Jiang wrote:
>>> -static int hmem_register_device(struct device *host, int target_nid,
>>> -				const struct resource *res)
>>> +static int hmem_register_device(int target_nid, const struct resource *res)
>>>   {
>>> +	struct device *host = &dax_hmem_pdev->dev;
>>>   	struct platform_device *pdev;
>>>   	struct memregion_info info;
>>>   	long id;
>>> @@ -125,7 +127,8 @@ static int hmem_register_device(struct device *host, int target_nid,
>>>   
>>>   static int dax_hmem_platform_probe(struct platform_device *pdev)
>>>   {
>>> -	return walk_hmem_resources(&pdev->dev, hmem_register_device);
>>> +	dax_hmem_pdev = pdev;
> 
>> Is there never more than 1 DAX HMEM platform device that can show up? The global pointer makes me nervous.
> 
> IIUC,
> 
> Referring to the device creation logic in `__hmem_register_resource()` (shown below),
> only one `hmem_platform` instance can ever be registered. This ensures the change is safe.
> 
> 
> However, I agree that using a global pointer in a function that may be called multiple times
> does raise valid concerns.

Note: The creation of the platform device is moved in patch 7/7. I think the placed
it was moved to may not be correct. The creation of the platform device should be
in hmem_init so that it is always create one instance.

-Nathan

> 
> To strengthen this, how about:
> 1. Add a comment clarifying single-instance enforcement
> 2. Add a warn_on/bug_on for it: `WARN_ON(dax_hmem_pdev && dax_hmem_pdev != pdev)`
> 
> 
> static void __hmem_register_resource(int target_nid, struct resource *res)
> {
> 	struct platform_device *pdev;
> 	struct resource *new;
> 	int rc;
> 
> 	new = __request_region(&hmem_active, res->start, resource_size(res), "",
> 			       0);
> 	if (!new) {
> 		pr_debug("hmem range %pr already active\n", res);
> 		return;
> 	}
> 
> 	new->desc = target_nid;
> 
> 	if (platform_initialized)
> 		return;
> 
> 	pdev = platform_device_alloc("hmem_platform", 0);
> 	if (!pdev) {
> 		pr_err_once("failed to register device-dax hmem_platform device\n");
> 		return;
> 	}
> 
> 	rc = platform_device_add(pdev);
> 	if (rc)
> 		platform_device_put(pdev);
> 	else
> 		platform_initialized = true;
> }
> 
> Thanks
> Zhijian
> 
> 
> 
> 
> 
>>
>> DJ