linux-kernel - Re: [PATCH] base: soc: set machine name in soc_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9d24dc05-3056-0cdb-025f-73123486db65@gmail.com>
Date:   Fri, 17 Mar 2023 12:59:11 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] base: soc: set machine name in soc_device_register if
 empty

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@...il.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