linux-kernel - Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt handling

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: <20250525135631.3ae0a911@jic23-huawei>
Date: Sun, 25 May 2025 13:56:31 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de,
 Michael.Hennerich@...log.com, linux-iio@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 07/12] iio: accel: adxl313: add basic interrupt
 handling


> > +/**
> > + * adxl313_fifo_reset() - Reset the FIFO and interrupt status registers.
> > + * @data: The device data.
> > + *
> > + * Reset the FIFO status registers. Reading out status registers clears the  
> 
> I think you already read it before calling this. So how is it ever set?
> 
> > + * FIFO and interrupt configuration. Thus do not evaluate regmap return values.
> > + * Ignore particular read register content. Register content is not processed
> > + * any further. Therefore the function returns void.
> > + */
> > +static void adxl313_fifo_reset(struct adxl313_data *data)  
> 
> As below.  This isn't a reset.  Fifo reset is normally the term used
> for when we have lost tracking of what is in the fifo and drop all data,
> not normal readback.

Ok. After next patch it became more obvious how this is being used.

I'd combine the patches probably to avoid confusion or the odd state
that any interrupt resets the fifo after this patch.

> 
> > +{
> > +	unsigned int regval;
> > +	int samples;
> > +
> > +	adxl313_set_measure_en(data, false);  
> Disabling measurement to read a fifo is unusual -  is this really necessary
> as it presumably puts a gap in the data, which is what we are trying
> to avoid by using a fifo.

This makes more sense as well if we are in a reset condition.
I just got thrown off by the lack of a 'good' path in this patch.

> 
> > +
> > +	samples = adxl313_get_samples(data);
> > +	if (samples > 0)
> > +		adxl313_fifo_transfer(data, samples);
> > +
> > +	regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &regval);  
> 
> Not processing the convents of INT_SOURCE every time you read it
> introduces race conditions.  This logic needs a rethink so that
> never happens.  I guess this is why you are disabling measurement
> to stop the status changing?  Just whatever each read of INT_SOURCE
> tells us we need to handle and all should be fine without disabling
> measurement.  That read should only clear bits that are set, so no
> race conditions.
> 
> > +
> > +	adxl313_set_measure_en(data, true);
> > +}