[PATCH 0/4] RFC: firmware: meson-sm: add chipid sysfs export

[PATCH 0/4] RFC: firmware: meson-sm: add chipid sysfs export

[Viacheslav Bocharov]

The Amlogic Meson SoC Secure Monitor implements a call to retrieve an
unique SoC ID starting from the GX Family and all new families.
But GX-family chips (e.g. GXB, GXL and newer) supports also 128-bit
chip ID. 128-bit chip ID consists 32-bit SoC version and 96-bit OTP data.

This patchset introduces an exported sysfs string for the 128-bit chipid,
considering a check for the version of the returned value. If the chip
does not support version 2 of the call, it falls back to (where possible)
information from the meson-gx-socinfo driver to supplement the data from OTP.

Viacheslav Bocharov (4):
  firmware: meson-sm: change sprintf to scnprintf
  firmware: meson_sm: Add chipid number sysfs entry
  soc: amlogic: meson-gx-socinfo: export socinfo for use in other
    modules
  firmware: meson_sm: use meson_gx_socinfo for compatibility

 drivers/firmware/meson/meson_sm.c      | 72 +++++++++++++++++++++++++-
 drivers/soc/amlogic/meson-gx-socinfo.c | 34 +++++++-----
 2 files changed, 90 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH 1/4] firmware: meson-sm: change sprintf to scnprintf

[Viacheslav Bocharov]

Update sprintf in serial_show frunction to scnprintf command to
prevent sysfs buffer overflow (buffer always is PAGE_SIZE bytes).

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 drivers/firmware/meson/meson_sm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index ed60f1103053..c1c694b485ee 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -265,7 +265,7 @@ static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
 		return ret;
 	}
 
-	ret = sprintf(buf, "%12phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
+	ret = scnprintf(buf, PAGE_SIZE, "%12phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
 
 	kfree(id_buf);
 
-- 
2.34.1

[PATCH 2/4] firmware: meson_sm: Add chipid number sysfs entry

[Viacheslav Bocharov]

The Amlogic Meson SoC Secure Monitor implements a call to retrieve an
unique CHIP ID with CPU ID (128 bits length) starting from the GX
Family and all new families.

The chipid string is simply exposed as a sysfs entry under the firmware
sysfs directory.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 drivers/firmware/meson/meson_sm.c | 54 ++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index c1c694b485ee..2820f4ac871b 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -240,9 +240,10 @@ struct meson_sm_firmware *meson_sm_get(struct device_node *sm_node)
 }
 EXPORT_SYMBOL_GPL(meson_sm_get);
 
-#define SM_CHIP_ID_LENGTH	119
+#define SM_CHIP_ID_LENGTH	128
 #define SM_CHIP_ID_OFFSET	4
 #define SM_CHIP_ID_SIZE		12
+#define SM_CHIP_IDv2_SIZE	16
 
 static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
@@ -274,8 +275,59 @@ static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(serial);
 
+static ssize_t chipid_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct meson_sm_firmware *fw;
+	uint8_t *id_buf;
+	int ret;
+
+	fw = platform_get_drvdata(pdev);
+
+	id_buf = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
+	if (!id_buf)
+		return -ENOMEM;
+
+	ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
+				 2, 0, 0, 0, 0);
+	if (ret < 0) {
+		kfree(id_buf);
+		return ret;
+	}
+
+	int version = *((unsigned int *)id_buf);
+
+	if (version == 2)
+		ret = scnprintf(buf, PAGE_SIZE, "%16phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
+	else {
+		/**
+		 * Legacy 12-byte chip ID read out, transform data
+		 * to expected order format.
+		 */
+		uint8_t *buff;
+
+		buff = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
+		if (!buff)
+			return -ENOMEM;
+		((uint32_t *)buff)[0] = 0; // CPU_ID is empty
+		/* Transform into expected order for display */
+		ch = (uint8_t *)(id_buf + 4);
+		for (i = 0; i < 12; i++)
+			buff[i + 4] = ch[11 - i];
+		ret = scnprintf(buf, PAGE_SIZE, "%16phN\n", &buff);
+		kfree(buff);
+	}
+
+	kfree(id_buf);
+	return ret;
+}
+
+static DEVICE_ATTR_RO(chipid);
+
 static struct attribute *meson_sm_sysfs_attributes[] = {
 	&dev_attr_serial.attr,
+	&dev_attr_chipid.attr,
 	NULL,
 };
 
