linux-kernel - Re: [PATCH] iio: trigger: Remove redundant conditionals

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: <20250504153044.1109d508@jic23-huawei>
Date: Sun, 4 May 2025 15:30:44 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Gyeyoung Baek <gye976@...il.com>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: trigger: Remove redundant conditionals

On Sun,  4 May 2025 03:56:50 +0900
Gyeyoung Baek <gye976@...il.com> wrote:

> Checks for null initially and return early.
> So there is no need to check for null later.
> 
> Signed-off-by: Gyeyoung Baek <gye976@...il.com>

So the key thing here is what does this path mean.  trig == NULL
means we are clearing the current trigger.  The snag is you just jumped
over the code that removes the old trigger or sets
indio_dev->trig = NULL.

So I think the new version you have here is broken.

For changes like this it is fairly easy to test them using the
dummy driver.  Please make sure to do so and make sure you trigger
all paths.  Here that would be.

No trigger -> trigger 1
trigger 1 -> trigger 2
trigger 2 -> no trigger
 

> ---
>  drivers/iio/industrialio-trigger.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 54416a384232..6abf2a450202 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -453,36 +453,32 @@ static ssize_t current_trigger_store(struct device *dev,
>  	}
>  
>  	trig = iio_trigger_acquire_by_name(buf);
> -	if (oldtrig == trig) {
> +	if (!trig || trig == oldtrig) {
>  		ret = len;
>  		goto out_trigger_put;
>  	}
> -
> -	if (trig && indio_dev->info->validate_trigger) {
> +	if (indio_dev->info->validate_trigger) {
>  		ret = indio_dev->info->validate_trigger(indio_dev, trig);
>  		if (ret)
>  			goto out_trigger_put;
>  	}
> -
> -	if (trig && trig->ops && trig->ops->validate_device) {
> +	if (trig->ops && trig->ops->validate_device) {
>  		ret = trig->ops->validate_device(trig, indio_dev);
>  		if (ret)
>  			goto out_trigger_put;
>  	}
>  
> -	indio_dev->trig = trig;

This and...

> -
>  	if (oldtrig) {

this need to run if oldtrig was set and trig is not.

>  		if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
>  			iio_trigger_detach_poll_func(oldtrig,
>  						     indio_dev->pollfunc_event);
>  		iio_trigger_put(oldtrig);
>  	}
> -	if (indio_dev->trig) {
> -		if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
> -			iio_trigger_attach_poll_func(indio_dev->trig,
> -						     indio_dev->pollfunc_event);
> -	}
> +	if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
> +		iio_trigger_attach_poll_func(trig,
> +						indio_dev->pollfunc_event);
> +
> +	indio_dev->trig = trig;
>  
>  	return len;
>