linux-kernel - Re: [PATCH 2/8] iio: add enable and disable services to iio backend framework

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: <8c3de47c-bd72-4560-a43b-5275f2f7a059@foss.st.com>
Date: Wed, 19 Jun 2024 17:59:26 +0200
From: Olivier MOYSAN <olivier.moysan@...s.st.com>
To: Nuno Sá <noname.nuno@...il.com>,
        Nuno Sa
	<nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen
	<lars@...afoo.de>
CC: <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/8] iio: add enable and disable services to iio backend
 framework

Hi Nuno,

On 6/19/24 07:21, Nuno Sá wrote:
> On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
>> Add iio_backend_disable() and iio_backend_enable() APIs to allow
>> IIO backend consumer to request backend disabling and enabling.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@...s.st.com>
>> ---
> 
> Hi Olivier,
> 
> small notes from me...
> 
>>   drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
>>   include/linux/iio/backend.h        |  2 ++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
>> backend.c
>> index b950e30018ca..d3db048c086b 100644
>> --- a/drivers/iio/industrialio-backend.c
>> +++ b/drivers/iio/industrialio-backend.c
>> @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct
>> iio_backend *back)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
>>   
>> +/**
>> + * iio_backend_enable - Backend enable
>> + * @dev: Consumer device for the backend
>> + * @back: Backend device
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error number on failure.
>> + */
>> +int iio_backend_enable(struct device *dev, struct iio_backend *back)
>> +{
>> +	return iio_backend_op_call(back, enable);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);
> 
> We do already have devm_iio_backend_enable(). From a correctness stand point and even
> scalability, that API should now call your new iio_backend_enable() instead of
> directly call iio_backend_op_call(). I guess that change could be in this patch.
>

Sure. I have updated the patch.

>> +
>> +/**
>> + * iio_backend_disable - Backend disable
>> + * @dev: Consumer device for the backend
>> + * @back: Backend device
>> + *
>> + */
>> +void iio_backend_disable(struct device *dev, struct iio_backend *back)
>> +{
>> +	iio_backend_void_op_call(back, disable);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
>> +
> 
> We also have __iio_backend_disable() which is static since all users were using
> devm_iio_backend_enable(). I understand that's not suitable for you but I would
> instead rename the existing function to iio_backend_disable() and export it.
> 

Just renaming is not sufficient. The reason is that 
devm_add_action_or_reset() require an action with action(void *) 
prototype. So the prototype of iio_backend_disable() has to be changed 
to void iio_backend_disable(void *back).
I placed the same arguments in enable and disable for symmetry, but *dev 
is not required for time being in disable API. So it can make sense to 
change iio_backend_disable() prototype.
alternatively, we can call __iio_backend_disable() through this API.
Please, let me know is you have a preference.

Thanks
Olivier

> With the above changes:
> 
> Reviewed-by: Nuno Sa <nuno.sa@...log.com>
> 
> - Nuno Sá
>