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

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

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.

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