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
