| 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
| ||
|
Message-ID: <20251102103808.73cac929@jic23-huawei>
Date: Sun, 2 Nov 2025 10:38:08 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
marcelo.schmitt1@...il.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] iio: mpl3115: add threshold events support
On Fri, 31 Oct 2025 21:18:22 +0100
Antoni Pokusinski <apokusinski01@...il.com> wrote:
> Add support for pressure and temperature rising threshold events. For
> both channels *_en and *_value (in raw units) attributes are exposed.
>
> Since in write_event_config() the ctrl_reg1.active and ctrl_reg4
> are modified, accessing the data->ctrl_reg{1,4} in set_trigger_state()
> and write_event_config() needs to be now guarded by data->lock.
> Otherwise, it would be possible that 2 concurrent threads executing
> these functions would access the data->ctrl_reg{1,4} at the same time
> and then one would overwrite the other's result.
>
> Signed-off-by: Antoni Pokusinski <apokusinski01@...il.com>
Generally looks good to me, but some comments on the 24 bit value reading.
You got a lot of review quickly for this patch so I want to give some time
for follow up on v2 anyway.
Thanks,
Jonathan
> ---
> drivers/iio/pressure/mpl3115.c | 219 +++++++++++++++++++++++++++++++--
> 1 file changed, 209 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index c212dfdf59ff..472e9fd65776 100644
...
> @@ -347,15 +370,45 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> struct iio_dev *indio_dev = private;
> struct mpl3115_data *data = iio_priv(indio_dev);
> int ret;
> + __be32 val_press;
> + __be16 val_temp;
>
> ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
> if (ret < 0)
> return IRQ_HANDLED;
>
> - if (!(ret & MPL3115_INT_SRC_DRDY))
> + if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> + MPL3115_INT_SRC_DRDY)))
> return IRQ_NONE;
>
> - iio_trigger_poll_nested(data->drdy_trig);
> + if (ret & MPL3115_INT_SRC_DRDY)
> + iio_trigger_poll_nested(data->drdy_trig);
> +
> + if (ret & MPL3115_INT_SRC_PTH) {
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_PRESSURE, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + iio_get_time_ns(indio_dev));
> +
> + /* Reset the SRC_PTH bit in INT_SOURCE */
> + i2c_smbus_read_i2c_block_data(data->client,
> + MPL3115_OUT_PRESS,
> + 3, (u8 *)&val_press);
This is an oddity. Why read into a __be32 when it's a 24bit number?
I guess it doesn't really matter as you just need a big enough space
and throw the value away. However, I'd read it into a u8 [3]; then size off that
as well.
There are two existing cases of this in the driver. One of them should use
get_unaligned_be24 on a u8[3] buffer. The other one is more complex as it's
reading directly into the scan buffer that gets pushed to the kfifo and is
reading into a u8 buffer ultimately anyway so at least there is no
real suggestion of it being 32 bits (just a +4 shift to deal with natural
alignment as the storage has to be power of 2 in that case.).
hmm. I think either we should tidy up the easy case (_read_info_raw) +
use a u8[3] here or just stick to this being odd.
My preference would be to have another patch tidying up the other case
+ use a u8[3] here.
> + }
> +
> + if (ret & MPL3115_INT_SRC_TTH) {
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + iio_get_time_ns(indio_dev));
> +
> + /* Reset the SRC_TTH bit in INT_SOURCE */
> + i2c_smbus_read_i2c_block_data(data->client,
> + MPL3115_OUT_TEMP,
> + 2, (u8 *)&val_temp);
> + }
>
> return IRQ_HANDLED;
> }
> +
> +static int mpl3115_read_thresh(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct mpl3115_data *data = iio_priv(indio_dev);
> + int ret;
> + __be16 tmp;
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + switch (chan->type) {
> + case IIO_PRESSURE:
> + ret = i2c_smbus_read_i2c_block_data(data->client,
> + MPL3115_PRESS_TGT,
> + sizeof(tmp), (u8 *)&tmp);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * Target value for the pressure is
> + * 16-bit unsigned value in 2 Pa units
Trivial but wrap comments to 80 char limit. Obviously doesn't really matter;
just a question of consistency.
> + */
> + *val = be16_to_cpu(tmp) << 1;
> +
> + return IIO_VAL_INT;
> + case IIO_TEMP:
> + ret = i2c_smbus_read_byte_data(data->client, MPL3115_TEMP_TGT);
> + if (ret < 0)
> + return ret;
> +
> + /* Target value for the temperature is 8-bit 2's complement */
> + *val = sign_extend32(ret, 7);
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mpl3115_write_thresh(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct mpl3115_data *data = iio_priv(indio_dev);
> + __be16 tmp;
> +
> + if (info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + switch (chan->type) {
> + case IIO_PRESSURE:
> + val >>= 1;
> +
> + if (val < 0 || val > U16_MAX)
> + return -EINVAL;
> +
> + tmp = cpu_to_be16(val);
> +
> + return i2c_smbus_write_i2c_block_data(data->client,
> + MPL3115_PRESS_TGT,
> + sizeof(tmp), (u8 *)&tmp);
> + case IIO_TEMP:
> + if (val < S8_MIN || val > S8_MAX)
> + return -EINVAL;
> +
> + return i2c_smbus_write_byte_data(data->client,
> + MPL3115_TEMP_TGT, val);
> + default:
> + return -EINVAL;
> + }
> +}
Powered by blists - more mailing lists