[PATCH] base: soc: set machine name in soc_device_register if empty

[PATCH] base: soc: set machine name in soc_device_register if empty

[Heiner Kallweit]

Several SoC drivers use the same of-based mechanism to populate the machine
name. Therefore move this to the core and try to populate the machine name
in soc_device_register if it's not set yet.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/base/soc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 0fb1d4ab9..8dec5228f 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -7,6 +7,7 @@
 
 #include <linux/sysfs.h>
 #include <linux/init.h>
+#include <linux/of.h>
 #include <linux/stat.h>
 #include <linux/slab.h>
 #include <linux/idr.h>
@@ -110,6 +111,18 @@ static void soc_release(struct device *dev)
 	kfree(soc_dev);
 }
 
+static void soc_device_set_machine(struct soc_device_attribute *soc_dev_attr)
+{
+	struct device_node *np;
+
+	if (soc_dev_attr->machine)
+		return;
+
+	np = of_find_node_by_path("/");
+	of_property_read_string(np, "model", &soc_dev_attr->machine);
+	of_node_put(np);
+}
+
 static struct soc_device_attribute *early_soc_dev_attr;
 
 struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
@@ -118,6 +131,8 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
 	const struct attribute_group **soc_attr_groups;
 	int ret;
 
+	soc_device_set_machine(soc_dev_attr);
+
 	if (!soc_bus_registered) {
 		if (early_soc_dev_attr)
 			return ERR_PTR(-EBUSY);
-- 
2.39.2

Re: [PATCH] base: soc: set machine name in soc_device_register if empty

[Greg Kroah-Hartman]

On Thu, Mar 16, 2023 at 09:35:59PM +0100, Heiner Kallweit wrote:
> Several SoC drivers use the same of-based mechanism to populate the machine
> name. Therefore move this to the core and try to populate the machine name
> in soc_device_register if it's not set yet.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/base/soc.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 0fb1d4ab9..8dec5228f 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/sysfs.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
>  #include <linux/stat.h>
>  #include <linux/slab.h>
>  #include <linux/idr.h>
> @@ -110,6 +111,18 @@ static void soc_release(struct device *dev)
>  	kfree(soc_dev);
>  }
>  
> +static void soc_device_set_machine(struct soc_device_attribute *soc_dev_attr)
> +{
> +	struct device_node *np;
> +
> +	if (soc_dev_attr->machine)
> +		return;
> +
> +	np = of_find_node_by_path("/");
> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
> +	of_node_put(np);
> +}
> +
>  static struct soc_device_attribute *early_soc_dev_attr;
>  
>  struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> @@ -118,6 +131,8 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  	const struct attribute_group **soc_attr_groups;
>  	int ret;
>  
> +	soc_device_set_machine(soc_dev_attr);
> +

Does this mean some SoC drivers should also be changed at the same time
if they are trying to do this?

And is "model" the correct of property for this?  I thought that devices
also had "model" as a valid entry, is this documented somewhere in the
DTS schema that I couldn't find?

thanks,

greg k-h

Re: [PATCH] base: soc: set machine name in soc_device_register if empty

[Heiner Kallweit]

On 17.03.2023 06:08, Greg Kroah-Hartman wrote:
> On Thu, Mar 16, 2023 at 09:35:59PM +0100, Heiner Kallweit wrote:
>> Several SoC drivers use the same of-based mechanism to populate the machine
>> name. Therefore move this to the core and try to populate the machine name
>> in soc_device_register if it's not set yet.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/base/soc.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> index 0fb1d4ab9..8dec5228f 100644
>> --- a/drivers/base/soc.c
>> +++ b/drivers/base/soc.c
>> @@ -7,6 +7,7 @@
>>  
>>  #include <linux/sysfs.h>
>>  #include <linux/init.h>
>> +#include <linux/of.h>
>>  #include <linux/stat.h>
>>  #include <linux/slab.h>
>>  #include <linux/idr.h>
>> @@ -110,6 +111,18 @@ static void soc_release(struct device *dev)
>>  	kfree(soc_dev);
>>  }
>>  
>> +static void soc_device_set_machine(struct soc_device_attribute *soc_dev_attr)
>> +{
>> +	struct device_node *np;
>> +
>> +	if (soc_dev_attr->machine)
>> +		return;
>> +
>> +	np = of_find_node_by_path("/");
>> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
>> +	of_node_put(np);
>> +}
>> +
>>  static struct soc_device_attribute *early_soc_dev_attr;
>>  
>>  struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
>> @@ -118,6 +131,8 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>>  	const struct attribute_group **soc_attr_groups;
>>  	int ret;
>>  
>> +	soc_device_set_machine(soc_dev_attr);
>> +
> 
> Does this mean some SoC drivers should also be changed at the same time
> if they are trying to do this?
> 
The then duplicated code can be removed from SoC drivers afterwards.
There's no need to do it at the same time.
This change just adds a fallback in case the SoC driver doesn't set "machine".
Means if a SoC driver populates machine differently, then this is respected
and not overwritten.

