linux-kernel - Re: [PATCH v4 1/7] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <hsemgg5s3nptxeh3jyim6ahgb37yb3h3hcbdtwixiisyf7ehqk@r7zlg23yz5cv>
Date: Wed, 17 Sep 2025 16:57:52 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: sboyd@...nel.org, 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, kishon@...nel.org, sre@...nel.org, 
	krzysztof.kozlowski@...aro.org, 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, wenst@...omium.org, casey.connolly@...aro.org, 
	Jonathan Cameron <jonathan.cameron@...wei.com>, Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add()
 and devm variant

Hello AngeloGioacchino,

On Wed, Sep 17, 2025 at 01:41:40PM +0200, AngeloGioacchino Del Regno wrote:
> Il 16/09/25 15:25, Uwe Kleine-König ha scritto:
> > Hello AngeloGioacchino,
> > 
> > On Tue, Sep 16, 2025 at 10:44:39AM +0200, AngeloGioacchino Del Regno wrote:
> > > +/**
> > > + * 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 negative ERR_PTR.
> > > + */
> > > +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;
> > > +	sdev->usid = sparent->usid;
> > > +
> > > +	ret = dev_set_name(&sdev->dev, "%d-%02x.%d.auto",
> > > +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
> > 
> > If I understand correctly sub_sdev->devid is globally unique. I wonder
> > if a namespace that is specific to the parent spmi device would be more
> > sensible?!
> 
> Only in the context of the children of sdev. I'm not sure of what you're proposing
> here, looks like it would complicate the code for no big reason - unless I am
> misunderstanding something here.

The thing that I wondered about is: Why use sdev->usid if
sub_sdev->devid is already a unique description of the subdevice? And
for other device types (platform devices, mfd) the device identifiers
are not globally unique. So I just wondered why spmi is different here.

> > > +	if (ret)
> > > +		goto err_put_dev;
> > > +
> > > +	ret = device_add(&sdev->dev);
> > > +	if (ret) {
> > > +		dev_err(&sdev->dev, "Can't add %s, status %d\n",
> > 
> > I'd use %pe instead of %d here.
> > 
> 
> The only reason why I am using %d is for consistency with the rest of the code that
> is in SPMI - there is another device_add() call in spmi_device_add() which prints
> the same error in the very same way as I'm doing here.
> 
> I agree that using %pe makes error prints more readable, but perhaps that should be
> done as a later cleanup to keep prints consistent (and perhaps that should not be
> done only in SPMI anyway).
> 
> If you have really strong opinions about doing that right now I can do it, but I
> anyway prefer seeing that as a later commit doing that in the entire SPMI codebase.

My approach would be to first convert the driver to use %pe and then
add the new code. But I don't feel strong.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)