Re: [PATCH v10 06/11] 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 reference to the parent SPMI device acquired via `spmi_find_device_by_of_node()` is leaked on both success and error paths.
- [Low] Returning a pointer (`ERR_PTR`) from an integer-returning function causes a type mismatch and build warning.

Pre-existing issues:
- [High] The upper half of the SDAM memory cannot be accessed via the nvmem API due to a bound restriction mismatch.
--

--- Patch [6]: [PATCH v10 06/11] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add() ---
commit 8fe2831bc2e512389dd631ef16ab000bbe8f6706
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 (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for SDAM
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

> diff --git a/drivers/nvmem/qcom-spmi-sdam.c b/drivers/nvmem/qcom-spmi-sdam.c
> index 4f1cca6eab71e..3ad842676dde5 100644
> --- a/drivers/nvmem/qcom-spmi-sdam.c
> +++ b/drivers/nvmem/qcom-spmi-sdam.c

[ ... ]

> @@ -110,28 +110,47 @@ 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;
>  
> +	if (!dev->parent)
> +		return -ENODEV;
> +
>  	sdam = devm_kzalloc(&pdev->dev, sizeof(*sdam), GFP_KERNEL);
>  	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_find_device_by_of_node(dev->parent->of_node);
> +	if (!sparent)
> +		return ERR_PTR(-ENODEV);

[Severity: Low]
Does returning ERR_PTR(-ENODEV) here cause a compiler warning?

Since sdam_probe() returns an int, this implicitly casts a pointer to an
integer. This triggers a -Wint-conversion warning, which results in a build
failure on modern compiler configurations using -Werror=int-conversion.


[Severity: High]
Does this code leak the sparent device reference?

spmi_find_device_by_of_node() internally invokes bus_find_device_by_of_node(),
which returns a device with an incremented reference count via get_device().

I don't see a corresponding put_device(&sparent->dev) on the success or
error paths before returning from sdam_probe(). Because
devm_spmi_subdevice_alloc_and_add() registers the device but does not assume
ownership of this specific reference, the parent PMIC device object appears
to be permanently leaked on every probe.

> +
> +	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
> +	if (IS_ERR(sub_sdev))
> +		return PTR_ERR(sub_sdev);

[ ... ]

> @@ -159,7 +178,7 @@ static int sdam_probe(struct platform_device *pdev)
>  	}
>  	dev_dbg(&pdev->dev,
>  		"SDAM base=%#x size=%u registered successfully\n",
> -		sdam->base, sdam->size);
> +		sdam_regmap_config.reg_base, sdam->size);

[Severity: High]
This is a pre-existing issue, but does setting the nvmem framework size limit
to sdam->size earlier in this function restrict the maximum readable offsets
incorrectly?

The driver maps physical memory starting at SDAM_MEM_START (0x40), and
sdam_is_valid() mandates that the input offset must be >= 0x40.

However, configuring sdam->sdam_config.size = sdam->size forces nvmem
consumers to pass an offset strictly less than sdam->size. This permits
access to only a fraction of the intended size, completely blocking access
to the upper portion of the SDAM memory block which are valid physical
SDAM registers approved by sdam_is_valid().

>  
>  	return 0;
>  }

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

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