[PATCH] driver core: platform: set numa_node before platform_add_device()

[PATCH] driver core: platform: set numa_node before platform_add_device()

[guojinhui.liam]

platform_add_device creates numa_node attribute of sysfs according to
whether dev_to_node(dev) is equal to NUMA_NO_NODE. So set the numa node
of the device before creating numa_node attribute of sysfs.

Fixes: 4a60406d3592 ("driver core: platform: expose numa_node to users in sysfs")
Signed-off-by: guojinhui.liam <guojinhui.liam@bytedance.com>
---
 drivers/acpi/acpi_platform.c | 4 +---
 drivers/base/platform.c      | 4 ++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 48d15dd785f6..adcbfbdc343f 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -178,11 +178,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..355abf91930a 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;
+	struct acpi_device *adev = to_acpi_device_node(pdevinfo->fwnode);
 
 	pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
 	if (!pdev)
@@ -841,6 +842,9 @@ struct platform_device *platform_device_register_full(
 			goto err;
 	}
 
+	if (adev)
+		set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
+
 	ret = platform_device_add(pdev);
 	if (ret) {
 err:
-- 
2.20.1

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

[Greg KH]

On Tue, Sep 12, 2023 at 03:09:00PM +0800, guojinhui.liam wrote:
> platform_add_device creates numa_node attribute of sysfs according to
> whether dev_to_node(dev) is equal to NUMA_NO_NODE. So set the numa node
> of the device before creating numa_node attribute of sysfs.

Why?  What will this allow to happen differently?  What is broken with
the current code?

> Fixes: 4a60406d3592 ("driver core: platform: expose numa_node to users in sysfs")
> Signed-off-by: guojinhui.liam <guojinhui.liam@bytedance.com>

Please use your name here, and not your email alias, as the first part
of the signed-off-by line.

And also, please fix up your email client to have the correct name as
well.

> ---
>  drivers/acpi/acpi_platform.c | 4 +---
>  drivers/base/platform.c      | 4 ++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 48d15dd785f6..adcbfbdc343f 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -178,11 +178,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..355abf91930a 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;
> +	struct acpi_device *adev = to_acpi_device_node(pdevinfo->fwnode);
>  
>  	pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
>  	if (!pdev)
> @@ -841,6 +842,9 @@ struct platform_device *platform_device_register_full(
>  			goto err;
>  	}
>  
> +	if (adev)
> +		set_dev_node(&pdev->dev, acpi_get_node(adev->handle));

Are you sure that this platform code can always call acpi functions?

thanks,

greg k-h

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

[kernel test robot]

Hi guojinhui.liam,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc1 next-20230912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/guojinhui-liam/driver-core-platform-set-numa_node-before-platform_add_device/20230912-151119
base:   linus/master
patch link:    https://lore.kernel.org/r/20230912070900.1862-1-guojinhui.liam%40bytedance.com
patch subject: [PATCH] driver core: platform: set numa_node before platform_add_device()
config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20230912/202309122309.mbxAnAIe-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230912/202309122309.mbxAnAIe-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/base/platform.c: In function 'platform_device_register_full':
>> drivers/base/platform.c:846:42: error: implicit declaration of function 'acpi_get_node'; did you mean 'acpi_get_name'? [-Werror=implicit-function-declaration]
     846 |                 set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
         |                                          ^~~~~~~~~~~~~
         |                                          acpi_get_name
>> drivers/base/platform.c:846:60: error: invalid use of undefined type 'struct acpi_device'
     846 |                 set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
         |                                                            ^~
   cc1: some warnings being treated as errors


vim +846 drivers/base/platform.c

   797	
   798	/**
   799	 * platform_device_register_full - add a platform-level device with
   800	 * resources and platform-specific data
   801	 *
   802	 * @pdevinfo: data used to create device
   803	 *
   804	 * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
   805	 */
   806	struct platform_device *platform_device_register_full(
   807			const struct platform_device_info *pdevinfo)
   808	{
   809		int ret;
   810		struct platform_device *pdev;
   811		struct acpi_device *adev = to_acpi_device_node(pdevinfo->fwnode);
   812	
   813		pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
   814		if (!pdev)
   815			return ERR_PTR(-ENOMEM);
   816	
   817		pdev->dev.parent = pdevinfo->parent;
   818		pdev->dev.fwnode = pdevinfo->fwnode;
   819		pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
   820		pdev->dev.of_node_reused = pdevinfo->of_node_reused;
   821	
   822		if (pdevinfo->dma_mask) {
   823			pdev->platform_dma_mask = pdevinfo->dma_mask;
   824			pdev->dev.dma_mask = &pdev->platform_dma_mask;
   825			pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
   826		}
   827	
   828		ret = platform_device_add_resources(pdev,
   829				pdevinfo->res, pdevinfo->num_res);
   830		if (ret)
   831			goto err;
   832	
   833		ret = platform_device_add_data(pdev,
   834				pdevinfo->data, pdevinfo->size_data);
   835		if (ret)
   836			goto err;
   837	
   838		if (pdevinfo->properties) {
   839			ret = device_create_managed_software_node(&pdev->dev,
   840								  pdevinfo->properties, NULL);
   841			if (ret)
   842				goto err;
   843		}
   844	
   845		if (adev)
 > 846			set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
   847	
   848		ret = platform_device_add(pdev);
   849		if (ret) {
   850	err:
   851			ACPI_COMPANION_SET(&pdev->dev, NULL);
   852			platform_device_put(pdev);
   853			return ERR_PTR(ret);
   854		}
   855	
   856		return pdev;
   857	}
   858	EXPORT_SYMBOL_GPL(platform_device_register_full);
   859	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[guojinhui]

> On Tue, Sep 12, 2023 at 03:09:00PM +0800, guojinhui.liam wrote:
> > platform_add_device creates numa_node attribute of sysfs according to
> > whether dev_to_node(dev) is equal to NUMA_NO_NODE. So set the numa node
> > of the device before creating numa_node attribute of sysfs.
>
> Why?  What will this allow to happen differently?  What is broken with
> the current code?

Commit 4a60406d3592 can run well with dtb, because of_device_add() set the numa_node before device_add().
I think it didn't test acpi before submitting.

> > Fixes: 4a60406d3592 ("driver core: platform: expose numa_node to users in sysfs")
> > Signed-off-by: guojinhui.liam <guojinhui.liam@bytedance.com>
>
> Please use your name here, and not your email alias, as the first part
> of the signed-off-by line.
>
> And also, please fix up your email client to have the correct name as
> well.

I will fix them with a new patch v2.

> > ---
> >  drivers/acpi/acpi_platform.c | 4 +---
> >  drivers/base/platform.c      | 4 ++++
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > index 48d15dd785f6..adcbfbdc343f 100644
> > --- a/drivers/acpi/acpi_platform.c
> > +++ b/drivers/acpi/acpi_platform.c
> > @@ -178,11 +178,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..355abf91930a 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;
> > +	struct acpi_device *adev = to_acpi_device_node(pdevinfo->fwnode);
> >  
> >  	pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
> >  	if (!pdev)
> > @@ -841,6 +842,9 @@ struct platform_device *platform_device_register_full(
> >  			goto err;
> >  	}
> >  
> > +	if (adev)
> > +		set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
>
> Are you sure that this platform code can always call acpi functions?

Thanks for your review, there are something wrong without enabling CONFIG_ACPI.
I will fix it with patch v2 soon.

thanks,

guojinhui

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

[kernel test robot]

Hi guojinhui.liam,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc1 next-20230913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/guojinhui-liam/driver-core-platform-set-numa_node-before-platform_add_device/20230912-151119
base:   linus/master
patch link:    https://lore.kernel.org/r/20230912070900.1862-1-guojinhui.liam%40bytedance.com
patch subject: [PATCH] driver core: platform: set numa_node before platform_add_device()
config: um-allyesconfig (https://download.01.org/0day-ci/archive/20230913/202309131848.CgiiHpZu-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230913/202309131848.CgiiHpZu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309131848.CgiiHpZu-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/base/platform.c:15:
   In file included from include/linux/of_irq.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/base/platform.c:15:
   In file included from include/linux/of_irq.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/base/platform.c:15:
   In file included from include/linux/of_irq.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/base/platform.c:846:28: error: implicit declaration of function 'acpi_get_node' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                   set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
                                            ^
   drivers/base/platform.c:846:28: note: did you mean 'acpi_get_name'?
   include/acpi/acpixf.h:525:9: note: 'acpi_get_name' declared here
                                acpi_get_name(acpi_handle object, u32 name_type,
                                ^
   include/acpi/platform/aclinux.h:93:21: note: expanded from macro 'ACPI_EXTERNAL_RETURN_STATUS'
           static ACPI_INLINE prototype {return(AE_NOT_CONFIGURED);}
                              ^
   drivers/base/platform.c:846:46: error: incomplete definition of type 'struct acpi_device'
                   set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
                                                          ~~~~^
   include/linux/acpi.h:788:8: note: forward declaration of 'struct acpi_device'
   struct acpi_device;
          ^
   12 warnings and 2 errors generated.


vim +/acpi_get_node +846 drivers/base/platform.c

   797	
   798	/**
   799	 * platform_device_register_full - add a platform-level device with
   800	 * resources and platform-specific data
   801	 *
   802	 * @pdevinfo: data used to create device
   803	 *
   804	 * Returns &struct platform_device pointer on success, or ERR_PTR() on error.
   805	 */
   806	struct platform_device *platform_device_register_full(
   807			const struct platform_device_info *pdevinfo)
   808	{
   809		int ret;
   810		struct platform_device *pdev;
   811		struct acpi_device *adev = to_acpi_device_node(pdevinfo->fwnode);
   812	
   813		pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
   814		if (!pdev)
   815			return ERR_PTR(-ENOMEM);
   816	
   817		pdev->dev.parent = pdevinfo->parent;
   818		pdev->dev.fwnode = pdevinfo->fwnode;
   819		pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
   820		pdev->dev.of_node_reused = pdevinfo->of_node_reused;
   821	
   822		if (pdevinfo->dma_mask) {
   823			pdev->platform_dma_mask = pdevinfo->dma_mask;
   824			pdev->dev.dma_mask = &pdev->platform_dma_mask;
   825			pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
   826		}
   827	
   828		ret = platform_device_add_resources(pdev,
   829				pdevinfo->res, pdevinfo->num_res);
   830		if (ret)
   831			goto err;
   832	
   833		ret = platform_device_add_data(pdev,
   834				pdevinfo->data, pdevinfo->size_data);
   835		if (ret)
   836			goto err;
   837	
   838		if (pdevinfo->properties) {
   839			ret = device_create_managed_software_node(&pdev->dev,
   840								  pdevinfo->properties, NULL);
   841			if (ret)
   842				goto err;
   843		}
   844	
   845		if (adev)
 > 846			set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
   847	
   848		ret = platform_device_add(pdev);
   849		if (ret) {
   850	err:
   851			ACPI_COMPANION_SET(&pdev->dev, NULL);
   852			platform_device_put(pdev);
   853			return ERR_PTR(ret);
   854		}
   855	
   856		return pdev;
   857	}
   858	EXPORT_SYMBOL_GPL(platform_device_register_full);
   859	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Jinhui Guo]

> Hi guojinhui.liam,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.6-rc1 next-20230913]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/guojinhui-liam/driver-core-platform-set-numa_node-before-platform_add_device/20230912-151119
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20230912070900.1862-1-guojinhui.liam%40bytedance.com
> patch subject: [PATCH] driver core: platform: set numa_node before platform_add_device()
> config: um-allyesconfig (https://download.01.org/0day-ci/archive/20230913/202309131848.CgiiHpZu-lkp@intel.com/config)
> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230913/202309131848.CgiiHpZu-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202309131848.CgiiHpZu-lkp@intel.com/
> ...

Is there anyone known how to stop this test for my first patch? I have sent a new one to review, which fixes the compile bug.

thanks,

Jinhui Guo

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

[Nick Desaulniers]

On Wed, Sep 13, 2023 at 4:46 AM Jinhui Guo <guojinhui.liam@bytedance.com> wrote:
>
> > Hi guojinhui.liam,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on linus/master]
> > [also build test ERROR on v6.6-rc1 next-20230913]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/guojinhui-liam/driver-core-platform-set-numa_node-before-platform_add_device/20230912-151119
> > base:   linus/master
> > patch link:    https://lore.kernel.org/r/20230912070900.1862-1-guojinhui.liam%40bytedance.com
> > patch subject: [PATCH] driver core: platform: set numa_node before platform_add_device()
> > config: um-allyesconfig (https://download.01.org/0day-ci/archive/20230913/202309131848.CgiiHpZu-lkp@intel.com/config)
> > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230913/202309131848.CgiiHpZu-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202309131848.CgiiHpZu-lkp@intel.com/
> > ...
>
> Is there anyone known how to stop this test for my first patch? I have sent a new one to review, which fixes the compile bug.

Then it's ok to either ignore the report, or do as you did and say "I
sent a V+1." Doesn't hurt to have a link to the next revision on lore
if you are to reply.

kbuild test robot will test each patch sent to LKML; it might not stop
ongoing tests if a V+1 is sent.

>
> thanks,
>
> Jinhui Guo
>


-- 
Thanks,
~Nick Desaulniers

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

[Jinhui Guo]

On Mon, 18 Sep 2023 12:30:58 +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 14, 2023 at 11:32 PM Jinhui Guo
> <guojinhui.liam@bytedance.com> wrote:
> >
> > platform_add_device()
> 
> According to "git grep" this function is not present in 6.6-rc2.
> 
> If you mean platform_device_add(), please update the patch subject and
> changelog accordingly.
> 

This is my mistake, the function name was written wrong.
I will fix it in the next patch.

> > creates the numa_node attribute of sysfs according
> > to whether dev_to_node(dev) is equal to NUMA_NO_NODE. So set the numa node
> > of device before creating numa_node attribute of sysfs.
> 
> It would be good to also say that this needs to be done in
> platform_device_register_full(), because that's where the platform
> device object is allocated.
> 

Thaks for your suggestion. I will modify my decription soon.

> However, what about adding the NUMA node information to pdevinfo?  It
> would be more straightforward to handle it then AFAICS.
> 

I have tried three potential solutions to fix the bug:
1. The first one is what the current patch do.

2. Add a new function interface only for acpi_create_platform_device() call.
   But the code will be a bit redundant.

3. Add an member "numa_node" in `struct platform_device_info`, just as what
   `struct device` done:

```
struct platform_device_info {
	...;
#ifdef CONFIG_NUMA
	int		numa_node;
#endif
```

But not all the call to platform_device_register_full() would set numa_node,
and many of them use ` memset(&pdevinfo, 0, sizeof(pdevinfo));` to initialize
`struct platform_device_info`. It could initialize numa_node to zero and
result in wrong numa_node information in sysfs.

```
struct platform_device *platform_device_register_full(
		const struct platform_device_info *pdevinfo) {
	...;
	/*
	 * (1) It will initialize numa_node in `struct device` to NUMA_NO_NODE.
	 *     NUMA_NO_NODE is -1.
	 */
	pdev = platform_device_alloc(pdevinfo->name, pdevinfo->id);
	...;
	/*
	 * (2) If we add set_dev_node() here, we have to make sure pdevinfo->numa_node
	 *     is correct. But It is difficult to do so, especially drivers don't want to
	 *     set numa_node. Instead of initializing pdevinfo->numa_node to NUMA_NO_NODE,
	 *     they are accustomed to memset `struct platform_device_info` to be zero.
	 */
	set_dev_node(&pdev->dev, pdevinfo->numa_node);
	...;
	/*
	 * (3) The sysfs attribute numa_node will create here.
	 */
	ret = platform_device_add(pdev);
	...;
}
```

> > 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>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@intel.com/
> > Signed-off-by: Jinhui Guo <guojinhui.liam@bytedance.com>
> > ---
> > V4 -> V5: Add Cc: stable line and changes from the previous submited
> > patches
> > V3 -> V4: Refactor code to be an ACPI function call
> > V2 -> V3: Fix Signed-off name
> > V1 -> V2: Fix compile error without enabling CONFIG_ACPI
> >
> >  drivers/acpi/acpi_platform.c | 4 +---
> >  drivers/base/platform.c      | 1 +
> >  include/linux/acpi.h         | 5 +++++
> >  3 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> > index 48d15dd785f6..adcbfbdc343f 100644
> > --- a/drivers/acpi/acpi_platform.c
> > +++ b/drivers/acpi/acpi_platform.c
> > @@ -178,11 +178,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..35c891075d95 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -841,6 +841,7 @@ struct platform_device *platform_device_register_full(
> >                         goto err;
> >         }
> >
> > +       set_dev_node(&pdev->dev, ACPI_NODE_GET(ACPI_COMPANION(&pdev->dev)));
> >         ret = platform_device_add(pdev);
> >         if (ret) {
> >  err:
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index a73246c3c35e..6a349d53f19e 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -477,6 +477,10 @@ static inline int acpi_get_node(acpi_handle handle)
> >         return 0;
> >  }
> >  #endif
> > +
> > +#define ACPI_NODE_GET(adev) ((adev) && (adev)->handle ? \
> > +       acpi_get_node((adev)->handle) : NUMA_NO_NODE)
> > +
> >  extern int pnpacpi_disabled;
> >
> >  #define PXM_INVAL      (-1)
> > @@ -770,6 +774,7 @@ const char *acpi_get_subsystem_id(acpi_handle handle);
> >  #define ACPI_COMPANION_SET(dev, adev)  do { } while (0)
> >  #define ACPI_HANDLE(dev)               (NULL)
> >  #define ACPI_HANDLE_FWNODE(fwnode)     (NULL)
> > +#define ACPI_NODE_GET(adev)            NUMA_NO_NODE
> >
> >  #include <acpi/acpi_numa.h>
> >
> > --
> > 2.20.1
> >

Thanks,

Jinhui Guo

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

[Rafael J. Wysocki]

On Mon, Sep 18, 2023 at 2:41 PM Jinhui Guo <guojinhui.liam@bytedance.com> wrote:
>
> On Mon, 18 Sep 2023 12:30:58 +0200, Rafael J. Wysocki wrote:
> > On Thu, Sep 14, 2023 at 11:32 PM Jinhui Guo
> > <guojinhui.liam@bytedance.com> wrote:
> > >
> > > platform_add_device()
> >
> > According to "git grep" this function is not present in 6.6-rc2.
> >
> > If you mean platform_device_add(), please update the patch subject and
> > changelog accordingly.
> >
>
> This is my mistake, the function name was written wrong.
> I will fix it in the next patch.
>
> > > creates the numa_node attribute of sysfs according
> > > to whether dev_to_node(dev) is equal to NUMA_NO_NODE. So set the numa node
> > > of device before creating numa_node attribute of sysfs.
> >
> > It would be good to also say that this needs to be done in
> > platform_device_register_full(), because that's where the platform
> > device object is allocated.
> >
>
> Thaks for your suggestion. I will modify my decription soon.
>
> > However, what about adding the NUMA node information to pdevinfo?  It
> > would be more straightforward to handle it then AFAICS.
> >
>
> I have tried three potential solutions to fix the bug:
> 1. The first one is what the current patch do.
>
> 2. Add a new function interface only for acpi_create_platform_device() call.
>    But the code will be a bit redundant.
>
> 3. Add an member "numa_node" in `struct platform_device_info`, just as what
>    `struct device` done:
>
> ```
> struct platform_device_info {
>         ...;
> #ifdef CONFIG_NUMA
>         int             numa_node;
> #endif
> ```
>
> But not all the call to platform_device_register_full() would set numa_node,
> and many of them use ` memset(&pdevinfo, 0, sizeof(pdevinfo));` to initialize
> `struct platform_device_info`. It could initialize numa_node to zero and
> result in wrong numa_node information in sysfs.

Well, platform_device_register_full() need not take that value as the
numa node number directly.  It may, for example, take the number from
pdevinfo, subtract 1 from it and use the result of that as the numa
node number, if not negative.