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

[Greg KH <greg@kroah.com>]

On Mon, Jun 17, 2024 at 11:01:23AM +0800, guojinhui.liam wrote:
> From: Jinhui Guo <guojinhui.liam@bytedance.com>
> 
> Setting the devices' numa_node needs to be done in
> platform_device_register_full(), because that's where the
> platform device object is allocated.

Why in the world is a platform device on a numa node?  Are you sure you
are using platform devices properly if this is an issue?

And what platform devices / drivers care about this?

> Fixes: 4a60406d3592 ("driver core: platform: expose numa_node to users in sysfs")
> Cc: stable@vger.kernel.org
> Reported-by: kernel test robot <lkp@intel.com>

The robot reported this problem?

> Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@intel.com/

That's a problem with an older version of this patch, not this one.

> Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
> ---
> 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(-)

Any reason why you didn't cc the relevent maintainers here?

> 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

Ick, no, why?  Again, platform devices should NOT care about this.  If
they do, they should not be a platform device.

>  };
>  extern struct platform_device *platform_device_register_full(
>  		const struct platform_device_info *pdevinfo);
>  
> +#ifdef CONFIG_NUMA
> +static inline int platform_devinfo_get_node(const struct platform_device_info *pdevinfo)
> +{
> +	return pdevinfo ? pdevinfo->numa_node - 1 : NUMA_NO_NODE;
> +}
> +
> +static inline void platform_devinfo_set_node(struct platform_device_info *pdevinfo,
> +					     int node)
> +{
> +	pdevinfo->numa_node = node + 1;

Why +1?

thanks,

greg k-h