-- 
2.34.1

Re: [PATCH 2/4] firmware: meson_sm: Add chipid number sysfs entry

[kernel test robot]

Hi Viacheslav,

kernel test robot noticed the following build errors:

[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.6 next-20231102]
[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/Viacheslav-Bocharov/firmware-meson-sm-change-sprintf-to-scnprintf/20231102-172556
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20231102074916.3280809-3-adeep%40lexina.in
patch subject: [PATCH 2/4] firmware: meson_sm: Add chipid number sysfs entry
config: arm64-randconfig-003-20231103 (https://download.01.org/0day-ci/archive/20231103/202311030513.HxyDUCuc-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/202311030513.HxyDUCuc-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/202311030513.HxyDUCuc-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/meson/meson_sm.c: In function 'chipid_show':
>> drivers/firmware/meson/meson_sm.c:315:17: error: 'ch' undeclared (first use in this function)
     315 |                 ch = (uint8_t *)(id_buf + 4);
         |                 ^~
   drivers/firmware/meson/meson_sm.c:315:17: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/firmware/meson/meson_sm.c:316:22: error: 'i' undeclared (first use in this function)
     316 |                 for (i = 0; i < 12; i++)
         |                      ^


vim +/ch +315 drivers/firmware/meson/meson_sm.c

   277	
   278	static ssize_t chipid_show(struct device *dev, struct device_attribute *attr,
   279				 char *buf)
   280	{
   281		struct platform_device *pdev = to_platform_device(dev);
   282		struct meson_sm_firmware *fw;
   283		uint8_t *id_buf;
   284		int ret;
   285	
   286		fw = platform_get_drvdata(pdev);
   287	
   288		id_buf = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
   289		if (!id_buf)
   290			return -ENOMEM;
   291	
   292		ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
   293					 2, 0, 0, 0, 0);
   294		if (ret < 0) {
   295			kfree(id_buf);
   296			return ret;
   297		}
   298	
   299		int version = *((unsigned int *)id_buf);
   300	
   301		if (version == 2)
   302			ret = scnprintf(buf, PAGE_SIZE, "%16phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
   303		else {
   304			/**
   305			 * Legacy 12-byte chip ID read out, transform data
   306			 * to expected order format.
   307			 */
   308			uint8_t *buff;
   309	
   310			buff = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
   311			if (!buff)
   312				return -ENOMEM;
   313			((uint32_t *)buff)[0] = 0; // CPU_ID is empty
   314			/* Transform into expected order for display */
 > 315			ch = (uint8_t *)(id_buf + 4);
 > 316			for (i = 0; i < 12; i++)
   317				buff[i + 4] = ch[11 - i];
   318			ret = scnprintf(buf, PAGE_SIZE, "%16phN\n", &buff);
   319			kfree(buff);
   320		}
   321	
   322		kfree(id_buf);
   323		return ret;
   324	}
   325	

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

[PATCH 3/4] soc: amlogic: meson-gx-socinfo: export socinfo for use in other modules

[Viacheslav Bocharov]

Move socinfo variable to global scope and export it as meson_gx_socinfo.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 drivers/soc/amlogic/meson-gx-socinfo.c | 34 +++++++++++++++-----------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/soc/amlogic/meson-gx-socinfo.c b/drivers/soc/amlogic/meson-gx-socinfo.c
index 6abb730344ab..0517f96a383b 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo.c
+++ b/drivers/soc/amlogic/meson-gx-socinfo.c
@@ -24,6 +24,10 @@
 #define SOCINFO_MINOR	GENMASK(15, 8)
 #define SOCINFO_MISC	GENMASK(7, 0)
 
+
+unsigned int meson_gx_socinfo;
+EXPORT_SYMBOL(meson_gx_socinfo);
+
 static const struct meson_gx_soc_id {
 	const char *name;
 	unsigned int id;
@@ -131,10 +135,10 @@ static int __init meson_gx_socinfo_init(void)
 	struct soc_device *soc_dev;
 	struct device_node *np;
 	struct regmap *regmap;
-	unsigned int socinfo;
 	struct device *dev;
 	int ret;
 
+	meson_gx_socinfo = 0;
 	/* look up for chipid node */
 	np = of_find_compatible_node(NULL, NULL, "amlogic,meson-gx-ao-secure");
 	if (!np)
@@ -160,11 +164,13 @@ static int __init meson_gx_socinfo_init(void)
 		return -ENODEV;
 	}
 
-	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo);
-	if (ret < 0)
+	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &meson_gx_socinfo);
+	if (ret < 0) {
+		meson_gx_socinfo = 0;
 		return ret;
+	}
 
