Re: [PATCH v11 07/12] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The driver incorrectly passes `dev->parent` to `spmi_get_parent_spmi_device()`, causing unconditional probe failure.
- [High] Double free of `of_node` reference in `of_spmi_register_devices()` error path.

Pre-existing issues:
- [High] The NVMEM device size strictly limits accessible offsets, completely blocking access to the upper half of the SDAM memory.
--

--- Patch [7]: [PATCH v11 07/12] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add() ---
commit 35c63d844ba2cb90d68e57b63eabc7db7d3178aa
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

Some Qualcomm PMICs integrate a SDAM device, internally located in a specific
address range reachable through SPMI communication. Instead of using the parent
SPMI device as a syscon, this patch registers a new SPMI sub-device for SDAM
and initializes its own regmap. This avoids manually adding the register base
address to every R/W call.

> diff --git a/drivers/nvmem/qcom-spmi-sdam.c b/drivers/nvmem/qcom-spmi-sdam.c
> index 4f1cca6eab71e..ad5ae47ce4b0e 100644
> --- a/drivers/nvmem/qcom-spmi-sdam.c
> +++ b/drivers/nvmem/qcom-spmi-sdam.c
[ ... ]
> @@ -110,8 +110,17 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
>  
>  static int sdam_probe(struct platform_device *pdev)
>  {
> +	struct regmap_config sdam_regmap_config = {
> +		.reg_bits = 16,
> +		.val_bits = 8,
> +		.max_register = 0xff,
> +		.fast_io = true,
> +	};
>  	struct sdam_chip *sdam;
>  	struct nvmem_device *nvmem;
> +	struct spmi_device *sparent;
> +	struct spmi_subdevice *sub_sdev;
> +	struct device *dev = &pdev->dev;
>  	unsigned int val;
>  	int rc;
>  
> @@ -119,19 +128,26 @@ static int sdam_probe(struct platform_device *pdev)
>  	if (!sdam)
>  		return -ENOMEM;
>  
> -	sdam->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> -	if (!sdam->regmap) {
> -		dev_err(&pdev->dev, "Failed to get regmap handle\n");
> -		return -ENXIO;
> -	}
> +	sparent = spmi_get_parent_spmi_device(dev->parent);

[Severity: High]
Does this incorrectly pass the parent device instead of the current device,
triggering a type check failure?

If we look at spmi_get_parent_spmi_device():

drivers/spmi/spmi.c:spmi_get_parent_spmi_device() {
	if (parent->type == &spmi_dev_type)
		return to_spmi_device(parent);
	...
}

The helper checks the type of the parent of the passed device. When passed the
PMIC (dev->parent), it checks the SPMI controller, which will fail and return
NULL, causing the probe to fail unconditionally.

> +	if (!sparent)
> +		return -ENODEV;
>  
> -	rc = of_property_read_u32(pdev->dev.of_node, "reg", &sdam->base);
> +	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
> +	if (IS_ERR(sub_sdev))
> +		return PTR_ERR(sub_sdev);
> +
> +	rc = device_property_read_u32(dev, "reg", &sdam_regmap_config.reg_base);
>  	if (rc < 0) {
> -		dev_err(&pdev->dev, "Failed to get SDAM base, rc=%d\n", rc);
> +		dev_err(dev, "Failed to get SDAM base, rc=%d\n", rc);
>  		return -EINVAL;
>  	}
>  
> -	rc = regmap_read(sdam->regmap, sdam->base + SDAM_SIZE, &val);
> +	sdam->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &sdam_regmap_config);
> +	if (IS_ERR(sdam->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sdam->regmap),
> +				     "Failed to get regmap handle\n");
> +
> +	rc = regmap_read(sdam->regmap, SDAM_SIZE, &val);
>  	if (rc < 0) {
>  		dev_err(&pdev->dev, "Failed to read SDAM_SIZE rc=%d\n", rc);
>  		return -EINVAL;
>  	}

[Severity: High]
This is a pre-existing issue, but does limiting the NVMEM device size block
access to the upper half of the SDAM memory?

Just below this line, the driver sets sdam->size = val * 32, which limits the
NVMEM framework's maximum valid offset to the raw memory size. The NVMEM core
strictly enforces that offset + bytes <= nvmem->size.

However, if we look at sdam_is_valid():

drivers/nvmem/qcom-spmi-sdam.c:sdam_is_valid() {
	if (offset >= SDAM_MEM_START && offset <= sdam_mem_end
			&& (offset + len - 1) <= sdam_mem_end)
		...
}

It strictly requires offset to be at least SDAM_MEM_START (64 bytes).
Because the framework caps the offset at 128 bytes, a consumer can only
request offsets between 64 and 127, rendering the upper 64 bytes
physically inaccessible.

[ ... ]

Additionally, there appears to be an issue in the SPMI core that warrants
review:

[Severity: High]
Does the error path in of_spmi_register_devices() double-free the of_node
reference?

When spmi_device_add() fails, the error path explicitly calls
of_node_put(node):

drivers/spmi/spmi.c:of_spmi_register_devices() {
	...
	dev_err(&sdev->dev, "failure adding device. status %pe\n", ERR_PTR(err));
	of_node_put(node);
	spmi_device_put(sdev);
	...
}

It then calls spmi_device_put(sdev), which drops the device reference count to
zero and invokes spmi_dev_release().

If we look at spmi_dev_release():

drivers/spmi/spmi.c:spmi_dev_release() {
	...
	if (IS_ENABLED(CONFIG_OF))
		of_node_put(dev->of_node);
	...
}

The release callback natively cleans up the of_node reference. Calling
of_node_put(node) just before spmi_device_put(sdev) causes a double-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707101821.173319-1-angelogioacchino.delregno@collabora.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy