linux-kernel - Re: [PATCH RFC v1 2/2] iio: light: rpr0521: Drop unnecessary checks for timestamp value

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: <20240928161527.73fe0ac1@jic23-huawei>
Date: Sat, 28 Sep 2024 16:15:27 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: lars@...afoo.de, andy.shevchenko@...il.com,
 u.kleine-koenig@...gutronix.de, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v1 2/2] iio: light: rpr0521: Drop unnecessary checks
 for timestamp value

On Sun, 22 Sep 2024 18:20:41 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> The rpr0521_trigger_consumer_handler() is registered as the trigger
> threaded handler in the devm_iio_triggered_buffer_setup() function.
> This function is being called in 2 ways:
> 
> a) when there is a registered trigger being trigger like sysfs or hrt.
> The call of the trigger handler (which is the iio_pollfunc_store_time())
> follows which saves the timestamp and then, wakes up the trigger
> threaded handler.
> 
> b) The irq handler is using the iio_trigger_poll_nested() which wakes
> up the trigger threaded handler.
> 
> In both cases, the pf->timestamp has already been assigned a value
> so there is no need to check if it is 0, neither to 0 it after
> the push to the buffer.

Not quite right (I think).  The caller of iio_trigger_poll_nested() might not
be this driver.  There are other potential triggers that are nested
because of need to check some status register, but can still be used
for other devices.   In theory you could drive light capture of
the as3935 for when you want to know how bright it was just after
a lightening strike :)

We don't have a general solution for timestamps when that
happens, so this driver is papering over that with this code.


Jonathan



> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>
> ---
>  drivers/iio/light/rpr0521.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 56f5fbbf79ac..ae6a22b91b8d 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -446,13 +446,8 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  	int err;
>  
>  	/* Use irq timestamp when reasonable. */
> -	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
> +	if (iio_trigger_using_own(indio_dev))
>  		pf->timestamp = data->irq_timestamp;
> -		data->irq_timestamp = 0;
> -	}
> -	/* Other chained trigger polls get timestamp only here. */
> -	if (!pf->timestamp)
> -		pf->timestamp = iio_get_time_ns(indio_dev);
>  
>  	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
>  		data->scan.channels,
> @@ -463,7 +458,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  	else
>  		dev_err(&data->client->dev,
>  			"Trigger consumer can't read from sensor.\n");
> -	pf->timestamp = 0;
>  
>  	iio_trigger_notify_done(indio_dev->trig);
>