-	if (!socinfo) {
+	if (!meson_gx_socinfo) {
 		pr_err("%s: invalid chipid value\n", __func__);
 		return -EINVAL;
 	}
@@ -175,13 +181,13 @@ static int __init meson_gx_socinfo_init(void)
 
 	soc_dev_attr->family = "Amlogic Meson";
 	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
-					   socinfo_to_major(socinfo),
-					   socinfo_to_minor(socinfo),
-					   socinfo_to_pack(socinfo),
-					   socinfo_to_misc(socinfo));
+					   socinfo_to_major(meson_gx_socinfo),
+					   socinfo_to_minor(meson_gx_socinfo),
+					   socinfo_to_pack(meson_gx_socinfo),
+					   socinfo_to_misc(meson_gx_socinfo));
 	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
-					 socinfo_to_soc_id(socinfo),
-					 socinfo_to_package_id(socinfo));
+					 socinfo_to_soc_id(meson_gx_socinfo),
+					 socinfo_to_package_id(meson_gx_socinfo));
 
 	soc_dev = soc_device_register(soc_dev_attr);
 	if (IS_ERR(soc_dev)) {
@@ -194,10 +200,10 @@ static int __init meson_gx_socinfo_init(void)
 
 	dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected\n",
 			soc_dev_attr->soc_id,
-			socinfo_to_major(socinfo),
-			socinfo_to_minor(socinfo),
-			socinfo_to_pack(socinfo),
-			socinfo_to_misc(socinfo));
+			socinfo_to_major(meson_gx_socinfo),
+			socinfo_to_minor(meson_gx_socinfo),
+			socinfo_to_pack(meson_gx_socinfo),
+			socinfo_to_misc(meson_gx_socinfo));
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 3/4] soc: amlogic: meson-gx-socinfo: export socinfo for use in other modules

[Krzysztof Kozlowski]

On 02/11/2023 08:49, Viacheslav Bocharov wrote:
> Move socinfo variable to global scope and export it as meson_gx_socinfo.

Why? What the patch is doing we can see from the diff. Commit msg should
explain why.

Best regards,
Krzysztof

[PATCH 4/4] firmware: meson_sm: use meson_gx_socinfo for compatibility

[Viacheslav Bocharov]

Use meson_gx_socinfo variable for chipid compatible call
from meson-gx-socinfo driver if available.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 drivers/firmware/meson/meson_sm.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index 2820f4ac871b..29b53a8a6941 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -23,6 +23,10 @@
 
 #include <linux/firmware/meson/meson_sm.h>
 
