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@subdimension.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));
ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
if (ret < 0)
return ret;
diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
index a0bbc6af9283..0fe8cfe0d7e7 100644
--- a/drivers/iio/pressure/mprls0025pa_i2c.c
+++ b/drivers/iio/pressure/mprls0025pa_i2c.c
@@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
if (cnt > MPR_MEASUREMENT_RD_SIZE)
return -EOVERFLOW;
- memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE);
ret = i2c_master_recv(client, data->rx_buf, cnt);
if (ret < 0)
return ret;
--
2.51.2On 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@subdimension.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? > ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN); > if (ret < 0) > return ret; > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c > index a0bbc6af9283..0fe8cfe0d7e7 100644 > --- a/drivers/iio/pressure/mprls0025pa_i2c.c > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt) > if (cnt > MPR_MEASUREMENT_RD_SIZE) > return -EOVERFLOW; > > - memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE); > ret = i2c_master_recv(client, data->rx_buf, cnt); > if (ret < 0) > return ret;
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@subdimension.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. best regards, peter > > ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN); > > if (ret < 0) > > return ret; > > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c > > index a0bbc6af9283..0fe8cfe0d7e7 100644 > > --- a/drivers/iio/pressure/mprls0025pa_i2c.c > > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c > > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt) > > if (cnt > MPR_MEASUREMENT_RD_SIZE) > > return -EOVERFLOW; > > > > - memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE); > > ret = i2c_master_recv(client, data->rx_buf, cnt); > > if (ret < 0) > > return ret; -- petre rodan
On Sat, 20 Dec 2025 10:25:19 +0200 Petre Rodan <petre.rodan@subdimension.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@subdimension.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. 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 > > best regards, > peter > > > > ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN); > > > if (ret < 0) > > > return ret; > > > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c > > > index a0bbc6af9283..0fe8cfe0d7e7 100644 > > > --- a/drivers/iio/pressure/mprls0025pa_i2c.c > > > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c > > > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt) > > > if (cnt > MPR_MEASUREMENT_RD_SIZE) > > > return -EOVERFLOW; > > > > > > - memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE); > > > ret = i2c_master_recv(client, data->rx_buf, cnt); > > > if (ret < 0) > > > return ret; >
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@subdimension.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@subdimension.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. > 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 > > > > > best regards, > > peter > > > > > > ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN); > > > > if (ret < 0) > > > > return ret; > > > > diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c > > > > index a0bbc6af9283..0fe8cfe0d7e7 100644 > > > > --- a/drivers/iio/pressure/mprls0025pa_i2c.c > > > > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c > > > > @@ -25,7 +25,6 @@ static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt) > > > > if (cnt > MPR_MEASUREMENT_RD_SIZE) > > > > return -EOVERFLOW; > > > > > > > > - memset(data->rx_buf, 0, MPR_MEASUREMENT_RD_SIZE); > > > > ret = i2c_master_recv(client, data->rx_buf, cnt); > > > > if (ret < 0) > > > > return ret; > > > -- petre rodan
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@subdimension.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@subdimension.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. 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 > > > 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
On Mon, 22 Dec 2025 11:06:07 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.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@subdimension.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@subdimension.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. 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. 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 > > > > > > 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
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@gmail.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@subdimension.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@subdimension.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
On Sat, 3 Jan 2026 10:00:19 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 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@gmail.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@subdimension.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@subdimension.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.
Ok. Add a comment on 0 not being a valid value for these channels and the
fact that it might be useful for diagnosing the condition you describe.
All I really want to do is avoid people seeing a 'memset is needed here' patch
and posting them for other drivers where it doesn't provide anything useful.
Jonathan
>
> > 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
> >
>
© 2016 - 2026 Red Hat, Inc.