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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/7/26 12:48, sashiko-bot@kernel.org wrote:
> 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).

The caller shall take care of its own resources refcounts, we can't assume in the
API that the caller did or did not do anything. There's no magic 8 ball to use.

> 
> 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?
> 

No. Simply no.

>> +	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);
>> +}
> 

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