2023-11-02 07:49:43

by Viacheslav

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

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


2023-11-02 07:49:47

by Viacheslav

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

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

Signed-off-by: Viacheslav Bocharov <[email protected]>
---
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

2023-11-02 07:49:49

by Viacheslav

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

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 <[email protected]>
---
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

2023-11-02 07:49:53

by Viacheslav

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

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

Signed-off-by: Viacheslav Bocharov <[email protected]>
---
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

2023-11-02 07:50:42

by Viacheslav

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

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 <[email protected]>
---
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

2023-11-02 09:24:28

by Krzysztof Kozlowski

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

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

2023-11-02 09:26:57

by Krzysztof Kozlowski

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

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

2023-11-02 10:02:42

by Viacheslav

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

В Чт, 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

2023-11-02 22:20:43

by kernel test robot

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

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/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2023-11-03 00:23:30

by kernel test robot

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

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/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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