linux-kernel - Re: [PATCH v2 07/15] iio: fix write_event

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: <20241101163255.5f4d9d70@jic23-huawei>
Date: Fri, 1 Nov 2024 16:32:55 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Julien Stephan <jstephan@...libre.com>
Cc: Mudit Sharma <muditsharma.info@...il.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Anshul Dalal <anshulusr@...il.com>, Javier Carrasco
 <javier.carrasco.cruz@...il.com>, Jean-Baptiste Maneyrol
 <jean-baptiste.maneyrol@....com>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Cosmin Tanislav
 <cosmin.tanislav@...log.com>, Ramona Gradinariu
 <ramona.gradinariu@...log.com>, Antoniu Miclaus
 <antoniu.miclaus@...log.com>, Dan Robertson <dan@...obertson.com>, Marcelo
 Schmitt <marcelo.schmitt@...log.com>, Matteo Martelli
 <matteomartelli3@...il.com>, Anand Ashok Dumbre
 <anand.ashok.dumbre@...inx.com>, Michal Simek <michal.simek@....com>,
 Mariel Tinaco <Mariel.Tinaco@...log.com>, Jagath Jog J
 <jagathjog1996@...il.com>, Lorenzo Bianconi <lorenzo@...nel.org>, Subhajit
 Ghosh <subhajit.ghosh@...aklogic.com>, Kevin Tsai <ktsai@...ellamicro.com>,
 Linus Walleij <linus.walleij@...aro.org>, Benson Leung
 <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, chrome-platform@...ts.linux.dev,
 Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [PATCH v2 07/15] iio: fix write_event_config signature

On Thu, 31 Oct 2024 16:27:02 +0100
Julien Stephan <jstephan@...libre.com> wrote:

> write_event_config callback use an int for state, but it is actually a
> boolean. iio_ev_state_store is actually using kstrtobool to check user
> input, then gives the converted boolean value to write_event_config.
> 
> Fix signature and update all iio drivers to use the new signature.
> 
> This patch has been partially written using coccinelle with the
> following script:
> 
> $ cat iio-bool.cocci
> // Options: --all-includes
> 
> virtual patch
> 
> @c1@
> identifier iioinfo;
> identifier wecfunc;
> @@
>  static const struct iio_info iioinfo = {
>         ...,
>         .write_event_config =
> (
>  wecfunc
> |
>  &wecfunc
> ),
>         ...,
>  };
> 
> @@
> identifier c1.wecfunc;
> identifier indio_dev, chan, type, dir, state;
> @@
>  int wecfunc(struct iio_dev *indio_dev, const struct iio_chan_spec *chan, enum iio_event_type type, enum iio_event_direction dir,
> -int
> +bool
>  state) {
>   ...
>  }
> 
> make coccicheck MODE=patch COCCI=iio-bool.cocci M=drivers/iio
> 
> Unfortunately, this script didn't match all files:
> * all write_event_config callbacks using iio_device_claim_direct_scoped
>   were not detected and not patched.
> * all files that do not assign and declare the write_event_config
>   callback in the same file.
> 
> iio.h was also manually updated.
> 
> The patch was build tested using allmodconfig config.
> 
> cc: Julia Lawall <julia.lawall@...ia.fr>
> Signed-off-by: Julien Stephan <jstephan@...libre.com>

Hi Julien,

I went through these by hand.  There are somewhere maybe it is worth
passing booleans down into leaf functions and only convert to int
right at the end (for a field write) but I don't think we care.
The cases here are more of the variety of converting a bool to an int
to use it as a bool (like the ones you clear up later in this series).

Neither is wrong, just inefficient, so applied and pushed out as testing
for 0-day to take a look.
Fingers crossed nothing got missed!

Jonathan

> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> index ab427f3461dbbef535c2ec2cf2982202ca97bb82..f07fba17048e7b5c1958807b14d4bcb3ff87e26d 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -617,7 +617,7 @@ static int
>  fxls8962af_write_event_config(struct iio_dev *indio_dev,
>  			      const struct iio_chan_spec *chan,
>  			      enum iio_event_type type,
> -			      enum iio_event_direction dir, int state)
> +			      enum iio_event_direction dir, bool state)
>  {
>  	struct fxls8962af_data *data = iio_priv(indio_dev);
>  	u8 enable_event, enable_bits;
This passes the state variable into fxls8962af_event_setup() as an integer
and uses it as a boolean.  Might as well be bool all the way.


> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index 81e718cdeae32d60581cb490148f4f1c0bd695c7..1a352c88598e5d701256aa8659a7f9683bce56f9 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -1159,7 +1159,7 @@ static int gp2ap020a00f_write_event_config(struct iio_dev *indio_dev,
>  					   const struct iio_chan_spec *chan,
>  					   enum iio_event_type type,
>  					   enum iio_event_direction dir,
> -					   int state)
> +					   bool state)
This one could do with a follow up as the state variable is passed as an integer
to another function that then uses it as a bool.

>  {
>  	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
>  	enum gp2ap020a00f_cmd cmd;