> And is "model" the correct of property for this?  I thought that devices
> also had "model" as a valid entry, is this documented somewhere in the
> DTS schema that I couldn't find?
> 

"model" is used by basically all SoC drivers for this purpose, a quick grep reveals:

drivers/soc/samsung/exynos-chipid.c:    of_property_read_string(root, "model", &soc_dev_attr->machine);
drivers/soc/fsl/guts.c: if (of_property_read_string(of_root, "model", &machine))
drivers/soc/amlogic/meson-mx-socinfo.c: of_property_read_string(np, "model", &soc_dev_attr->machine);
drivers/soc/amlogic/meson-gx-socinfo.c: of_property_read_string(np, "model", &soc_dev_attr->machine);
drivers/soc/aspeed/aspeed-socinfo.c:    of_property_read_string(np, "model", &machine);
drivers/soc/ti/k3-socinfo.c:    of_property_read_string(node, "model", &soc_dev_attr->machine);
drivers/soc/loongson/loongson2_guts.c:  if (of_property_read_string(root, "model", &machine))
drivers/soc/renesas/renesas-soc.c:      of_property_read_string(np, "model", &soc_dev_attr->machine);
drivers/soc/imx/soc-imx.c:      ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
drivers/soc/imx/soc-imx8m.c:    ret = of_property_read_string(of_root, "model", &soc_dev_attr->machine);

Some don't set machine at all.

> thanks,
> 
> greg k-h

Heiner

Re: [PATCH] base: soc: set machine name in soc_device_register if empty

[Greg Kroah-Hartman]

On Fri, Mar 17, 2023 at 10:25:50AM +0100, Heiner Kallweit wrote:
> On 17.03.2023 06:08, Greg Kroah-Hartman wrote:
> > On Thu, Mar 16, 2023 at 09:35:59PM +0100, Heiner Kallweit wrote:
> >> Several SoC drivers use the same of-based mechanism to populate the machine
> >> name. Therefore move this to the core and try to populate the machine name
> >> in soc_device_register if it's not set yet.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/base/soc.c | 15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> >> index 0fb1d4ab9..8dec5228f 100644
> >> --- a/drivers/base/soc.c
> >> +++ b/drivers/base/soc.c
> >> @@ -7,6 +7,7 @@
> >>  
> >>  #include <linux/sysfs.h>
> >>  #include <linux/init.h>
> >> +#include <linux/of.h>
> >>  #include <linux/stat.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/idr.h>
> >> @@ -110,6 +111,18 @@ static void soc_release(struct device *dev)
> >>  	kfree(soc_dev);
> >>  }
> >>  
> >> +static void soc_device_set_machine(struct soc_device_attribute *soc_dev_attr)
> >> +{
> >> +	struct device_node *np;
> >> +
> >> +	if (soc_dev_attr->machine)
> >> +		return;
> >> +
> >> +	np = of_find_node_by_path("/");
> >> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
> >> +	of_node_put(np);
> >> +}
> >> +
> >>  static struct soc_device_attribute *early_soc_dev_attr;
> >>  
> >>  struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
> >> @@ -118,6 +131,8 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> >>  	const struct attribute_group **soc_attr_groups;
> >>  	int ret;
> >>  
> >> +	soc_device_set_machine(soc_dev_attr);
> >> +
> > 
> > Does this mean some SoC drivers should also be changed at the same time
> > if they are trying to do this?
> > 
> The then duplicated code can be removed from SoC drivers afterwards.
> There's no need to do it at the same time.
> This change just adds a fallback in case the SoC driver doesn't set "machine".
> Means if a SoC driver populates machine differently, then this is respected
> and not overwritten.

Please send this as a patch series that add this, and then removes this
code from the SoC drivers so we can verify that it is all working
properly.

> > And is "model" the correct of property for this?  I thought that devices
> > also had "model" as a valid entry, is this documented somewhere in the
> > DTS schema that I couldn't find?
> > 
> 
> "model" is used by basically all SoC drivers for this purpose, a quick grep reveals:
> 
> drivers/soc/samsung/exynos-chipid.c:    of_property_read_string(root, "model", &soc_dev_attr->machine);
> drivers/soc/fsl/guts.c: if (of_property_read_string(of_root, "model", &machine))
> drivers/soc/amlogic/meson-mx-socinfo.c: of_property_read_string(np, "model", &soc_dev_attr->machine);
> drivers/soc/amlogic/meson-gx-socinfo.c: of_property_read_string(np, "model", &soc_dev_attr->machine);
> drivers/soc/aspeed/aspeed-socinfo.c:    of_property_read_string(np, "model", &machine);
> drivers/soc/ti/k3-socinfo.c:    of_property_read_string(node, "model", &soc_dev_attr->machine);
> drivers/soc/loongson/loongson2_guts.c:  if (of_property_read_string(root, "model", &machine))
> drivers/soc/renesas/renesas-soc.c:      of_property_read_string(np, "model", &soc_dev_attr->machine);
> drivers/soc/imx/soc-imx.c:      ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
> drivers/soc/imx/soc-imx8m.c:    ret = of_property_read_string(of_root, "model", &soc_dev_attr->machine);
> 
> Some don't set machine at all.