+#ifdef CONFIG_MESON_GX_SOCINFO
+extern unsigned int meson_gx_socinfo;
+#endif
+
 struct meson_sm_cmd {
 	unsigned int index;
 	u32 smc_id;
@@ -310,7 +314,19 @@ static ssize_t chipid_show(struct device *dev, struct device_attribute *attr,
 		buff = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
 		if (!buff)
 			return -ENOMEM;
-		((uint32_t *)buff)[0] = 0; // CPU_ID is empty
+#ifdef CONFIG_MESON_GX_SOCINFO
+		uint8_t *ch;
+		int i;
+
+		((uint32_t *)buff)[0] =
+			((meson_gx_socinfo & 0xff000000)        |       // Family ID
+			((meson_gx_socinfo << 8) & 0xff0000)    |       // Chip Revision
+			((meson_gx_socinfo >> 8) & 0xff00));            // Package ID
+
+		((uint32_t *)buff)[0] = htonl(((uint32_t *)buff)[0]);
+#else
+		((uint32)t *)buff)[0] = 0;
+#endif
 		/* Transform into expected order for display */
 		ch = (uint8_t *)(id_buf + 4);
 		for (i = 0; i < 12; i++)
-- 
2.34.1

Re: [PATCH 4/4] firmware: meson_sm: use meson_gx_socinfo for compatibility

[Krzysztof Kozlowski]

On 02/11/2023 08:49, Viacheslav Bocharov wrote:
> Use meson_gx_socinfo variable for chipid compatible call
> from meson-gx-socinfo driver if available.
> 

So we are back to something like ARMv7 platform/mach-code with drivers
tightly coupled between subsystems. But it is not 2007 anymore and we
have Devicetree for this. Use it instead.

What's more, your commit msg does not explain at all why do you need to
do it. This is some "show" callback, which does not exist in current
code. Adding code in one patch and then changing it, looks like you add
incomplete or buggy feature.

Best regards,
Krzysztof

Re: [PATCH 4/4] firmware: meson_sm: use meson_gx_socinfo for compatibility

[Viacheslav]

В Чт, 02/11/2023 в 10:26 +0100, Krzysztof Kozlowski пишет:
> On 02/11/2023 08:49, Viacheslav Bocharov wrote:
> > Use meson_gx_socinfo variable for chipid compatible call
> > from meson-gx-socinfo driver if available.
> > 
> 
> So we are back to something like ARMv7 platform/mach-code with
> drivers
> tightly coupled between subsystems. But it is not 2007 anymore and we
> have Devicetree for this. Use it instead.
> 
> What's more, your commit msg does not explain at all why do you need
> to
> do it. This is some "show" callback, which does not exist in current
> code. Adding code in one patch and then changing it, looks like you
> add
> incomplete or buggy feature.
> 
> Best regards,
> Krzysztof
> 


Fair enough.
This patch is related to an adjacent one where the socinfo data
supplements the result of executing the chipid version 1 function.

Indeed, with the introduction of chipid v.2, we now have a second
option for obtaining soc info (the first being implemented via register
reading). And I'm somewhat contemplative: whether to move the meson-gx-
socinfo entirely to the secure monitor or to duplicate the code from
there.

As a driver, meson-gx-socinfo does not carry practical information
apart from outputting status in the boot log, and it cannot be reused
without modifications to the driver.

--
Viacheslav Bocharov

Re: [PATCH 4/4] firmware: meson_sm: use meson_gx_socinfo for compatibility

[kernel test robot]

Hi Viacheslav,

kernel test robot noticed the following build errors:

[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.6 next-20231102]
[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/Viacheslav-Bocharov/firmware-meson-sm-change-sprintf-to-scnprintf/20231102-172556
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20231102074916.3280809-5-adeep%40lexina.in
patch subject: [PATCH 4/4] firmware: meson_sm: use meson_gx_socinfo for compatibility
config: arm64-randconfig-003-20231103 (https://download.01.org/0day-ci/archive/20231103/202311030839.2qiIuOUl-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/202311030839.2qiIuOUl-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/202311030839.2qiIuOUl-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/meson/meson_sm.c: In function 'chipid_show':
>> drivers/firmware/meson/meson_sm.c:328:19: error: 'uint32' undeclared (first use in this function); did you mean 'uint32_t'?
     328 |                 ((uint32)t *)buff)[0] = 0;
         |                   ^~~~~~
         |                   uint32_t
   drivers/firmware/meson/meson_sm.c:328:19: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/firmware/meson/meson_sm.c:328:26: error: expected ')' before 't'
     328 |                 ((uint32)t *)buff)[0] = 0;
         |                 ~        ^
         |                          )
