linux-kernel - Re: [PATCH v7 04/10] spmi: Implement spmi_subdevice_alloc_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aWdZAyqQQ4KwCJx-@smile.fi.intel.com>
Date: Wed, 14 Jan 2026 10:51:15 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, arnd@...db.de, gregkh@...uxfoundation.org,
	srini@...nel.org, vkoul@...nel.org, neil.armstrong@...aro.org,
	sre@...nel.org, sboyd@...nel.org, krzk@...nel.org,
	dmitry.baryshkov@....qualcomm.com, quic_wcheng@...cinc.com,
	melody.olvera@....qualcomm.com, quic_nsekar@...cinc.com,
	ivo.ivanov.ivanov1@...il.com, abelvesa@...nel.org,
	luca.weiss@...rphone.com, konrad.dybcio@....qualcomm.com,
	mitltlatltl@...il.com, krishna.kurapati@....qualcomm.com,
	linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
	linux-pm@...r.kernel.org, kernel@...labora.com,
	Jonathan Cameron <jonathan.cameron@...wei.com>
Subject: Re: [PATCH v7 04/10] spmi: Implement spmi_subdevice_alloc_and_add()
 and devm variant

On Wed, Jan 14, 2026 at 09:39:51AM +0100, AngeloGioacchino Del Regno wrote:
> Some devices connected over the SPMI bus may be big, in the sense
> that those may be a complex of devices managed by a single chip
> over the SPMI bus, reachable through a single SID.
> 
> Add new functions aimed at managing sub-devices of a SPMI device
> spmi_subdevice_alloc_and_add() and a spmi_subdevice_put_and_remove()
> for adding a new subdevice and removing it respectively, and also
> add their devm_* variants.
> 
> The need for such functions comes from the existence of	those
> complex Power Management ICs (PMICs), which feature one or many
> sub-devices, in some cases with these being even addressable on
> the chip in form of SPMI register ranges.
> 
> Examples of those devices can be found in both Qualcomm platforms
> with their PMICs having PON, RTC, SDAM, GPIO controller, and other
> sub-devices, and in newer MediaTek platforms showing similar HW
> features and a similar layout with those also having many subdevs.
> 
> Also, instead of generally exporting symbols, export them with a
> new "SPMI" namespace: all users will have to import this namespace
> to make use of the newly introduced exports.

...

> +static void devm_spmi_subdevice_remove(void *res)

For better readability we usually call the variable with a meaningful name, and
sub_sdev here seems the best.

> +{
> +	spmi_subdevice_remove(res);
> +}

...

> + * Pointer to newly allocated SPMI sub-device for success or negative ERR_PTR.

This is "negative ERR_PTR" nonsense. Either "negative errno" or "error pointer".
Or something alike.

> +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);
> +
> +	ret = ida_alloc(&spmi_subdevice_ida, GFP_KERNEL);
> +	if (ret < 0) {
> +		kfree(sub_sdev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	sdev = &sub_sdev->sdev;
> +	sdev->ctrl = sparent->ctrl;
> +	device_initialize(&sdev->dev);
> +	sdev->dev.parent = &sparent->dev;
> +	sdev->dev.bus = &spmi_bus_type;
> +	sdev->dev.type = &spmi_subdev_type;

> +	sub_sdev->devid = ret;

Too far, may be prone to mistakes in the future. The rewritten one would look
like

	sub_sdev = kzalloc(sizeof(*sub_sdev), GFP_KERNEL);
	if (!sub_sdev)
		return ERR_PTR(-ENOMEM);

	sdev = &sub_sdev->sdev;
	sdev->ctrl = sparent->ctrl;

	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;


> +	sdev->usid = sparent->usid;
> +
> +	ret = dev_set_name(&sdev->dev, "%d-%02x.%d.auto",
> +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
> +	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;
> +
> +err_put_dev:
> +	put_device(&sdev->dev);
> +	return ERR_PTR(ret);
> +}

-- 
With Best Regards,
Andy Shevchenko