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] [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;
>  


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