>> drivers/firmware/meson/meson_sm.c:328:30: error: expected ';' before 'buff'
     328 |                 ((uint32)t *)buff)[0] = 0;
         |                              ^~~~
         |                              ;
>> drivers/firmware/meson/meson_sm.c:328:34: error: expected statement before ')' token
     328 |                 ((uint32)t *)buff)[0] = 0;
         |                                  ^
>> drivers/firmware/meson/meson_sm.c:328:35: error: expected expression before '[' token
     328 |                 ((uint32)t *)buff)[0] = 0;
         |                                   ^
   drivers/firmware/meson/meson_sm.c:331:17: error: 'ch' undeclared (first use in this function)
     331 |                 ch = (uint8_t *)(id_buf + 4);
         |                 ^~
   drivers/firmware/meson/meson_sm.c:332:22: error: 'i' undeclared (first use in this function)
     332 |                 for (i = 0; i < 12; i++)
         |                      ^


vim +328 drivers/firmware/meson/meson_sm.c

   281	
   282	static ssize_t chipid_show(struct device *dev, struct device_attribute *attr,
   283				 char *buf)
   284	{
   285		struct platform_device *pdev = to_platform_device(dev);
   286		struct meson_sm_firmware *fw;
   287		uint8_t *id_buf;
   288		int ret;
   289	
   290		fw = platform_get_drvdata(pdev);
   291	
   292		id_buf = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
   293		if (!id_buf)
   294			return -ENOMEM;
   295	
   296		ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
   297					 2, 0, 0, 0, 0);
   298		if (ret < 0) {
   299			kfree(id_buf);
   300			return ret;
   301		}
   302	
   303		int version = *((unsigned int *)id_buf);
   304	
   305		if (version == 2)
   306			ret = scnprintf(buf, PAGE_SIZE, "%16phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
   307		else {
   308			/**
   309			 * Legacy 12-byte chip ID read out, transform data
   310			 * to expected order format.
   311			 */
   312			uint8_t *buff;
   313	
   314			buff = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
   315			if (!buff)
   316				return -ENOMEM;
   317	#ifdef CONFIG_MESON_GX_SOCINFO
   318			uint8_t *ch;
   319			int i;
   320	
   321			((uint32_t *)buff)[0] =
   322				((meson_gx_socinfo & 0xff000000)        |       // Family ID
   323				((meson_gx_socinfo << 8) & 0xff0000)    |       // Chip Revision
   324				((meson_gx_socinfo >> 8) & 0xff00));            // Package ID
   325	
   326			((uint32_t *)buff)[0] = htonl(((uint32_t *)buff)[0]);
   327	#else
 > 328			((uint32)t *)buff)[0] = 0;
   329	#endif
   330			/* Transform into expected order for display */
   331			ch = (uint8_t *)(id_buf + 4);
   332			for (i = 0; i < 12; i++)
   333				buff[i + 4] = ch[11 - i];
   334			ret = scnprintf(buf, PAGE_SIZE, "%16phN\n", &buff);
   335			kfree(buff);
   336		}
   337	
   338		kfree(id_buf);
   339		return ret;
   340	}
   341	

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