Message-ID: <9292d2ee-138a-4a59-9c96-31c1b4473a4e@lexina.in>
Date: Thu, 14 Mar 2024 15:22:59 +0300
From: Viacheslav <adeep@...ina.in>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Neil Armstrong <neil.armstrong@...aro.org>,
 Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org
Cc: Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH v3 2/4] soc: amlogic: meson-gx-socinfo-sm: Add Amlogic
 secure-monitor SoC Information driver

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@...ina.in>
>> ---
>>   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

Powered by blists - more mailing lists