Re: [PATCH v7] driver core: platform: set numa_node before platform_device_add()

[Jinhui Guo]

On Tue, 19 Sep 2023 14:12:20 +0200, Greg KH wrote:
> On Tue, Sep 19, 2023 at 08:03:41PM +0800, Jinhui Guo wrote:
> > Setting the devices' numa_node needs to be done in
> > platform_device_register_full(), because that's where the
> > platform device object is allocated.
> > 
> > Fixes: 4a60406d3592 ("driver core: platform: expose numa_node to users in sysfs")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> 
> The test robot did not report the original problem, that was a problem
> with your potential change.
> 
> > Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@xxxxxxxxx/
> 
> Likewise, this is not a real issue, it was a problem with your previous
> submission.
>

Thanks, I will fix it.
 
> > Signed-off-by: Jinhui Guo <guojinhui.liam@xxxxxxxxxxxxx>
> > ---
> > V6 -> V7
> >   1. Fix bug directly by adding numa_node to struct
> >      platform_device_info (suggested by Rafael J. Wysocki).
> >   2. Remove reviewer name.
> > 
> > V5 -> V6:
> >   1. Update subject to correct function name platform_device_add().
> >   2. Provide a more clear and accurate description of the changes
> >      made in commit (suggested by Rafael J. Wysocki).
> >   3. Add reviewer name.
> > 
> > V4 -> V5:
> >   Add Cc: stable line and changes from the previous submited patches.
> > 
> > V3 -> V4:
> >   Refactor code to be an ACPI function call (suggested by Greg Kroah-Hartman).
> > 
> > V2 -> V3:
> >   Fix Signed-off name.
> > 
> > V1 -> V2:
> >   Fix compile error without enabling CONFIG_ACPI.
> > ---
> > 
> >  drivers/acpi/acpi_platform.c    |  5 ++---
> >  drivers/base/platform.c         |  4 ++++
> >  include/linux/platform_device.h | 26 ++++++++++++++++++++++++++
> >  3 files changed, 32 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > index 48d15dd785f6..1ae7449f70dc 100644
> > --- a/drivers/acpi/acpi_platform.c
> > +++ b/drivers/acpi/acpi_platform.c
> > @@ -168,6 +168,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
> >  	pdevinfo.num_res = count;
> >  	pdevinfo.fwnode = acpi_fwnode_handle(adev);
> >  	pdevinfo.properties = properties;
> > +	platform_devinfo_set_node(&pdevinfo, acpi_get_node(adev->handle));
> >  
> >  	if (acpi_dma_supported(adev))
> >  		pdevinfo.dma_mask = DMA_BIT_MASK(32);
> > @@ -178,11 +179,9 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
> >  	if (IS_ERR(pdev))
> >  		dev_err(&adev->dev, "platform device creation failed: %ld\n",
> >  			PTR_ERR(pdev));
> > -	else {
> > -		set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
> > +	else
> >  		dev_dbg(&adev->dev, "created platform device %s\n",
> >  			dev_name(&pdev->dev));
> > -	}
> >  
> >  	kfree(resources);
> >  
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 76bfcba25003..c733bfb26149 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -808,6 +808,7 @@ struct platform_device *platform_device_register_full(
> >  {
> >  	int ret;
> >  	struct platform_device *pdev;
> > +	int numa_node = platform_devinfo_get_node(pdevinfo);
> >  
> >  	pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
> >  	if (!pdev)
> > @@ -841,6 +842,9 @@ struct platform_device *platform_device_register_full(
> >  			goto err;
> >  	}
> >  
> > +	if (numa_node >= 0)
> > +		set_dev_node(&pdev->dev, numa_node);
> 
> Why not just always set it?  Why check?  Would that matter?
> 

I think it is better to add a check here, because we cannot be sure that the caller
will pass the correct parameters. If the caller pass pdevinfo->numa_node == -1, we
will get -2 here, but many check for numa_node just determine whether it is NUMA_NO_NODE.

> 
> > +
> >  	ret = platform_device_add(pdev);
> >  	if (ret) {
> >  err:
> > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > index 7a41c72c1959..78e11b79f1af 100644
> > --- a/include/linux/platform_device.h
> > +++ b/include/linux/platform_device.h
> > @@ -132,10 +132,36 @@ struct platform_device_info {
> >  		u64 dma_mask;
> >  
> >  		const struct property_entry *properties;
> > +
> > +#ifdef CONFIG_NUMA
> > +		int numa_node;	/* NUMA node this platform device is close to plus 1 */
> > +#endif
> 
> Why #ifdef?
> 

If CONFIG_NUMA=n, numa_node will be useless.

> And why an int?
> 

Keep the same as it in `struct device`.

> And why +1?
> 

Because not all the drivers will set numa_node, they just initialize `struct platform_device_info`
to zero.

> And what do you mean by "close to"?
> 
> And why would a platform device care about a numa node?  These are
> devices that should NEVER care about numa things as they are not on a
> real bus, or should care about performance things.  If they are, then
> the device is on the wrong bus, right?
> 
> What device are you having numa problems with?  Why would acpi devices
> care?
> 
> The node number in the device itself should be all that you need here,
> no need to duplicate it, right?
> 

1. "close to" may be better replaced with "proximity", indicates the proximity numa node of the device.

2. Some platform devices like accelerator in arm, they would be treated as platform devices. If we can
   get its proximity numa node through sysfs, we can bind its memory access to specify numa node, it
   will improve the performance for the app.

```
+--------+
| accel1 |    
+--------+
   |AXI (interconnect)
+--------+
| node1  |
+--------+
```

3. We can get numa node from struct device, but for platform device, it does not expose to userspace.
   And we just use `struct platform_device_info` to pass the numa node to `struct device`.

> thanks,
> 
> greg k-h
>

thanks,

Jinhui Guo