Re: [PATCH v10 05/11] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spmi_subdevice_alloc_and_add() fuses allocation and registration, preventing initialization of device properties (e.g., of_node, fwnode) before device discovery.
--

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

spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

This commit introduces new functions to allocate and register sub-devices
for SPMI devices, along with their devm_* equivalents. The exports are
placed under the "SPMI" namespace.

> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index cae89dd915d4a..cd047aba38ab1 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c
[ ... ]
> @@ -431,6 +457,58 @@ struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(spmi_device_alloc);
>  
> +/**
> + * spmi_subdevice_alloc_and_add(): Allocate and add a new SPMI sub-device
> + * @sparent:	SPMI parent device with previously registered SPMI controller
> + *
> + * Returns:
> + * Pointer to newly allocated SPMI sub-device for success or error pointer.
> + */
> +struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent)
> +{
> +	struct spmi_subdevice *sub_sdev;
> +	struct spmi_device *sdev;
> +	int ret;
[ ... ]
> +	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]
By combining allocation and registration into a single function, doesn't this
prevent the caller from initializing device properties like of_node or fwnode
before the device is discovered?

If device_add() is called immediately, the initial KOBJ_ADD uevent will be
emitted without a MODALIAS, which can break module autoloading. Furthermore,
fw_devlink might fail to correctly establish device links, and
spmi_device_match() is likely to unconditionally fail to find a matching driver
during the initial bus_probe_device() since of_node is NULL.

Would it be better to split this into separate alloc and add operations so the
caller can safely configure the device properties before adding it to the system?

> +	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);
> +}
> +EXPORT_SYMBOL_NS_GPL(spmi_subdevice_alloc_and_add, "SPMI");

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

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