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: <24eda02c59eb8a7c2d167af358364152a93b3440.camel@gmail.com>
Date: Mon, 24 Jun 2024 10:13:05 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Olivier MOYSAN <olivier.moysan@...s.st.com>, Nuno Sa
 <nuno.sa@...log.com>,  Lars-Peter Clausen <lars@...afoo.de>,
 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

On Sun, 2024-06-23 at 14:56 +0100, Jonathan Cameron wrote:
> On Thu, 20 Jun 2024 12:07:47 +0200
> Nuno Sá <noname.nuno@...il.com> wrote:
> 
> > On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote:
> > > 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.
> > >   
> > 
> > Oh, yes, you're right. I would prefer your later option. Call
> > __iio_backend_disable() from __iio_backend_disable() with a proper typed
> ?  That looks like an infinite loop :)  

Ahaha, yes it looks. But hopefully you got what I really meant :)

- Nuno Sá
>