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 is next attempt to publish data from the Amlogic secure monitor
chipid call. After discussions with Neil Armstrong, it was decided to
publish the chipid call results through the soc driver. Since
soc_device_match cannot wait for the soc driver to load, and the secure
monitor calls in turn depend on the sm driver, it was necessary to create
a new driver rather than expand an existing one.
In the patches, in addition to writing the driver:
- convert commonly used structures and functions of the meson-gx-socinfo
driver to a header file.
- add secure-monitor references for amlogic,meson-gx-ao-secure sections
in dts files of the a1, axg, g12, gx families.
---
Changes
v2 [1] -> v3:
- rebase
- update dependency in Kconfig for MESON_GX_SOCINFO_SM
- add links to secure-monitor in soc driver sections for A1, AXG, GX, G12
v1 [2] -> v2:
- create cpu_id structure for socinfo variable
- create meson_sm_chip_id for result of sm call
- remove shared functions
- move from funcs for bit operations to C bit fields
Links:
- [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
- [2] https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
Viacheslav Bocharov (4):
soc: amlogic: meson-gx-socinfo: move common code to header file
soc: amlogic: meson-gx-socinfo-sm: Add Amlogic secure-monitor SoC
Information driver
soc: amlogic: meson-gx-socinfo: add new definition for Amlogic A113X
package
arm64: dts: meson: add dts links to secure-monitor for soc driver in
a1, axg, gx, g12
arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 1 +
arch/arm64/boot/dts/amlogic/meson-axg.dtsi | 1 +
.../boot/dts/amlogic/meson-g12-common.dtsi | 1 +
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 1 +
drivers/soc/amlogic/Kconfig | 10 +
drivers/soc/amlogic/Makefile | 1 +
.../soc/amlogic/meson-gx-socinfo-internal.h | 121 +++++++++++
drivers/soc/amlogic/meson-gx-socinfo-sm.c | 192 ++++++++++++++++++
drivers/soc/amlogic/meson-gx-socinfo.c | 136 ++-----------
9 files changed, 342 insertions(+), 122 deletions(-)
create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
base-commit: 480e035fc4c714fb5536e64ab9db04fedc89e910
--
2.43.2
Add links to secure-monitor in soc driver section for A1, AXG, GX, G12
Amlogic family for use with meson-socinfo-sm driver.
Signed-off-by: Viacheslav Bocharov <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 1 +
arch/arm64/boot/dts/amlogic/meson-axg.dtsi | 1 +
arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 1 +
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 1 +
4 files changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
index c03e207ea6c5..d47f056117fc 100644
--- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
@@ -407,6 +407,7 @@ hwrng: rng@5118 {
sec_AO: ao-secure@5a20 {
compatible = "amlogic,meson-gx-ao-secure", "syscon";
reg = <0x0 0x5a20 0x0 0x140>;
+ secure-monitor = <&sm>;
amlogic,has-chip-id;
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index 6d12b760b90f..45791ec6694a 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -1689,6 +1689,7 @@ mux {
sec_AO: ao-secure@140 {
compatible = "amlogic,meson-gx-ao-secure", "syscon";
reg = <0x0 0x140 0x0 0x140>;
+ secure-monitor = <&sm>;
amlogic,has-chip-id;
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index 9d5eab6595d0..a8c1c72543b7 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -2026,6 +2026,7 @@ cec_AO: cec@100 {
sec_AO: ao-secure@140 {
compatible = "amlogic,meson-gx-ao-secure", "syscon";
reg = <0x0 0x140 0x0 0x140>;
+ secure-monitor = <&sm>;
amlogic,has-chip-id;
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 2673f0dbafe7..656e08b3d872 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -471,6 +471,7 @@ cec_AO: cec@100 {
sec_AO: ao-secure@140 {
compatible = "amlogic,meson-gx-ao-secure", "syscon";
reg = <0x0 0x140 0x0 0x140>;
+ secure-monitor = <&sm>;
amlogic,has-chip-id;
};
--
2.43.2
Amlogic SoCs have a SoC information secure-monitor call for SoC type,
package type, revision information and chipid.
This patchs adds support for secure-monitor call decoding and exposing
with the SoC bus infrastructure in addition to the previous SoC
Information driver.
Signed-off-by: Viacheslav Bocharov <[email protected]>
---
drivers/soc/amlogic/Kconfig | 10 ++
drivers/soc/amlogic/Makefile | 1 +
drivers/soc/amlogic/meson-gx-socinfo-sm.c | 192 ++++++++++++++++++++++
3 files changed, 203 insertions(+)
create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
index d08e398bdad4..82fc77ca3b4b 100644
--- a/drivers/soc/amlogic/Kconfig
+++ b/drivers/soc/amlogic/Kconfig
@@ -26,6 +26,16 @@ config MESON_GX_SOCINFO
Say yes to support decoding of Amlogic Meson GX SoC family
information about the type, package and version.
+config MESON_GX_SOCINFO_SM
+ bool "Amlogic Meson GX SoC Information driver via Secure Monitor"
+ depends on (ARM64 && ARCH_MESON || COMPILE_TEST) && MESON_SM=y
+ default ARCH_MESON && MESON_SM
+ select SOC_BUS
+ help
+ Say yes to support decoding of Amlogic Meson GX SoC family
+ information about the type, package and version via secure
+ monitor call.
+
config MESON_MX_SOCINFO
bool "Amlogic Meson MX SoC Information driver"
depends on (ARM && ARCH_MESON) || COMPILE_TEST
diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
index c25f835e6a26..45d9d6f5904c 100644
--- a/drivers/soc/amlogic/Makefile
+++ b/drivers/soc/amlogic/Makefile
@@ -2,4 +2,5 @@
obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
+obj-$(CONFIG_MESON_GX_SOCINFO_SM) += meson-gx-socinfo-sm.o
obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
diff --git a/drivers/soc/amlogic/meson-gx-socinfo-sm.c b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
new file mode 100644
index 000000000000..e30e1d2feb61
--- /dev/null
+++ b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Copyright (c) 2024 JetHome
+ * Author: Neil Armstrong <[email protected]>
+ * Author: Viacheslav Bocharov <[email protected]>
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#include <linux/firmware/meson/meson_sm.h>
+
+#include "meson-gx-socinfo-internal.h"
+
+static char *socinfo_get_chipid(struct device *dev, struct meson_sm_firmware *fw,
+ union meson_cpu_id *socinfo)
+{
+ char *buf;
+ struct meson_sm_chip_id *id_buf;
+ int ret;
+
+ id_buf = devm_kzalloc(dev, sizeof(struct meson_sm_chip_id)+1, GFP_KERNEL);
+ if (!id_buf)
+ return NULL;
+
+ ret = meson_sm_call_read(fw, id_buf, sizeof(struct meson_sm_chip_id), SM_GET_CHIP_ID,
+ 2, 0, 0, 0, 0);
+ if (ret < 0) {
+ kfree(id_buf);
+ return NULL;
+ }
+ dev_info(dev, "got sm version call %i\n", id_buf->version);
+
+ if (id_buf->version != 2) {
+
+ u8 tmp;
+ /**
+ * Legacy 12-byte chip ID read out, transform data
+ * to expected order format
+ */
+ memmove((void *)&id_buf->serial, (void *)&id_buf->cpu_id, 12);
+ for (int i = 0; i < 6; i++) {
+ tmp = id_buf->serial[i];
+ id_buf->serial[i] = id_buf->serial[11 - i];
+ id_buf->serial[11 - i] = tmp;
+ }
+ id_buf->cpu_id.v2.major_id = socinfo->v1.major_id;
+ id_buf->cpu_id.v2.pack_id = socinfo->v1.pack_id;
+ id_buf->cpu_id.v2.chip_rev = socinfo->v1.chip_rev;
+ id_buf->cpu_id.v2.reserved = socinfo->v1.reserved;
+ id_buf->cpu_id.v2.layout_ver = socinfo->v1.layout_ver;
+ } else {
+ /**
+ * rewrite socinfo from regmap with value from secure monitor call
+ */
+ socinfo->v1.major_id = id_buf->cpu_id.v2.major_id;
+ socinfo->v1.pack_id = id_buf->cpu_id.v2.pack_id;
+ socinfo->v1.chip_rev = id_buf->cpu_id.v2.chip_rev;
+ socinfo->v1.reserved = id_buf->cpu_id.v2.reserved;
+ socinfo->v1.layout_ver = id_buf->cpu_id.v2.layout_ver;
+ }
+
+ buf = kasprintf(GFP_KERNEL, "%4phN%12phN", &(id_buf->cpu_id), &(id_buf->serial));
+
+ kfree(id_buf);
+
+ return buf;
+}
+
+static int meson_gx_socinfo_sm_probe(struct platform_device *pdev)
+{
+ struct soc_device_attribute *soc_dev_attr;
+ struct soc_device *soc_dev;
+ struct device_node *sm_np;
+ struct meson_sm_firmware *fw;
+ struct regmap *regmap;
+ union meson_cpu_id socinfo;
+ struct device *dev;
+ int ret;
+
+ /* check if chip-id is available */
+ if (!of_property_read_bool(pdev->dev.of_node, "amlogic,has-chip-id"))
+ return -ENODEV;
+
+ /* node should be a syscon */
+ regmap = syscon_node_to_regmap(pdev->dev.of_node);
+ if (IS_ERR(regmap)) {
+ dev_err(&pdev->dev, "failed to get regmap\n");
+ return -ENODEV;
+ }
+
+ sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
+ if (!sm_np) {
+ dev_err(&pdev->dev, "no secure-monitor node found\n");
+ return -ENODEV;
+ }
+
+ fw = meson_sm_get(sm_np);
+ of_node_put(sm_np);
+ if (!fw) {
+ dev_info(&pdev->dev, "secure-monitor device not ready, probe later\n");
+ return -EPROBE_DEFER;
+ }
+
+ ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo.raw);
+ if (ret < 0)
+ return ret;
+
+ if (!socinfo.raw) {
+ dev_err(&pdev->dev, "invalid regmap chipid value\n");
+ return -EINVAL;
+ }
+
+ soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
+ GFP_KERNEL);
+ if (!soc_dev_attr)
+ return -ENOMEM;
+
+ soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
+
+ soc_dev_attr->family = "Amlogic Meson";
+ soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
+ socinfo.v1.major_id,
+ socinfo.v1.chip_rev,
+ socinfo.v1.pack_id,
+ (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
+ soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
+ socinfo_v1_to_soc_id(socinfo),
+ socinfo_v1_to_package_id(socinfo));
+
+ soc_dev = soc_device_register(soc_dev_attr);
+
+
+ if (IS_ERR(soc_dev)) {
+ kfree(soc_dev_attr->revision);
+ kfree_const(soc_dev_attr->soc_id);
+ kfree(soc_dev_attr);
+ return PTR_ERR(soc_dev);
+ }
+
+ dev = soc_device_to_device(soc_dev);
+ platform_set_drvdata(pdev, soc_dev);
+
+ dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected (SM)\n",
+ soc_dev_attr->soc_id,
+ socinfo.v1.major_id,
+ socinfo.v1.chip_rev,
+ socinfo.v1.pack_id,
+ (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
+
+ return PTR_ERR_OR_ZERO(dev);
+}
+
+
+static int meson_gx_socinfo_sm_remove(struct platform_device *pdev)
+{
+ struct soc_device *soc_dev = platform_get_drvdata(pdev);
+
+ soc_device_unregister(soc_dev);
+ return 0;
+}
+
+static const struct of_device_id meson_gx_socinfo_match[] = {
+ { .compatible = "amlogic,meson-gx-ao-secure", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_gx_socinfo_match);
+
+static struct platform_driver meson_gx_socinfo_driver = {
+ .probe = meson_gx_socinfo_sm_probe,
+ .remove = meson_gx_socinfo_sm_remove,
+ .driver = {
+ .name = "meson-gx-socinfo-sm",
+ .of_match_table = meson_gx_socinfo_match,
+ },
+};
+
+
+module_platform_driver(meson_gx_socinfo_driver);
+
+MODULE_AUTHOR("Viacheslav Bocharov <[email protected]>");
+MODULE_DESCRIPTION("Amlogic Meson GX SOC SM driver");
+MODULE_LICENSE("GPL");
--
2.43.2
Move common constants and inline functions from meson-gx-socinfo driver
to header file. Create new structures for store meson64 cpu_id and chip_id.
Signed-off-by: Viacheslav Bocharov <[email protected]>
---
.../soc/amlogic/meson-gx-socinfo-internal.h | 120 ++++++++++++++++
drivers/soc/amlogic/meson-gx-socinfo.c | 136 ++----------------
2 files changed, 134 insertions(+), 122 deletions(-)
create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
diff --git a/drivers/soc/amlogic/meson-gx-socinfo-internal.h b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
new file mode 100644
index 000000000000..3ebb80972fc7
--- /dev/null
+++ b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
@@ -0,0 +1,120 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Copyright (c) 2024 JetHome
+ * Author: Neil Armstrong <[email protected]>
+ * Author: Viacheslav Bocharov <[email protected]>
+ *
+ */
+
+#ifndef _MESON_GX_SOCINFO_INTERNAL_H_
+#define _MESON_GX_SOCINFO_INTERNAL_H_
+
+#include <linux/types.h>
+
+#define AO_SEC_SD_CFG8 0xe0
+#define AO_SEC_SOCINFO_OFFSET AO_SEC_SD_CFG8
+
+union meson_cpu_id {
+ struct { // cpu_id v1
+ u32 layout_ver:4;
+ u32 reserved:4;
+ u32 chip_rev:8;
+ u32 pack_id:8;
+ u32 major_id:8;
+ } v1;
+ struct { // cpu_id v2
+ u32 major_id:8;
+ u32 chip_rev:8;
+ u32 pack_id:8;
+ u32 reserved:4;
+ u32 layout_ver:4;
+ } v2;
+ u32 raw;
+};
+
+struct meson_sm_chip_id {
+ u32 version;
+ union meson_cpu_id cpu_id;
+ u8 serial[12];
+};
+
+static const struct meson_gx_soc_id {
+ const char *name;
+ unsigned int id;
+} soc_ids[] = {
+ { "GXBB", 0x1f },
+ { "GXTVBB", 0x20 },
+ { "GXL", 0x21 },
+ { "GXM", 0x22 },
+ { "TXL", 0x23 },
+ { "TXLX", 0x24 },
+ { "AXG", 0x25 },
+ { "GXLX", 0x26 },
+ { "TXHD", 0x27 },
+ { "G12A", 0x28 },
+ { "G12B", 0x29 },
+ { "SM1", 0x2b },
+ { "A1", 0x2c },
+};
+
+static const struct meson_gx_package_id {
+ const char *name;
+ unsigned int major_id;
+ unsigned int pack_id;
+ unsigned int pack_mask;
+} soc_packages[] = {
+ { "S905", 0x1f, 0, 0x20 }, /* pack_id != 0x20 */
+ { "S905H", 0x1f, 0x3, 0xf }, /* pack_id & 0xf == 0x3 */
+ { "S905M", 0x1f, 0x20, 0xf0 }, /* pack_id == 0x20 */
+ { "S905D", 0x21, 0, 0xf0 },
+ { "S905X", 0x21, 0x80, 0xf0 },
+ { "S905W", 0x21, 0xa0, 0xf0 },
+ { "S905L", 0x21, 0xc0, 0xf0 },
+ { "S905M2", 0x21, 0xe0, 0xf0 },
+ { "S805X", 0x21, 0x30, 0xf0 },
+ { "S805Y", 0x21, 0xb0, 0xf0 },
+ { "S912", 0x22, 0, 0x0 }, /* Only S912 is known for GXM */
+ { "962X", 0x24, 0x10, 0xf0 },
+ { "962E", 0x24, 0x20, 0xf0 },
+ { "A113X", 0x25, 0x37, 0xff },
+ { "A113D", 0x25, 0x22, 0xff },
+ { "S905D2", 0x28, 0x10, 0xf0 },
+ { "S905Y2", 0x28, 0x30, 0xf0 },
+ { "S905X2", 0x28, 0x40, 0xf0 },
+ { "A311D", 0x29, 0x10, 0xf0 },
+ { "S922X", 0x29, 0x40, 0xf0 },
+ { "S905D3", 0x2b, 0x4, 0xf5 },
+ { "S905X3", 0x2b, 0x5, 0xf5 },
+ { "S905X3", 0x2b, 0x10, 0x3f },
+ { "S905D3", 0x2b, 0x30, 0x3f },
+ { "A113L", 0x2c, 0x0, 0xf8 },
+};
+
+static inline const char *socinfo_v1_to_package_id(union meson_cpu_id socinfo)
+{
+ int i;
+
+ for (i = 0 ; i < ARRAY_SIZE(soc_packages) ; ++i) {
+ if (soc_packages[i].major_id == socinfo.v1.major_id &&
+ soc_packages[i].pack_id ==
+ (socinfo.v1.pack_id & soc_packages[i].pack_mask))
+ return soc_packages[i].name;
+ }
+
+ return "Unknown";
+}
+
+static inline const char *socinfo_v1_to_soc_id(union meson_cpu_id socinfo)
+{
+ int i;
+
+ for (i = 0 ; i < ARRAY_SIZE(soc_ids) ; ++i) {
+ if (soc_ids[i].id == socinfo.v1.major_id)
+ return soc_ids[i].name;
+ }
+
+ return "Unknown";
+}
+
+#endif /* _MESON_GX_SOCINFO_INTERNAL_H_ */
diff --git a/drivers/soc/amlogic/meson-gx-socinfo.c b/drivers/soc/amlogic/meson-gx-socinfo.c
index 6abb730344ab..006f3b09940d 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo.c
+++ b/drivers/soc/amlogic/meson-gx-socinfo.c
@@ -12,118 +12,10 @@
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/sys_soc.h>
-#include <linux/bitfield.h>
#include <linux/regmap.h>
#include <linux/mfd/syscon.h>
-#define AO_SEC_SD_CFG8 0xe0
-#define AO_SEC_SOCINFO_OFFSET AO_SEC_SD_CFG8
-
-#define SOCINFO_MAJOR GENMASK(31, 24)
-#define SOCINFO_PACK GENMASK(23, 16)
-#define SOCINFO_MINOR GENMASK(15, 8)
-#define SOCINFO_MISC GENMASK(7, 0)
-
-static const struct meson_gx_soc_id {
- const char *name;
- unsigned int id;
-} soc_ids[] = {
- { "GXBB", 0x1f },
- { "GXTVBB", 0x20 },
- { "GXL", 0x21 },
- { "GXM", 0x22 },
- { "TXL", 0x23 },
- { "TXLX", 0x24 },
- { "AXG", 0x25 },
- { "GXLX", 0x26 },
- { "TXHD", 0x27 },
- { "G12A", 0x28 },
- { "G12B", 0x29 },
- { "SM1", 0x2b },
- { "A1", 0x2c },
-};
-
-static const struct meson_gx_package_id {
- const char *name;
- unsigned int major_id;
- unsigned int pack_id;
- unsigned int pack_mask;
-} soc_packages[] = {
- { "S905", 0x1f, 0, 0x20 }, /* pack_id != 0x20 */
- { "S905H", 0x1f, 0x3, 0xf }, /* pack_id & 0xf == 0x3 */
- { "S905M", 0x1f, 0x20, 0xf0 }, /* pack_id == 0x20 */
- { "S905D", 0x21, 0, 0xf0 },
- { "S905X", 0x21, 0x80, 0xf0 },
- { "S905W", 0x21, 0xa0, 0xf0 },
- { "S905L", 0x21, 0xc0, 0xf0 },
- { "S905M2", 0x21, 0xe0, 0xf0 },
- { "S805X", 0x21, 0x30, 0xf0 },
- { "S805Y", 0x21, 0xb0, 0xf0 },
- { "S912", 0x22, 0, 0x0 }, /* Only S912 is known for GXM */
- { "962X", 0x24, 0x10, 0xf0 },
- { "962E", 0x24, 0x20, 0xf0 },
- { "A113X", 0x25, 0x37, 0xff },
- { "A113D", 0x25, 0x22, 0xff },
- { "S905D2", 0x28, 0x10, 0xf0 },
- { "S905Y2", 0x28, 0x30, 0xf0 },
- { "S905X2", 0x28, 0x40, 0xf0 },
- { "A311D", 0x29, 0x10, 0xf0 },
- { "S922X", 0x29, 0x40, 0xf0 },
- { "S905D3", 0x2b, 0x4, 0xf5 },
- { "S905X3", 0x2b, 0x5, 0xf5 },
- { "S905X3", 0x2b, 0x10, 0x3f },
- { "S905D3", 0x2b, 0x30, 0x3f },
- { "A113L", 0x2c, 0x0, 0xf8 },
-};
-
-static inline unsigned int socinfo_to_major(u32 socinfo)
-{
- return FIELD_GET(SOCINFO_MAJOR, socinfo);
-}
-
-static inline unsigned int socinfo_to_minor(u32 socinfo)
-{
- return FIELD_GET(SOCINFO_MINOR, socinfo);
-}
-
-static inline unsigned int socinfo_to_pack(u32 socinfo)
-{
- return FIELD_GET(SOCINFO_PACK, socinfo);
-}
-
-static inline unsigned int socinfo_to_misc(u32 socinfo)
-{
- return FIELD_GET(SOCINFO_MISC, socinfo);
-}
-
-static const char *socinfo_to_package_id(u32 socinfo)
-{
- unsigned int pack = socinfo_to_pack(socinfo);
- unsigned int major = socinfo_to_major(socinfo);
- int i;
-
- for (i = 0 ; i < ARRAY_SIZE(soc_packages) ; ++i) {
- if (soc_packages[i].major_id == major &&
- soc_packages[i].pack_id ==
- (pack & soc_packages[i].pack_mask))
- return soc_packages[i].name;
- }
-
- return "Unknown";
-}
-
-static const char *socinfo_to_soc_id(u32 socinfo)
-{
- unsigned int id = socinfo_to_major(socinfo);
- int i;
-
- for (i = 0 ; i < ARRAY_SIZE(soc_ids) ; ++i) {
- if (soc_ids[i].id == id)
- return soc_ids[i].name;
- }
-
- return "Unknown";
-}
+#include "meson-gx-socinfo-internal.h"
static int __init meson_gx_socinfo_init(void)
{
@@ -131,7 +23,7 @@ static int __init meson_gx_socinfo_init(void)
struct soc_device *soc_dev;
struct device_node *np;
struct regmap *regmap;
- unsigned int socinfo;
+ union meson_cpu_id socinfo;
struct device *dev;
int ret;
@@ -160,11 +52,11 @@ static int __init meson_gx_socinfo_init(void)
return -ENODEV;
}
- ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo);
+ ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo.raw);
if (ret < 0)
return ret;
- if (!socinfo) {
+ if (!socinfo.raw) {
pr_err("%s: invalid chipid value\n", __func__);
return -EINVAL;
}
@@ -175,13 +67,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.v1.major_id,
+ socinfo.v1.chip_rev,
+ socinfo.v1.pack_id,
+ (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
- socinfo_to_soc_id(socinfo),
- socinfo_to_package_id(socinfo));
+ socinfo_v1_to_soc_id(socinfo),
+ socinfo_v1_to_package_id(socinfo));
soc_dev = soc_device_register(soc_dev_attr);
if (IS_ERR(soc_dev)) {
@@ -194,10 +86,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.v1.major_id,
+ socinfo.v1.chip_rev,
+ socinfo.v1.pack_id,
+ (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
return 0;
}
--
2.43.2
On 14/03/2024 07:59, Viacheslav Bocharov wrote:
> Amlogic SoCs have a SoC information secure-monitor call for SoC type,
> package type, revision information and chipid.
> This patchs adds support for secure-monitor call decoding and exposing
> with the SoC bus infrastructure in addition to the previous SoC
> Information driver.
>
> Signed-off-by: Viacheslav Bocharov <[email protected]>
> ---
> drivers/soc/amlogic/Kconfig | 10 ++
> drivers/soc/amlogic/Makefile | 1 +
> drivers/soc/amlogic/meson-gx-socinfo-sm.c | 192 ++++++++++++++++++++++
> 3 files changed, 203 insertions(+)
> create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
>
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index d08e398bdad4..82fc77ca3b4b 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -26,6 +26,16 @@ config MESON_GX_SOCINFO
> Say yes to support decoding of Amlogic Meson GX SoC family
> information about the type, package and version.
>
> +config MESON_GX_SOCINFO_SM
> + bool "Amlogic Meson GX SoC Information driver via Secure Monitor"
> + depends on (ARM64 && ARCH_MESON || COMPILE_TEST) && MESON_SM=y
> + default ARCH_MESON && MESON_SM
> + select SOC_BUS
> + help
> + Say yes to support decoding of Amlogic Meson GX SoC family
> + information about the type, package and version via secure
> + monitor call.
> +
I wonder, why do you need socinfo driver per each SoC? Usually it is one
or two per entire arch.
> +
> +static int meson_gx_socinfo_sm_probe(struct platform_device *pdev)
> +{
> + struct soc_device_attribute *soc_dev_attr;
> + struct soc_device *soc_dev;
> + struct device_node *sm_np;
> + struct meson_sm_firmware *fw;
> + struct regmap *regmap;
> + union meson_cpu_id socinfo;
> + struct device *dev;
> + int ret;
> +
> + /* check if chip-id is available */
> + if (!of_property_read_bool(pdev->dev.of_node, "amlogic,has-chip-id"))
> + return -ENODEV;
> +
> + /* node should be a syscon */
> + regmap = syscon_node_to_regmap(pdev->dev.of_node);
> + if (IS_ERR(regmap)) {
> + dev_err(&pdev->dev, "failed to get regmap\n");
Syntax is:
return dev_err_probe()
> + return -ENODEV;
Anyway wrong return code, use the real one you got.
> + }
> +
> + sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
> + if (!sm_np) {
> + dev_err(&pdev->dev, "no secure-monitor node found\n");
> + return -ENODEV;
-EINVAL
> + }
> +
> + fw = meson_sm_get(sm_np);
> + of_node_put(sm_np);
> + if (!fw) {
> + dev_info(&pdev->dev, "secure-monitor device not ready, probe later\n");
No, you never print messages on deferred probe.
> + return -EPROBE_DEFER;
> + }
> +
> + ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo.raw);
> + if (ret < 0)
> + return ret;
> +
> + if (!socinfo.raw) {
> + dev_err(&pdev->dev, "invalid regmap chipid value\n");
> + return -EINVAL;
> + }
> +
> + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
> + GFP_KERNEL);
> + if (!soc_dev_attr)
> + return -ENOMEM;
> +
> + soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
> +
> + soc_dev_attr->family = "Amlogic Meson";
> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
> + socinfo.v1.major_id,
> + socinfo.v1.chip_rev,
> + socinfo.v1.pack_id,
> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
> + socinfo_v1_to_soc_id(socinfo),
> + socinfo_v1_to_package_id(socinfo));
> +
> + soc_dev = soc_device_register(soc_dev_attr);
> +
> +
> + if (IS_ERR(soc_dev)) {
> + kfree(soc_dev_attr->revision);
> + kfree_const(soc_dev_attr->soc_id);
> + kfree(soc_dev_attr);
That's a double free. This was not tested.
> + return PTR_ERR(soc_dev);
> + }
> +
> + dev = soc_device_to_device(soc_dev);
> + platform_set_drvdata(pdev, soc_dev);
> +
> + dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected (SM)\n",
> + soc_dev_attr->soc_id,
> + socinfo.v1.major_id,
> + socinfo.v1.chip_rev,
> + socinfo.v1.pack_id,
> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
> +
> + return PTR_ERR_OR_ZERO(dev);
> +}
> +
> +
> +static int meson_gx_socinfo_sm_remove(struct platform_device *pdev)
> +{
> + struct soc_device *soc_dev = platform_get_drvdata(pdev);
> +
> + soc_device_unregister(soc_dev);
If you free the memory in probe() error path, why you did not decide to
free it here as well? It is symmetrical, so this should make you wonder
- error path is wrong.
Best regards,
Krzysztof
On Thu, Mar 14, 2024 at 09:59:50AM +0300, Viacheslav Bocharov wrote:
> 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.
>
How old or new are these SoCs ? The reason I ask is that it is really
sad to see vendors still creating their custom interfaces for such things
despite the standard SMCCC interface SOC_ID introduced in SMCCC v1.2 some
time in 2020.
Hopefully they migrated to the std interface and just use the driver in
the kernel without needing to add this every time they fancy playing
with the interface for no reason.
--
Regards,
Sudeep
Hi!
Thanks for review.
14/03/2024 10.11, Krzysztof Kozlowski wrote:
> On 14/03/2024 07:59, Viacheslav Bocharov wrote:
>> Amlogic SoCs have a SoC information secure-monitor call for SoC type,
>> package type, revision information and chipid.
>> This patchs adds support for secure-monitor call decoding and exposing
>> with the SoC bus infrastructure in addition to the previous SoC
>> Information driver.
>>
>> Signed-off-by: Viacheslav Bocharov <[email protected]>
>> ---
>> drivers/soc/amlogic/Kconfig | 10 ++
>> drivers/soc/amlogic/Makefile | 1 +
>> drivers/soc/amlogic/meson-gx-socinfo-sm.c | 192 ++++++++++++++++++++++
>> 3 files changed, 203 insertions(+)
>> create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
>>
>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
>> index d08e398bdad4..82fc77ca3b4b 100644
>> --- a/drivers/soc/amlogic/Kconfig
>> +++ b/drivers/soc/amlogic/Kconfig
>> @@ -26,6 +26,16 @@ config MESON_GX_SOCINFO
>> Say yes to support decoding of Amlogic Meson GX SoC family
>> information about the type, package and version.
>>
>> +config MESON_GX_SOCINFO_SM
>> + bool "Amlogic Meson GX SoC Information driver via Secure Monitor"
>> + depends on (ARM64 && ARCH_MESON || COMPILE_TEST) && MESON_SM=y
>> + default ARCH_MESON && MESON_SM
>> + select SOC_BUS
>> + help
>> + Say yes to support decoding of Amlogic Meson GX SoC family
>> + information about the type, package and version via secure
>> + monitor call.
>> +
>
> I wonder, why do you need socinfo driver per each SoC? Usually it is one
> or two per entire arch.
We use this driver for GX and almost all newer SoCs from AmLogic
(similar to the original meson-gx-socinfo).
In the fourth patch, this driver is specifically enabled for the GX, G12
(g12a, g12b, sm1), AXG, A1 families.
>
>> +
>> +static int meson_gx_socinfo_sm_probe(struct platform_device *pdev)
>> +{
>> + struct soc_device_attribute *soc_dev_attr;
>> + struct soc_device *soc_dev;
>> + struct device_node *sm_np;
>> + struct meson_sm_firmware *fw;
>> + struct regmap *regmap;
>> + union meson_cpu_id socinfo;
>> + struct device *dev;
>> + int ret;
>> +
>> + /* check if chip-id is available */
>> + if (!of_property_read_bool(pdev->dev.of_node, "amlogic,has-chip-id"))
>> + return -ENODEV;
>> +
>> + /* node should be a syscon */
>> + regmap = syscon_node_to_regmap(pdev->dev.of_node);
>> + if (IS_ERR(regmap)) {
>> + dev_err(&pdev->dev, "failed to get regmap\n");
>
> Syntax is:
>
> return dev_err_probe()
>
>> + return -ENODEV;
>
> Anyway wrong return code, use the real one you got.
>
Thanks!
I'l fix all dev_err&return calls to this helper.
>> + }
>> +
>> + sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
>> + if (!sm_np) {
>> + dev_err(&pdev->dev, "no secure-monitor node found\n");
>> + return -ENODEV;
>
> -EINVAL
Fixed.
>
>> + }
>> +
>> + fw = meson_sm_get(sm_np);
>> + of_node_put(sm_np);
>> + if (!fw) {
>> + dev_info(&pdev->dev, "secure-monitor device not ready, probe later\n");
>
> No, you never print messages on deferred probe.
Fixed to dev_dbg like in dev_err_probe.
>
>> + return -EPROBE_DEFER;
>> + }
>> +
>> + ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo.raw);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (!socinfo.raw) {
>> + dev_err(&pdev->dev, "invalid regmap chipid value\n");
>> + return -EINVAL;
>> + }
>> +
>> + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>> + GFP_KERNEL);
>> + if (!soc_dev_attr)
>> + return -ENOMEM;
>> +
>> + soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
>> +
>> + soc_dev_attr->family = "Amlogic Meson";
>> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
>> + socinfo.v1.major_id,
>> + socinfo.v1.chip_rev,
>> + socinfo.v1.pack_id,
>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
>> + socinfo_v1_to_soc_id(socinfo),
>> + socinfo_v1_to_package_id(socinfo));
>> +
>> + soc_dev = soc_device_register(soc_dev_attr);
>> +
>> +
>> + if (IS_ERR(soc_dev)) {
>> + kfree(soc_dev_attr->revision);
>> + kfree_const(soc_dev_attr->soc_id);
>> + kfree(soc_dev_attr);
>
> That's a double free. This was not tested.
Please, describe the problem.
I don't quite understand what the issue is:
- kfree() releases memory allocated with kmalloc()
- kasprintf() allocates memory using kmalloc_track_caller()
Technically, I see no difficulty in freeing the newly allocated memory.
In case of memory allocation issues in kasprintf, we would just get
NULL, which kfree should also handle properly. Considering that we don't
need soc_dev_attr anymore, we don't need to worry about the contents of
revision and .soc_id.
I see that kfree_const has crept in by accident, which is essentially
needed here only if we replace kasprintf with kasprintf_const for
soc_id, but it does not introduce any erroneous behavior.
>
>> + return PTR_ERR(soc_dev);
>> + }
>> +
>> + dev = soc_device_to_device(soc_dev);
>> + platform_set_drvdata(pdev, soc_dev);
>> +
>> + dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected (SM)\n",
>> + soc_dev_attr->soc_id,
>> + socinfo.v1.major_id,
>> + socinfo.v1.chip_rev,
>> + socinfo.v1.pack_id,
>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>> +
>> + return PTR_ERR_OR_ZERO(dev);
>> +}
>> +
>> +
>> +static int meson_gx_socinfo_sm_remove(struct platform_device *pdev)
>> +{
>> + struct soc_device *soc_dev = platform_get_drvdata(pdev);
>> +
>> + soc_device_unregister(soc_dev);
>
> If you free the memory in probe() error path, why you did not decide to
> free it here as well? It is symmetrical, so this should make you wonder
> - error path is wrong.
This is something I can easily explain:
In the case where we have successfully registered with
soc_device_register, we clean up everything that was manually allocated
and not used.
In the case of unloading the driver, the cleanup should be handled by
the soc_device_unregister command.
Technically, it's not possible to insert memory release because until
the command is called, the driver is active, and afterwards, there's no
guarantee of the pointer's validity.
Perhaps it would have been better if soc_device_register copied the
entire soc_device_attribute structure and took care of memory allocation
and release itself, then we could comfortably free any excess memory
back in _probe.
Are you currently recommending not to release memory within the if
(IS_ERR(soc_dev)) section?
--
Best regards
Viacheslav
Hi!
14/03/2024 13.40, Sudeep Holla wrote:
> On Thu, Mar 14, 2024 at 09:59:50AM +0300, Viacheslav Bocharov wrote:
>> 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.
>>
>
> How old or new are these SoCs ? The reason I ask is that it is really
> sad to see vendors still creating their custom interfaces for such things
> despite the standard SMCCC interface SOC_ID introduced in SMCCC v1.2 some
> time in 2020.
Most of these SoC were created before 2020.
>
> Hopefully they migrated to the std interface and just use the driver in
> the kernel without needing to add this every time they fancy playing
> with the interface for no reason.
>
--
Best regards
Viacheslav
On 14/03/2024 13:22, Viacheslav wrote:
>> +
>>> + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>>> + GFP_KERNEL);
>>> + if (!soc_dev_attr)
>>> + return -ENOMEM;
>>> +
>>> + soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
>>> +
>>> + soc_dev_attr->family = "Amlogic Meson";
>>> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
>>> + socinfo.v1.major_id,
>>> + socinfo.v1.chip_rev,
>>> + socinfo.v1.pack_id,
>>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>>> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
>>> + socinfo_v1_to_soc_id(socinfo),
>>> + socinfo_v1_to_package_id(socinfo));
>>> +
>>> + soc_dev = soc_device_register(soc_dev_attr);
>>> +
>>> +
>>> + if (IS_ERR(soc_dev)) {
>>> + kfree(soc_dev_attr->revision);
>>> + kfree_const(soc_dev_attr->soc_id);
>>> + kfree(soc_dev_attr);
>>
>> That's a double free. This was not tested.
>
>
> Please, describe the problem.
Test your code. What's the point of arguing over it if regular test
would show this?
> I don't quite understand what the issue is:
>
> - kfree() releases memory allocated with kmalloc()
So point me where is kmalloc(). I don't see. I see only devm.
> - kasprintf() allocates memory using kmalloc_track_caller()
>
> Technically, I see no difficulty in freeing the newly allocated memory.
> In case of memory allocation issues in kasprintf, we would just get
> NULL, which kfree should also handle properly. Considering that we don't
> need soc_dev_attr anymore, we don't need to worry about the contents of
> .revision and .soc_id.
Please pay attention that my comment is under specific line. We do not
discuss unrelated code.
>
> I see that kfree_const has crept in by accident, which is essentially
> needed here only if we replace kasprintf with kasprintf_const for
> .soc_id, but it does not introduce any erroneous behavior.
>
>>
>>> + return PTR_ERR(soc_dev);
>>> + }
>>> +
>>> + dev = soc_device_to_device(soc_dev);
>>> + platform_set_drvdata(pdev, soc_dev);
>>> +
>>> + dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected (SM)\n",
>>> + soc_dev_attr->soc_id,
>>> + socinfo.v1.major_id,
>>> + socinfo.v1.chip_rev,
>>> + socinfo.v1.pack_id,
>>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>>> +
>>> + return PTR_ERR_OR_ZERO(dev);
>>> +}
>>> +
>>> +
>>> +static int meson_gx_socinfo_sm_remove(struct platform_device *pdev)
>>> +{
>>> + struct soc_device *soc_dev = platform_get_drvdata(pdev);
>>> +
>>> + soc_device_unregister(soc_dev);
>>
>> If you free the memory in probe() error path, why you did not decide to
>> free it here as well? It is symmetrical, so this should make you wonder
>> - error path is wrong.
>
> This is something I can easily explain:
>
> In the case where we have successfully registered with
> soc_device_register, we clean up everything that was manually allocated
> and not used.
> In the case of unloading the driver, the cleanup should be handled by
> the soc_device_unregister command.
>
> Technically, it's not possible to insert memory release because until
> the command is called, the driver is active, and afterwards, there's no
> guarantee of the pointer's validity.
Then you do not understand lifecycle of device. There is release here
via devm. Exactly at after my comment, when } finishes.
> Perhaps it would have been better if soc_device_register copied the
> entire soc_device_attribute structure and took care of memory allocation
> and release itself, then we could comfortably free any excess memory
> back in _probe.
>
> Are you currently recommending not to release memory within the if
> (IS_ERR(soc_dev)) section?
You have double free which will be pointed out by testing. Yes, of
course I recommend not to have double free, so not to release memory
which is being released.
Best regards,
Krzysztof
14/03/2024 16.31, Krzysztof Kozlowski wrote:
> On 14/03/2024 13:22, Viacheslav wrote:
>>> +
>>>> + soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
>>>> + GFP_KERNEL);
>>>> + if (!soc_dev_attr)
>>>> + return -ENOMEM;
>>>> +
>>>> + soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
>>>> +
>>>> + soc_dev_attr->family = "Amlogic Meson";
>>>> + soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
>>>> + socinfo.v1.major_id,
>>>> + socinfo.v1.chip_rev,
>>>> + socinfo.v1.pack_id,
>>>> + (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
>>>> + soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
>>>> + socinfo_v1_to_soc_id(socinfo),
>>>> + socinfo_v1_to_package_id(socinfo));
>>>> +
>>>> + soc_dev = soc_device_register(soc_dev_attr);
>>>> +
>>>> +
>>>> + if (IS_ERR(soc_dev)) {
>>>> + kfree(soc_dev_attr->revision);
>>>> + kfree_const(soc_dev_attr->soc_id);
>>>> + kfree(soc_dev_attr);
>>>
>>> That's a double free. This was not tested.
>>
>>
>> Please, describe the problem.
>
> Test your code. What's the point of arguing over it if regular test
> would show this?
>
>> I don't quite understand what the issue is:
>>
>> - kfree() releases memory allocated with kmalloc()
>
> So point me where is kmalloc(). I don't see. I see only devm.
I missed the point that devm_kzalloc is automatically freed. You are
right. Thanks!
--
Best regards
Viacheslav
On Thu, Mar 14, 2024 at 03:25:02PM +0300, Viacheslav wrote:
> Hi!
>
> 14/03/2024 13.40, Sudeep Holla wrote:
> > On Thu, Mar 14, 2024 at 09:59:50AM +0300, Viacheslav Bocharov wrote:
> > > 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.
> > >
> >
> > How old or new are these SoCs ? The reason I ask is that it is really
> > sad to see vendors still creating their custom interfaces for such things
> > despite the standard SMCCC interface SOC_ID introduced in SMCCC v1.2 some
> > time in 2020.
>
> Most of these SoC were created before 2020.
>
Fair enough then. Hope they use SOC_ID on newer SoCs.
--
Regards,
Sudeep