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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <a16cafd4-4d6c-45be-b241-45d2d6479bb1@collabora.com>
Date: Wed, 17 Sep 2025 13:41:40 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.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

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.

>> +	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.

Cheers,
Angelo

>> +			dev_name(&sdev->dev), 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");
>> +
> 
> Best regards
> Uwe