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

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

Posted by guojinhui 2 years, 5 months ago

From: "guojinhui" <guojinhui.liam@bytedance.com>

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")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@intel.com/
Signed-off-by: guojinhui <guojinhui.liam@bytedance.com>
---
 drivers/acpi/acpi_platform.c |  4 +---
 drivers/base/platform.c      | 13 +++++++++++++
 2 files changed, 14 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..206dc7b020cd 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -795,6 +795,18 @@ void platform_device_unregister(struct platform_device *pdev)
 }
 EXPORT_SYMBOL_GPL(platform_device_unregister);
 
+#ifdef CONFIG_ACPI
+static inline void platform_set_dev_node(struct platform_device *pdev)
+{
+	struct acpi_device *adev = to_acpi_device_node(pdev->dev.fwnode);
+
+	if (adev && adev->handle)
+		set_dev_node(&pdev->dev, acpi_get_node(adev->handle));
+}
+#else
+static inline void platform_set_dev_node(struct platform_device *pdev) {}
+#endif
+
 /**
  * platform_device_register_full - add a platform-level device with
  * resources and platform-specific data
@@ -841,6 +853,7 @@ struct platform_device *platform_device_register_full(
 			goto err;
 	}
 
+	platform_set_dev_node(pdev);
 	ret = platform_device_add(pdev);
 	if (ret) {
 err:
-- 
2.20.1

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

Posted by Greg KH 2 years, 5 months ago

On Wed, Sep 13, 2023 at 04:38:25PM +0800, guojinhui wrote:
> From: "guojinhui" <guojinhui.liam@bytedance.com>
> 
> 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")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@intel.com/
> Signed-off-by: guojinhui <guojinhui.liam@bytedance.com>
> ---
>  drivers/acpi/acpi_platform.c |  4 +---
>  drivers/base/platform.c      | 13 +++++++++++++
>  2 files changed, 14 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..206dc7b020cd 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -795,6 +795,18 @@ void platform_device_unregister(struct platform_device *pdev)
>  }
>  EXPORT_SYMBOL_GPL(platform_device_unregister);
>  
> +#ifdef CONFIG_ACPI

#ifdef do not belong in .c files if at all possible.

Why can't this be an acpi call instead?  Why does this have to be in the
driver core?  Platform drivers shouldn't know anything about acpi, this
feels really odd.

> +static inline void platform_set_dev_node(struct platform_device *pdev)

Also, it's not "platform_set", it is acpi-specifc, right?  Again, the
ACPI core should handle this for its ACPI-platform devices, the driver
core shouldn't care at all.

thanks,

greg k-h

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

Posted by Jinhui Guo 2 years, 5 months ago

On Wed, 13 Sep 2023 06:37:03PM +0200, Greg KH wrote:
> On Wed, Sep 13, 2023 at 04:38:25PM +0800, guojinhui wrote:
> > From: "guojinhui" <guojinhui.liam@bytedance.com>
> > 
> > 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")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202309122309.mbxAnAIe-lkp@intel.com/
> > Signed-off-by: guojinhui <guojinhui.liam@bytedance.com>
> > ---
> >  drivers/acpi/acpi_platform.c |  4 +---
> >  drivers/base/platform.c      | 13 +++++++++++++
> >  2 files changed, 14 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..206dc7b020cd 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -795,6 +795,18 @@ void platform_device_unregister(struct platform_device *pdev)
> >  }
> >  EXPORT_SYMBOL_GPL(platform_device_unregister);
> >  
> > +#ifdef CONFIG_ACPI
> 
> #ifdef do not belong in .c files if at all possible.
> 
> Why can't this be an acpi call instead?  Why does this have to be in the
> driver core?  Platform drivers shouldn't know anything about acpi, this
> feels really odd.
> 
> > +static inline void platform_set_dev_node(struct platform_device *pdev)
> 
> Also, it's not "platform_set", it is acpi-specifc, right?  Again, the
> ACPI core should handle this for its ACPI-platform devices, the driver
> core shouldn't care at all.
> 
> thanks,
> 
> greg k-h
>

I agree with that. I will try to fix it in the ACPI code which call the platform function
soon.

thanks,

Jinhui Guo