Message-ID: <aVjMk4Z-VRaRYYeK@sunspire.home.arpa>
Date: Sat, 3 Jan 2026 10:00:19 +0200
From: Petre Rodan <petre.rodan@...dimension.ro>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Marcelo Schmitt <marcelo.schmitt1@...il.com>,
	David Lechner <dlechner@...libre.com>,
	Nuno S?? <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
	Andreas Klinger <ak@...klinger.de>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH 06/14] iio: pressure: mprls0025pa: memset rx_buf before
 reading new data


Hello and a Happy New Year!

On Sat, Dec 27, 2025 at 02:31:49PM +0000, Jonathan Cameron wrote:
> On Mon, 22 Dec 2025 11:06:07 -0300
> Marcelo Schmitt <marcelo.schmitt1@...il.com> wrote:
> 
> > Hi Petre,
> > 
> > On 12/22, Petre Rodan wrote:
> > > 
> > > hello Jonathan,
> > > 
> > > thank you for the review.
> > > 
> > > On Sun, Dec 21, 2025 at 06:21:51PM +0000, Jonathan Cameron wrote:  
> > > > On Sat, 20 Dec 2025 10:25:19 +0200
> > > > Petre Rodan <petre.rodan@...dimension.ro> wrote:
> > > >   
> > > > > Hello,
> > > > > 
> > > > > On Sat, Dec 20, 2025 at 01:47:05AM -0300, Marcelo Schmitt wrote:  
> > > > > > On 12/18, Petre Rodan wrote:    
> > > > > > > Zero out input buffer before reading the new conversion.
> > > > > > > Perform this operation in core instead of in the bus specific code.
> > > > > > > 
> > > > > > > Signed-off-by: Petre Rodan <petre.rodan@...dimension.ro>
> > > > > > > ---
> > > > > > >  drivers/iio/pressure/mprls0025pa.c     | 2 ++
> > > > > > >  drivers/iio/pressure/mprls0025pa_i2c.c | 1 -
> > > > > > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> > > > > > > index 00b1ff1e50a8..7cc8dd0d8476 100644
> > > > > > > --- a/drivers/iio/pressure/mprls0025pa.c
> > > > > > > +++ b/drivers/iio/pressure/mprls0025pa.c
> > > > > > > @@ -16,6 +16,7 @@
> > > > > > >  #include <linux/mod_devicetable.h>
> > > > > > >  #include <linux/module.h>
> > > > > > >  #include <linux/property.h>
> > > > > > > +#include <linux/string.h>
> > > > > > >  #include <linux/units.h>
> > > > > > >  
> > > > > > >  #include <linux/gpio/consumer.h>
> > > > > > > @@ -239,6 +240,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
> > > > > > >  		}
> > > > > > >  	}
> > > > > > >  
> > > > > > > +	memset(data->rx_buf, 0, sizeof(data->rx_buf));    
> > > > > > This is unusual and I don't think it's needed for the SPI path. Doesn't the I2C
> > > > > > subsystem overwrite the rx buffer with what it reads from the device?    
> > > > > 
> > > > > I thought it's best practice to ensure that old conversions are not accidentally re-used in case the read operation fell thru the cracks.
> > > > > that's exactly why in this particular case the BUSY flag is implemented on the hardware side.
> > > > > 
> > > > > please tell me how a few byte memset() would be detrimental.  
> > > > 
> > > > We don't normally do this as old data isn't a potential leak of anything
> > > > sensitive.  
> > > 
> > > from my point of view as someone writing drivers for chemistry lab instruments, stale readings are to be avoided at all costs.
> > > it's not about leaking sensitive data, it's about providing a warning sign if the read operation fails silently.
> > > 
> > > as an extreme (but fictional) example, a pilot looking at an altimeter would immediately recognize that something is wrong with it's pitot tube if it's giving out an off-scale static reading. if instead the output is believable (which would be the case when older readings are repeated due to an uncaught intermittent read error) then there would be some uncertainty and he would not know to definitely ignore the output of this particular instrument and trust another one instead.
> > > 
> > > the same logic applies to any instrument in a lab setting. 
> > > a digital titration system that mixes multiple reagents needs to rely on fresh conversions to know when to stop a process. some advanced sensors even provide an incrementing conversion counter, others simply signal that a measurement is ongoing/not fresh via a BUSY flag and these are designed so that the driver can avoid a stale reading.
> > > 
> > > getting back to my driver, some pressure sensor series have a latch-up sensitivity and they misbehave during reads in various ways under certain conditions. I understand that you say that silent fails are unlikely but I'd like to keep the memset, for peace of mind.  
> > 
> > I agree with you that old conversions should not be accidentally re-used nor
> > errors silently be ignored. But, to me, memset the read buffer to zero looks
> > like we don't trust the underlying I2C and SPI layers. In that case, we should
> > fix data read in those subsystems (if there is anyhting be fixed there).
> > Though probably unlikely scenario to happen, how would one trust the sensor
> > reading in a scenario where the extected measurement would be close to zero.
> 
> There are two different ways to get stale data in these buffers.
> 1) read fail. Those we absolutely should trust to be handled correctly by lower layer
>    with errors reported and the data therefore not used.  Whether it is stale or random
>    garbage doesn't really matter.  0 is often a perfectly valid reading so no extra
>    info from seeing that in the buffer.

