| 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: <20251029224605.3ixkvmmkm36iwh22@antoni-VivoBook-ASUSLaptop-X512FAY-K512FA>
Date: Wed, 29 Oct 2025 23:46:05 +0100
From: Antoni Pokusinski <apokusinski01@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>, jic23@...nel.org,
dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
marcelo.schmitt1@...il.com
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: mpl3115: add threshold events support
On Wed, Oct 29, 2025 at 10:24:49AM +0200, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 10:33:52PM +0100, Antoni Pokusinski wrote:
> > Add support for pressure and temperature rising threshold events.
>
> ...
>
> > @@ -322,7 +339,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> > .storagebits = 32,
> > .shift = 12,
> > .endianness = IIO_BE,
> > - }
> > + },
> > + .event_spec = mpl3115_temp_press_event,
> > + .num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
> > },
> > {
> > .type = IIO_TEMP,
> > @@ -338,7 +357,9 @@ static const struct iio_chan_spec mpl3115_channels[] = {
> > .storagebits = 16,
> > .shift = 4,
> > .endianness = IIO_BE,
> > - }
> > + },
>
> Just a side note below, no action from you required on this!
>
> Yeah, yet another reminder for the comma/not-a-comma choices made initially and
> why it's important to follow the advice
>
> > + .event_spec = mpl3115_temp_press_event,
> > + .num_event_specs = ARRAY_SIZE(mpl3115_temp_press_event),
> > },
> > IIO_CHAN_SOFT_TIMESTAMP(2),
> > };
>
> ...
>
> > - if (!(ret & MPL3115_INT_SRC_DRDY))
> > + if (!(ret & (MPL3115_INT_SRC_DRDY | MPL3115_INT_SRC_PTH |
> > + MPL3115_INT_SRC_TTH)))
>
> Can we rather keep this split logical?
>
> if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> MPL3115_INT_SRC_DRDY)))
>
> > return IRQ_NONE;
>
> ...
>
> > - u8 ctrl_reg1 = data->ctrl_reg1;
> > - u8 ctrl_reg4 = data->ctrl_reg4;
> > + u8 ctrl_reg1, ctrl_reg4;
>
> > + guard(mutex)(&data->lock);
>
> Why this is moved? Before the access to the data->ctrl* was done without
> locking. Is it an existing bug?
>
Since this patchset adds `write_event_config()` in which CTRL_REG1.ACTIVE
and CTRL_REG4 are modified, the lock now needs to guard the read of
data->ctrl_regX as well. Otherwise, we could have e.g. 2 concurrent
threads executing `set_trigger_state()` and `write_event_config()` that
would read data->ctrl_regX at the same time and then one would overwrite
the other's values in `config_interrupt()`.
In the current driver I don't think there is any bug in here. The only
place (except probe) where the data->ctrl_regX is modified is
`config_interrupt()`, called from `set_trigger_state()`. If we had
concurrent calls to this function, then the final values of CTRL_REG1
and CTRL_REG4 would simply depend on which thread is scheduled as the last one.
With the `guard(mutex)` before accessing data->ctrl_reg1, the situation
would be exactly the same.
> > + ctrl_reg1 = data->ctrl_reg1;
> > + ctrl_reg4 = data->ctrl_reg4;
> >
> > if (state) {
> > ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > ctrl_reg4 |= MPL3115_CTRL4_INT_EN_DRDY;
> > } else {
> > - ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > - }
> >
> > - guard(mutex)(&data->lock);
> > + if (!ctrl_reg4)
> > + ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > + }
> >
> > return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
>
> ...
>
> > +static int mpl3115_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + bool state)
> > +{
> > + struct mpl3115_data *data = iio_priv(indio_dev);
> > + u8 int_en_mask;
> > + u8 ctrl_reg1, ctrl_reg4;
> > +
> > + switch (chan->type) {
> > + case IIO_PRESSURE:
> > + int_en_mask = MPL3115_CTRL4_INT_EN_PTH;
> > + break;
> > + case IIO_TEMP:
> > + int_en_mask = MPL3115_CTRL4_INT_EN_TTH;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> > + guard(mutex)(&data->lock);
>
> Similar Q here, why do you protect data that was (still is?) not protected before?
>
Same situation here as in `set_trigger_state()`
> > + ctrl_reg1 = data->ctrl_reg1;
> > + ctrl_reg4 = data->ctrl_reg4;
> > +
> > + if (state) {
> > + ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > + ctrl_reg4 |= int_en_mask;
> > + } else {
> > + ctrl_reg4 &= ~int_en_mask;
> > +
> > + if (!ctrl_reg4)
> > + ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> > + }
> > +
> > + return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> > +}
>
> ...
>
> > +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, press_pa;
> > + __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, 2,
>
> sizeof() ?
>
> > + (u8 *) &tmp);
>
> Here and elsewhere, drop the space between casting and operand.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + /**
>
> It's not a kernel-doc.
>
> > + * Target value for the pressure is
> > + * 16-bit unsigned value in 2 Pa units
> > + */
> > + press_pa = be16_to_cpu(tmp) << 1;
> > + *val = press_pa / KILO;
> > + *val2 = (press_pa % KILO) * MILLI;
> > +
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + 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);
> > + u8 tmp[2];
>
> Use proper __be16 type.
>
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EINVAL;
> > +
> > + switch (chan->type) {
> > + case IIO_PRESSURE:
> > + val = (val * KILO + val2 / MILLI) >> 1;
>
> > + if (val < 0 || val > 0xffff)
> > + return -EINVAL;
>
> U16_MAX?
>
> > + tmp[0] = FIELD_GET(GENMASK(15, 8), val);
> > + tmp[1] = FIELD_GET(GENMASK(7, 0), val);
> > +
> > + return i2c_smbus_write_i2c_block_data(data->client,
> > + MPL3115_PRESS_TGT, 2, tmp);
>
> sizeof()
>
> > + case IIO_TEMP:
> > + if (val < -128 || val > 127)
> > + return -EINVAL;
>
> S8_MIN, S8_MAX ?
>
> > + return i2c_smbus_write_byte_data(data->client,
> > + MPL3115_TEMP_TGT, val);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Kind regards,
Antoni
Powered by blists - more mailing lists