Re: [PATCH v11 05/12] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `spmi_subdevice_alloc_and_add()` merges device allocation and registration, preventing callers from safely setting device properties before driver matching.
- [Medium] Missing `of_node_put()` in `spmi_subdev_release()` leads to a device tree node reference count leak.
--

--- Patch [5]: [PATCH v11 05/12] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant ---
commit a8b1557eae1d7ee7a9490cb239a6b54f95758f0d
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

This commit adds new functions to allocate, add, and remove sub-devices of
an SPMI device. It introduces spmi_subdevice_alloc_and_add() along with
its devm_* variant to support complex PMICs with multiple sub-features.
The symbols are exported under a new "SPMI" namespace.

diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index 14ec9de58467a..16ffcb1370089 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -34,6 +35,19 @@ static const struct device_type spmi_dev_type = {

[ ... ]

> +static void spmi_subdev_release(struct device *dev)
> +{
> +	struct spmi_device *sdev = to_spmi_device(dev);
> +	struct spmi_subdevice *sub_sdev = container_of(sdev, struct spmi_subdevice, sdev);
> +
> +	ida_free(&spmi_subdevice_ida, sub_sdev->devid);
> +	kfree(sub_sdev);
> +}

[Severity: Medium]
Does this leak the device tree node reference count?

If a caller assigns an of_node to the subdevice (for example,
sdev->dev.of_node = of_node_get(child)), the standard spmi_dev_release()
handles cleanup by calling of_node_put(dev->of_node).

Should spmi_subdev_release() include this same cleanup when the device
reference count reaches zero upon unregistration?

[ ... ]

> +struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent)
> +{
> +	struct spmi_subdevice *sub_sdev;
> +	struct spmi_device *sdev;
> +	int ret;
> +
> +	sub_sdev = kzalloc_obj(*sub_sdev);
> +	if (!sub_sdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sdev = &sub_sdev->sdev;
> +	sdev->ctrl = sparent->ctrl;
> +	sdev->usid = sparent->usid;
> +
> +	ret = ida_alloc(&spmi_subdevice_ida, GFP_KERNEL);
> +	if (ret < 0) {
> +		kfree(sub_sdev);
> +		return ERR_PTR(ret);
> +	}
> +	sub_sdev->devid = ret;
> +
> +	device_initialize(&sdev->dev);
> +	sdev->dev.parent = &sparent->dev;
> +	sdev->dev.bus = &spmi_bus_type;
> +	sdev->dev.type = &spmi_subdev_type;
> +
> +	ret = dev_set_name(&sdev->dev, "%u-%02x.%u.auto",
> +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
> +	if (ret)
> +		goto err_put_dev;
> +
> +	ret = device_add(&sdev->dev);

[Severity: High]
Does combining allocation and registration into a single function prevent
callers from properly configuring the device before driver matching occurs?

Because device_add() emits a KOBJ_ADD uevent and triggers bus matching,
if a parent driver needs to assign an of_node or fwnode for matching,
it is forced to do so after this function returns.

Setting dev.of_node after registration results in the KOBJ_ADD uevent
lacking the OF_COMPATIBLE modalias, which breaks module autoloading and
causes the initial driver probe to fail.

Would it be safer to separate the allocation and addition steps so callers
have a window to set device properties?

> +	if (ret) {
> +		dev_err(&sdev->dev, "Can't add device, status %pe\n", ERR_PTR(ret));
> +		goto err_put_dev;
> +	}
> +
> +	return sub_sdev;
> +
> +err_put_dev:
> +	put_device(&sdev->dev);
> +	return ERR_PTR(ret);
> +}

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

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