| 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: <20250927175125.66bcc18c@jic23-huawei>
Date: Sat, 27 Sep 2025 17:51:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-iio@...r.kernel.org, linux@...ck-us.net, rodrigo.gobbi.7@...il.com,
naresh.solanki@...ements.com, michal.simek@....com,
grantpeltier93@...il.com, farouk.bouabid@...rry.de,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH v3 3/4] iio: mpl3115: add support for DRDY interrupt
On Sat, 27 Sep 2025 00:01:49 +0200
Antoni Pokusinski <apokusinski01@...il.com> wrote:
> MPL3115 sensor features a "data ready" interrupt which indicates the
> presence of new measurements.
>
> Signed-off-by: Antoni Pokusinski <apokusinski01@...il.com>
Various comments inline. Main ones are more on the guard() usage combined
with gotos and split out he renames as a precursor patch so only the main
change occurs in this one.
Thanks,
Jonathan
> ---
> drivers/iio/pressure/mpl3115.c | 197 ++++++++++++++++++++++++++++++---
> 1 file changed, 184 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 80af672f65c6..13c8b338a15e 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
>
> #define MPL3115_DEVICE_ID 0xc4
>
> #define MPL3115_STATUS_PRESS_RDY BIT(2)
> #define MPL3115_STATUS_TEMP_RDY BIT(1)
>
> -#define MPL3115_CTRL_RESET BIT(2) /* software reset */
> -#define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> -#define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> -#define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
> +#define MPL3115_INT_SRC_DRDY BIT(7)
> +
> +#define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
> +
> +#define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> +#define MPL3115_CTRL1_OST BIT(1) /* initiate measurement */
> +#define MPL3115_CTRL1_ACTIVE BIT(0) /* continuous measurement */
Precursor patch should make the renames to CTRL1
That will reduce the noise in this patch.
> +#define MPL3115_CTRL1_OS_258MS GENMASK(5, 4) /* 64x oversampling */
> +
> +#define MPL3115_CTRL3_IPOL1 BIT(5)
> +#define MPL3115_CTRL3_IPOL2 BIT(1)
> +
> +#define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> +
> +#define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> +
> +#define MPL3115_INT2 BIT(2) /* flag that indicates INT2 in use */
>
> struct mpl3115_data {
> struct i2c_client *client;
> + struct iio_trigger *drdy_trig;
> struct mutex lock;
> u8 ctrl_reg1;
> };
>
> +enum mpl3115_irq_type {
> + INT2_ACTIVE_LOW = MPL3115_INT2 | IRQF_TRIGGER_FALLING,
> + INT2_ACTIVE_HIGH = MPL3115_INT2 | IRQF_TRIGGER_RISING,
> + INT1_ACTIVE_LOW = (!MPL3115_INT2) | IRQF_TRIGGER_FALLING,
> + INT1_ACTIVE_HIGH = (!MPL3115_INT2) | IRQF_TRIGGER_RISING,
This mixing and matching of IRQF_ definitions with locally defined
additional flags is fragile because it is more than possible
a future kernel wide change will change the IRQF_ values.
So keep them separate.
> +};
> +
> static int mpl3115_request(struct mpl3115_data *data)
> {
> int ret, tries = 15;
>
> /* trigger measurement */
> ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> - data->ctrl_reg1 | MPL3115_CTRL_OST);
> + data->ctrl_reg1 | MPL3115_CTRL1_OST);
More renames that shouldn't be in this patch.
> if (ret < 0)
> return ret;
>
> @@ -58,7 +86,7 @@ static int mpl3115_request(struct mpl3115_data *data)
> if (ret < 0)
> return ret;
> /* wait for data ready, i.e. OST cleared */
> - if (!(ret & MPL3115_CTRL_OST))
> + if (!(ret & MPL3115_CTRL1_OST))
More renames for the precursor patch.
> break;
> msleep(20);
> }
> @@ -166,9 +194,11 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> int ret, pos = 0;
>
> scoped_guard(mutex, &data->lock) {
> - ret = mpl3115_request(data);
> - if (ret < 0)
> - goto done;
> + if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
> + ret = mpl3115_request(data);
> + if (ret < 0)
> + goto done;
This follows on from comment in earlier patch.
> + }
>
> +
> +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct mpl3115_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 ctrl_reg1 = data->ctrl_reg1;
> +
> + if (state)
> + ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> + else
> + ctrl_reg1 &= ~MPL3115_CTRL1_ACTIVE;
> +
> + guard(mutex)(&data->lock);
As in earlier patch, don't mix guard() or anything from cleanup.h with
code doing gotos as scope can be very weirdly defined when you do.
This is actually bug free (I think), but doesn't match the guidance notes in cleanup.h
Various options.
1. Don't use guard here
2. Factor out the stuff under the lock. The helper function has clearly defined
separate scope so that can contain the goto reg1_cleanup.h bit.
> +
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> + ctrl_reg1);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> + state ? MPL3115_CTRL4_INT_EN_DRDY : 0);
> + if (ret < 0)
> + goto reg1_cleanup;
> +
> + data->ctrl_reg1 = ctrl_reg1;
> +
> + return 0;
> +
> +reg1_cleanup:
> + i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> + data->ctrl_reg1);
> + return ret;
> +}
>
> +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> + struct iio_dev *indio_dev)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
> + int ret, irq, irq_type, irq_cfg_flags = 0;
> +
> + irq = fwnode_irq_get_byname(fwnode, "INT1");
> + if (irq < 0) {
> + irq = fwnode_irq_get_byname(fwnode, "INT2");
> + if (irq < 0)
> + return 0;
> +
> + irq_cfg_flags |= MPL3115_INT2;
> + }
> +
> + irq_type = irq_get_trigger_type(irq);
> + if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> + return -EINVAL;
> +
> + irq_cfg_flags |= irq_type;
Commented on this before, but mixing flags that are local to this driver
with those that are global provides not guarantees against future changes
of the global ones to overlap with your local values.
So just track these as two separate values rather than combining them.
> +
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> + MPL3115_PT_DATA_EVENT_ALL);
> + if (ret < 0)
> + return ret;
> +
> + switch (irq_cfg_flags) {
> + case INT2_ACTIVE_HIGH:
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> + MPL3115_CTRL3_IPOL2);
> + if (ret)
> + return ret;
> +
> + break;
> + case INT2_ACTIVE_LOW:
> + break;
> + case INT1_ACTIVE_HIGH:
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> + MPL3115_CTRL5_INT_CFG_DRDY);
> + if (ret)
> + return ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> + MPL3115_CTRL3_IPOL1);
> + if (ret)
> + return ret;
> +
> + break;
> + case INT1_ACTIVE_LOW:
> + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> + MPL3115_CTRL5_INT_CFG_DRDY);
> + if (ret)
> + return ret;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> + "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->drdy_trig)
> + return -ENOMEM;
> +
> + data->drdy_trig->ops = &mpl3115_trigger_ops;
> + iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> + ret = devm_iio_trigger_register(&data->client->dev, data->drdy_trig);
Whilst unlikely the race matters. It is this call that creates the infrastructure
that might allow the interrupt generation to be triggered via userspace controls.
So the handler should probably be in place firsts. I.e. do the devm_request_threaded_irq
before this.
> + if (ret)
> + return ret;
> +
> + indio_dev->trig = iio_trigger_get(data->drdy_trig);
> +
> + return devm_request_threaded_irq(&data->client->dev, irq,
> + NULL,
> + mpl3115_interrupt_handler,
> + IRQF_ONESHOT,
> + "mpl3115_irq",
> + indio_dev);
wrap closer to 80 chars by combining some of those lines.
> +}
> +
> static int mpl3115_probe(struct i2c_client *client)
> {
> const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -258,15 +425,19 @@ static int mpl3115_probe(struct i2c_client *client)
>
> /* software reset, I2C transfer is aborted (fails) */
> i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> - MPL3115_CTRL_RESET);
> + MPL3115_CTRL1_RESET);
> msleep(50);
>
> - data->ctrl_reg1 = MPL3115_CTRL_OS_258MS;
> + data->ctrl_reg1 = MPL3115_CTRL1_OS_258MS;
As elsewhere. Do the rename as a precursor patch so that we reduce
the noise around the real changes in here and make that bit easier to review.
> ret = i2c_smbus_write_byte_data(client, MPL3115_CTRL_REG1,
> data->ctrl_reg1);
> if (ret < 0)
> return ret;
>
> + ret = mpl3115_trigger_probe(data, indio_dev);
> + if (ret)
> + return ret;
> +
> ret = iio_triggered_buffer_setup(indio_dev, NULL,
> mpl3115_trigger_handler, NULL);
> if (ret < 0)
> @@ -285,7 +456,7 @@ static int mpl3115_probe(struct i2c_client *client)
> static int mpl3115_standby(struct mpl3115_data *data)
> {
> return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> - data->ctrl_reg1 & ~MPL3115_CTRL_ACTIVE);
> + data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
As above. This isn't part of the main change here so should have been in a separate
precursor patch
> }
>
> static void mpl3115_remove(struct i2c_client *client)
Powered by blists - more mailing lists