So will this break systems that were not previously doing this by
showing an odd value?

thanks,

greg k-h

Re: [PATCH] base: soc: set machine name in soc_device_register if empty

[Heiner Kallweit]

On 17.03.2023 12:41, Greg Kroah-Hartman wrote:
> On Fri, Mar 17, 2023 at 10:25:50AM +0100, Heiner Kallweit wrote:
>> On 17.03.2023 06:08, Greg Kroah-Hartman wrote:
>>> On Thu, Mar 16, 2023 at 09:35:59PM +0100, Heiner Kallweit wrote:
>>>> Several SoC drivers use the same of-based mechanism to populate the machine
>>>> name. Therefore move this to the core and try to populate the machine name
>>>> in soc_device_register if it's not set yet.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/base/soc.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>>>> index 0fb1d4ab9..8dec5228f 100644
>>>> --- a/drivers/base/soc.c
>>>> +++ b/drivers/base/soc.c
>>>> @@ -7,6 +7,7 @@
>>>>  
>>>>  #include <linux/sysfs.h>
>>>>  #include <linux/init.h>
>>>> +#include <linux/of.h>
>>>>  #include <linux/stat.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/idr.h>
>>>> @@ -110,6 +111,18 @@ static void soc_release(struct device *dev)
>>>>  	kfree(soc_dev);
>>>>  }
>>>>  
>>>> +static void soc_device_set_machine(struct soc_device_attribute *soc_dev_attr)
>>>> +{
>>>> +	struct device_node *np;
>>>> +
>>>> +	if (soc_dev_attr->machine)
>>>> +		return;
>>>> +
>>>> +	np = of_find_node_by_path("/");
>>>> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
>>>> +	of_node_put(np);
>>>> +}
>>>> +
>>>>  static struct soc_device_attribute *early_soc_dev_attr;
>>>>  
>>>>  struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
>>>> @@ -118,6 +131,8 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>>>>  	const struct attribute_group **soc_attr_groups;
>>>>  	int ret;
>>>>  
>>>> +	soc_device_set_machine(soc_dev_attr);
>>>> +
>>>
>>> Does this mean some SoC drivers should also be changed at the same time
>>> if they are trying to do this?
>>>
>> The then duplicated code can be removed from SoC drivers afterwards.
>> There's no need to do it at the same time.
>> This change just adds a fallback in case the SoC driver doesn't set "machine".
>> Means if a SoC driver populates machine differently, then this is respected
>> and not overwritten.
> 
> Please send this as a patch series that add this, and then removes this
> code from the SoC drivers so we can verify that it is all working
> properly.
> 
OK, I'll make it a series incl. one use case where I have the hw to test.

>>> And is "model" the correct of property for this?  I thought that devices
>>> also had "model" as a valid entry, is this documented somewhere in the
>>> DTS schema that I couldn't find?
>>>
>>
>> "model" is used by basically all SoC drivers for this purpose, a quick grep reveals:
>>
>> drivers/soc/samsung/exynos-chipid.c:    of_property_read_string(root, "model", &soc_dev_attr->machine);
>> drivers/soc/fsl/guts.c: if (of_property_read_string(of_root, "model", &machine))
>> drivers/soc/amlogic/meson-mx-socinfo.c: of_property_read_string(np, "model", &soc_dev_attr->machine);
>> drivers/soc/amlogic/meson-gx-socinfo.c: of_property_read_string(np, "model", &soc_dev_attr->machine);
>> drivers/soc/aspeed/aspeed-socinfo.c:    of_property_read_string(np, "model", &machine);
>> drivers/soc/ti/k3-socinfo.c:    of_property_read_string(node, "model", &soc_dev_attr->machine);
>> drivers/soc/loongson/loongson2_guts.c:  if (of_property_read_string(root, "model", &machine))
>> drivers/soc/renesas/renesas-soc.c:      of_property_read_string(np, "model", &soc_dev_attr->machine);
>> drivers/soc/imx/soc-imx.c:      ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
>> drivers/soc/imx/soc-imx8m.c:    ret = of_property_read_string(of_root, "model", &soc_dev_attr->machine);
>>
>> Some don't set machine at all.
> 
> So will this break systems that were not previously doing this by
> showing an odd value?
> 
This change may populate "machine" in cases where "machine" isn't populated
currently. AFAICS the only impact is that this value is exposed via sysfs.
Odd values I'd expect only if the "model" property in DTS is odd.
Not sure whether there's any such case, this would need to be fixed
in the DTS.

> thanks,
> 
> greg k-h