Re: [PATCH RESEND v8 04/10] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Commit message claims to add spmi_subdevice_put_and_remove() but it is missing
- [Medium] Missing reference count on SPMI controller causes potential Use-After-Free (UAF)
- [Low] Format string type mismatch for controller number in device name
--

commit 481d7c4794f3b580a0e0d422d1a5cc8b6a3b1466
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

This commit introduces spmi_subdevice_alloc_and_add() and a devres variant
to manage complex sub-devices on the SPMI bus. It allows PMIC components
sharing a single SID to be represented as sub-devices. The new exports
are placed in the "SPMI" namespace.

> Add new functions aimed at managing sub-devices of a SPMI device
> spmi_subdevice_alloc_and_add() and a spmi_subdevice_put_and_remove()

This isn't a bug, but the commit message mentions
spmi_subdevice_put_and_remove(), while the implemented function is
actually named spmi_subdevice_remove(). Should the commit message be
updated to match the code?

> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index 91a40ea8031ad..cdf3e99194c4c 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c

[ ... ]

> +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(sizeof(*sub_sdev), GFP_KERNEL);
> +	if (!sub_sdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sdev = &sub_sdev->sdev;
> +	sdev->ctrl = sparent->ctrl;

Does this missing reference count on the controller introduce a potential
use-after-free regression?

If the parent PMIC device is removed and drops its reference to the controller,
could the subdevice outlive it and still hold a dangling pointer to
sdev->ctrl? Should this initialization take a reference via
get_device(&sdev->ctrl->dev)?

> +	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, "%d-%02x.%d.auto",
> +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);

This isn't a bug, but could the format specifier for sdev->ctrl->nr
cause a type mismatch warning? Looking at struct spmi_controller, nr is
an unsigned int, so should this use %u instead of %d?

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

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

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