for this sensor a raw conversion of 0 is out of bounds. valid values are between
 output_min and output_max:

	[MPR_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
	[MPR_FUNCTION_B] = { .output_min =  419430, .output_max =  3774874 },
	[MPR_FUNCTION_C] = { .output_min = 3355443, .output_max = 13421773 },

I expect that the user will recognize such an abnormal reading.

for the SPI transfer specifically there is a latch-up scenario in which the MOSI
signal is being clamped down by the sensor in sync with SCLK [1].
I do not know how all SPI controllers act when MOSI is forced to GND, thus
 provided the memset() for a hypothetical scenario in which the SPI controller
 just resets during the xfer (due to a brownout) and if this condition is not
  recognized by the low level spi layer (I see no way to test this).

[1] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1588325/am3358-spi-tx-data-corruption
(my guess is that ABP2 and MPR series of sensors share the same silicon implementation)

> 2) Changes in configuration that move the holes around or create some.  This is the
>    more interesting corner.  I don't think the stale data argument applies because
>    any software reading the data in the holes and doing anything with it is inherently
>    buggy (plus as above, 0 is almost always a valid reading!)  Some drivers will
>    even read other data into those holes (checksums etc are common).
> 
> So I don't see either as being a particularly problem here.  I don't mind the buffer
> being cleared on each read but it doesn't to me seem necessary for any correctness
> or security related reasons which are the two cases we need to make sure we clear
> data for.

clamping an output signal to GND smells to me like 'undefined behaviour' when
one looks at a large number of SPI controller implementations and this memset
would provide an early warning, at no cost.

> Jonathan
> 
> > 
> > My suggestion is to look for a way of ensuring the transfer timing requirements
> > specified in data sheet page 18 [1]. See the dummy delay transfer suggestion to
> > patch 09.
> > 
> > [1]: https://4donline.ihs.com/images/VipMasterIC/IC/HWSC/HWSC-S-A0016036563/HWSC-S-A0016036563-1.pdf?hkey=CECEF36DEECDED6468708AAF2E19C0C6

yes, thank you Marcelo. your delay xfer idea works and it will be part of V2.
but that issue is completely unrelated to the memset modification.

best regards,
peter

> > > > However in most drivers this only spills out at all as
> > > > a result of say a change in configured channels and is normally harmless
> > > > as userspace knows to ignore stuff in the gaps anyway.  If there is
> > > > another cases here (you mention the busy flag) then add a comment on why
> > > > it makes sense. I don't in general want drivers to start doing this as
> > > > it is in the fast path and sometimes the memset is non trivial (here it
> > > > is probably irrelevant as the buffer is small).
> > > > 
> > > > Thanks,
> > > > 
> > > > Jonathan  
> 

-- 
petre rodan

Download attachment "signature.asc" of type "application/pgp-signature" (871 bytes)

Powered by blists - more mailing lists