Re: [PATCH v3 2/4] soc: amlogic: meson-gx-socinfo-sm: Add Amlogic secure-monitor SoC Information driver
From: Viacheslav
Date: Thu Mar 14 2024 - 08:23:26 EST
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 <adeep@xxxxxxxxx>
---
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