[PATCH v6 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration

[PATCH v6 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

Extend the mma8452 driver with support for configuration of the
interrupt line in open-drain mode, which is needed for hardware designs
where the interrupt line is shared with other chips.

Adding drive-open-drain property to mma8452 device-tree node for such
designs to enable switching pin configuration to open-drain mode.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v6:
- Prevent interrupt storm if runtime suspend fails to set the device in
  standby mode by not setting suspended flag to true when failing to
  activate standby mode and by ensuring PM counter is not leaked in
  mma8452_interrupt().
- Don't acquire data->lock in mma8452_read_raw().
- Add fix for use-after-free in mma8452_probe() error path when CONFIG_PM
  is enabled.
- Fix (brown paper bug) build error introduced in v5.
- Renamed label in mma8452_interrupt().
- Add patch to return -ENODATA when missing device model information.
- Link to v5: https://patch.msgid.link/20260819-mma8452-open-drain-v5-0-d8aa590d7c36@geanix.com

Changes in v5:
- Squashed dev_err_probe() call to one line.
- Fixed typo in patch 2 title.
- Added synchronization between runtime suspend and interrupt handler.
- Link to v4: https://patch.msgid.link/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com

Changes in v4:
- Fixed interrupt handler to check runtime PM status before trying to
  access the chip.
- Split open-drain support and interrupt sharing into separate patches.
- Added new patch to reuse existing struct device * through mma8452_probe()
  function.
- Print warning message when irq type is not set by firmware.
- Link to v3: https://patch.msgid.link/20260805-mma8452-open-drain-v3-0-6149f406a409@geanix.com

Changes in v3:
- Reordered patches, swapping #2 and #3.
- Always add IRQF_SHARED flag.
- New patch to change it so IQRF_TRIGGER_LOW flag is only added when no
  trigger type is set by firmware.
- Link to v2: https://patch.msgid.link/20260715-mma8452-open-drain-v2-0-95be9f5f4795@geanix.com

Changes in v2:
- Commit message of patch 2 updated.
- Operator precedence bug fixed in flags argument to
  request_threaded_irq().
- Always check return value of mma8452_set_interrupt_pin_mode(), and just
  check for non-zero value.
- Added new patch with optimization of struct mma8452_data ordering.
- Link to v1: https://patch.msgid.link/20260715-mma8452-open-drain-v1-0-b1dd2a440c60@geanix.com

To: Jonathan Cameron <jic23@kernel.org>
To: Lars-Peter Clausen <lars@metafoo.de>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Martin Kepplinger <martink@posteo.de>
To: Sean Nyekjaer <sean@geanix.com>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Martin Kepplinger <martin.kepplinger@theobroma-systems.com>
To: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Esben Haabendal (9):
      dt-bindings: iio: accel: mma8452: Add drive-open-drain
      iio: accel: mma8452: Optimize struct mma8452_data member orders
      iio: accel: mma8452: Only apply trigger type when not set by firmware
      iio: accel: mma8452: Support interrupt sharing
      iio: accel: mma8452: Allow open drain interrupt pin configuration
      iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()
      iio: accel: mma8452: Drop unneeded lock acquire on read
      iio: accel: mma8452: Fix use-after-free bug in error error path
      iio: accel: mma8452: Use proper error code when missing device model

 .../devicetree/bindings/iio/accel/fsl,mma8452.yaml |   6 ++
 drivers/iio/accel/mma8452.c                        | 117 ++++++++++++++++-----
 2 files changed, 94 insertions(+), 29 deletions(-)
---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
change-id: 20250401-mma8452-open-drain-81577c41375c

Best regards,
--  
Esben Haabendal <esben@geanix.com>

[PATCH v6 1/9] dt-bindings: iio: accel: mma8452: Add drive-open-drain

[Esben Haabendal]

Add new boolean to configure selected interrupt pin to open drain instead
of the default push-pull mode.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
index b0dd2b4e116a..20701aa725d0 100644
--- a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
@@ -39,6 +39,12 @@ properties:
     minItems: 1
     maxItems: 2
 
+  drive-open-drain:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: the interrupt line will be configured as open drain, which is
+      useful if several sensors share the same interrupt line. (This binding is
+      taken from pinctrl.)
+
   vdd-supply: true
   vddio-supply: true
 

-- 
2.55.0

[PATCH v6 2/9] iio: accel: mma8452: Optimize struct mma8452_data member orders

[Esben Haabendal]

Reorder struct mma8452_data members to avoid holes.

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7d683686dd9d..f645a5c6fd1c 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -106,10 +106,7 @@ struct mma8452_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct iio_mount_matrix orientation;
-	u8 ctrl_reg1;
-	u8 data_cfg;
 	const struct mma_chip_info *chip_info;
-	int sleep_val;
 	struct regulator *vdd_reg;
 	struct regulator *vddio_reg;
 
@@ -118,6 +115,10 @@ struct mma8452_data {
 		__be16 channels[3];
 		aligned_s64 ts;
 	} buffer;
+
+	int sleep_val;
+	u8 ctrl_reg1;
+	u8 data_cfg;
 };
 
  /**

-- 
2.55.0

[PATCH v6 3/9] iio: accel: mma8452: Only apply trigger type when not set by firmware

[Esben Haabendal]

Instead of unconditionally overriding the trigger type, it is better to
only apply a default when no trigger type is set by firmware. This should
be reasonably backward compatible, and should only potentially cause
problems if systems exist where firmware specifies an incorrect trigger
type. With a bit of luck, there are no such systems.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index f645a5c6fd1c..1fb43c5b0b72 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1683,9 +1683,16 @@ static int mma8452_probe(struct i2c_client *client)
 		goto trigger_cleanup;
 
 	if (client->irq) {
+		unsigned long irq_flags;
+
+		irq_flags = irq_get_trigger_type(client->irq);
+		if (irq_flags == IRQ_TYPE_NONE) {
+			dev_info(dev, "invalid irq type, setting default active low\n");
+			irq_flags = IRQF_TRIGGER_LOW;
+		}
+		irq_flags |= IRQF_ONESHOT;
 		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
-					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-					   client->name, indio_dev);
+					   irq_flags, client->name, indio_dev);
 		if (ret)
 			goto buffer_cleanup;
 	}

-- 
2.55.0

[PATCH v6 4/9] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal]

Adding handling of rutnime PM suspension in the interrupt handler allows
sharing interrupt with other devices.

Keep in mind that the device by default is using push-pull for the irq pin,
which might require additional hardware design to allow interrupt sharing.

The suspended flag is added together with synchronize_irq() in order to
protect against race conditions when doing runtime suspend and device
removal. This way we ensure that interrupt handler does not try to access
the device while regulators are disabled.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 1fb43c5b0b72..8eb97e6793d6 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -119,6 +119,7 @@ struct mma8452_data {
 	int sleep_val;
 	u8 ctrl_reg1;
 	u8 data_cfg;
+	bool suspended;
 };
 
  /**
@@ -1056,14 +1057,24 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 	struct iio_dev *indio_dev = p;
 	struct mma8452_data *data = iio_priv(indio_dev);
 	irqreturn_t ret = IRQ_NONE;
+	int pm_status;
 	int src;
 
+	pm_status = pm_runtime_get_if_active(&data->client->dev);
+	if (pm_status == 0)
+		return IRQ_NONE; /* device is powered down */
+	if (READ_ONCE(data->suspended)) {
+		/* device is being removed */
+		ret = IRQ_NONE;
+		goto out_runtime_put;
+	}
+
 	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
 	if (src < 0)
-		return IRQ_NONE;
+		goto out_runtime_put;
 
 	if (!(src & (data->chip_info->enabled_events | MMA8452_INT_DRDY)))
-		return IRQ_NONE;
+		goto out_runtime_put;
 
 	if (src & MMA8452_INT_DRDY) {
 		iio_trigger_poll_nested(indio_dev->trig);
@@ -1089,6 +1100,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 		ret = IRQ_HANDLED;
 	}
 
+out_runtime_put:
+	if (pm_status > 0)
+		pm_runtime_put_autosuspend(&data->client->dev);
+
 	return ret;
 }
 
@@ -1690,7 +1705,7 @@ static int mma8452_probe(struct i2c_client *client)
 			dev_info(dev, "invalid irq type, setting default active low\n");
 			irq_flags = IRQF_TRIGGER_LOW;
 		}
-		irq_flags |= IRQF_ONESHOT;
+		irq_flags |= IRQF_ONESHOT | IRQF_SHARED;
 		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
 					   irq_flags, client->name, indio_dev);
 		if (ret)
@@ -1774,6 +1789,10 @@ static int mma8452_runtime_suspend(struct device *dev)
 		return -EAGAIN;
 	}
 
+	WRITE_ONCE(data->suspended, true);
+
+	synchronize_irq(client->irq);
+
 	ret = regulator_disable(data->vddio_reg);
 	if (ret) {
 		dev_err(dev, "failed to disable VDDIO regulator\n");
@@ -1808,6 +1827,8 @@ static int mma8452_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	WRITE_ONCE(data->suspended, false);
+
 	ret = mma8452_active(data);
 	if (ret < 0)
 		goto runtime_resume_failed;
@@ -1822,6 +1843,7 @@ static int mma8452_runtime_resume(struct device *dev)
 	return 0;
 
 runtime_resume_failed:
+	WRITE_ONCE(data->suspended, true);
 	regulator_disable(data->vddio_reg);
 	regulator_disable(data->vdd_reg);
 

-- 
2.55.0

[PATCH v6 5/9] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

When designing systems sharing the interrupt for mma8452 chips, it is
helpful to be able to configure the irq pin in open-drain mode (default is
push-pull).

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 8eb97e6793d6..d1e8eb2a4ad3 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -81,6 +81,8 @@
 #define  MMA8452_CTRL_REG2_RST			BIT(6)
 #define  MMA8452_CTRL_REG2_MODS_SHIFT		3
 #define  MMA8452_CTRL_REG2_MODS_MASK		0x1b
+#define MMA8452_CTRL_REG3			0x2c
+#define  MMA8452_CTRL_REG3_PP_OD		BIT(0)
 #define MMA8452_CTRL_REG4			0x2d
 #define MMA8452_CTRL_REG5			0x2e
 #define MMA8452_OFF_X				0x2f
@@ -120,6 +122,7 @@ struct mma8452_data {
 	u8 ctrl_reg1;
 	u8 data_cfg;
 	bool suspended;
+	bool open_drain;
 };
 
  /**
@@ -648,6 +651,22 @@ static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
 	return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
 }
 
+static int mma8452_set_interrupt_pin_mode(struct mma8452_data *data)
+{
+	int reg;
+
+	reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG3);
+	if (reg < 0)
+		return reg;
+
+	if (data->open_drain)
+		reg |= MMA8452_CTRL_REG3_PP_OD;
+	else
+		reg &= ~MMA8452_CTRL_REG3_PP_OD;
+
+	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG3, reg);
+}
+
 /* returns >0 if in freefall mode, 0 if not or <0 if an error occurred */
 static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
 {
@@ -1682,6 +1701,11 @@ static int mma8452_probe(struct i2c_client *client)
 			goto disable_regulators;
 	}
 
+	data->open_drain = device_property_read_bool(dev, "drive-open-drain");
+	ret = mma8452_set_interrupt_pin_mode(data);
+	if (ret)
+		goto trigger_cleanup;
+
 	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
 			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
 
@@ -1829,6 +1853,10 @@ static int mma8452_runtime_resume(struct device *dev)
 
 	WRITE_ONCE(data->suspended, false);
 
+	ret = mma8452_set_interrupt_pin_mode(data);
+	if (ret)
+		goto runtime_resume_failed;
+
 	ret = mma8452_active(data);
 	if (ret < 0)
 		goto runtime_resume_failed;

-- 
2.55.0

[PATCH v6 6/9] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

[Esben Haabendal]

In commit 32a5c04d4575 ("iio: accel: mma8452: Use dev_err_probe()") the
struct device * pointer was assigned to local variable dev, so we can just
as well reuse that throughout the function for sligthly more readable code.

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index d1e8eb2a4ad3..7ef1a9a91c31 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1587,7 +1587,7 @@ static int mma8452_probe(struct i2c_client *client)
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1597,21 +1597,20 @@ static int mma8452_probe(struct i2c_client *client)
 
 	data->chip_info = i2c_get_match_data(client);
 	if (!data->chip_info)
-		return dev_err_probe(&client->dev, -ENODEV,
-				     "unknown device model\n");
+		return dev_err_probe(dev, -ENODEV, "unknown device model\n");
 
-	ret = iio_read_mount_matrix(&client->dev, &data->orientation);
+	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
 		return ret;
 
-	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+	data->vdd_reg = devm_regulator_get(dev, "vdd");
 	if (IS_ERR(data->vdd_reg))
-		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_reg),
+		return dev_err_probe(dev, PTR_ERR(data->vdd_reg),
 				     "failed to get VDD regulator!\n");
 
-	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
+	data->vddio_reg = devm_regulator_get(dev, "vddio");
 	if (IS_ERR(data->vddio_reg))
-		return dev_err_probe(&client->dev, PTR_ERR(data->vddio_reg),
+		return dev_err_probe(dev, PTR_ERR(data->vddio_reg),
 				     "failed to get VDDIO regulator!\n");
 
 	ret = regulator_enable(data->vdd_reg);
@@ -1643,7 +1642,7 @@ static int mma8452_probe(struct i2c_client *client)
 		goto disable_regulators;
 	}
 
-	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
+	dev_info(dev, "registering %s accelerometer; ID 0x%x\n",
 		 data->chip_info->name, data->chip_info->chip_id);
 
 	i2c_set_clientdata(client, indio_dev);
@@ -1676,10 +1675,10 @@ static int mma8452_probe(struct i2c_client *client)
 	if (client->irq) {
 		int irq2;
 
-		irq2 = fwnode_irq_get_byname(dev_fwnode(&client->dev), "INT2");
+		irq2 = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
 
 		if (irq2 == client->irq) {
-			dev_dbg(&client->dev, "using interrupt line INT2\n");
+			dev_dbg(dev, "using interrupt line INT2\n");
 		} else {
 			ret = i2c_smbus_write_byte_data(client,
 							MMA8452_CTRL_REG5,
@@ -1687,7 +1686,7 @@ static int mma8452_probe(struct i2c_client *client)
 			if (ret < 0)
 				goto disable_regulators;
 
-			dev_dbg(&client->dev, "using interrupt line INT1\n");
+			dev_dbg(dev, "using interrupt line INT1\n");
 		}
 
 		ret = i2c_smbus_write_byte_data(client,
@@ -1736,14 +1735,13 @@ static int mma8452_probe(struct i2c_client *client)
 			goto buffer_cleanup;
 	}
 
-	ret = pm_runtime_set_active(&client->dev);
+	ret = pm_runtime_set_active(dev);
 	if (ret < 0)
 		goto free_irq;
 
-	pm_runtime_enable(&client->dev);
-	pm_runtime_set_autosuspend_delay(&client->dev,
-					 MMA8452_AUTO_SUSPEND_DELAY_MS);
-	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
@@ -1801,7 +1799,8 @@ static void mma8452_remove(struct i2c_client *client)
 #ifdef CONFIG_PM
 static int mma8452_runtime_suspend(struct device *dev)
 {
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct i2c_client *client = to_i2c_client(dev);
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int ret;
 
@@ -1809,7 +1808,7 @@ static int mma8452_runtime_suspend(struct device *dev)
 	ret = mma8452_standby(data);
 	mutex_unlock(&data->lock);
 	if (ret < 0) {
-		dev_err(&data->client->dev, "powering off device failed\n");
+		dev_err(&client->dev, "powering off device failed\n");
 		return -EAGAIN;
 	}
 

-- 
2.55.0

[PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read

[Esben Haabendal]

There is no need to acquire data->lock when calling mma8452_read(), and
dropping that makes it less likely to end up in an AB-BA deadlock
situation.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7ef1a9a91c31..9ae2c3e60576 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -504,9 +504,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 		if (!iio_device_claim_direct(indio_dev))
 			return -EBUSY;
 
-		mutex_lock(&data->lock);
 		ret = mma8452_read(data, buffer);
-		mutex_unlock(&data->lock);
 		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;

-- 
2.55.0

Re: [PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read

[Joshua Crofts]

On Tue, 25 Aug 2026 10:27:45 +0200
Esben Haabendal <esben@geanix.com> wrote:

> There is no need to acquire data->lock when calling mma8452_read(), and
> dropping that makes it less likely to end up in an AB-BA deadlock
> situation.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/iio/accel/mma8452.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7ef1a9a91c31..9ae2c3e60576 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -504,9 +504,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  		if (!iio_device_claim_direct(indio_dev))
>  			return -EBUSY;
>  
> -		mutex_lock(&data->lock);
>  		ret = mma8452_read(data, buffer);
> -		mutex_unlock(&data->lock);
>  		iio_device_release_direct(indio_dev);
>  		if (ret < 0)
>  			return ret;
> 

Sashiko has something to say and I tend to agree at the moment:

Could removing this lock expose mma8452_read() to race conditions with PM
auto-suspend and event configuration?

mma8452_read() can be interrupted by the PM auto-suspend worker, which puts
the device in STANDBY and disables regulators while mma8452_drdy() is actively
polling over I2C. This can lead to I/O timeouts or errors.

Additionally, concurrent sysfs writes to event configurations invoke
mma8452_change_config(), which puts the hardware into STANDBY to modify
registers. The Standby transition flushes the hardware FIFO. If this occurs
between the mma8452_drdy() check and the i2c_smbus_read_i2c_block_data() in
mma8452_read(), the block read will fetch flushed or stale data.


-- 
Kind regards,
Joshua Crofts

Re: [PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read

[Esben Haabendal]

"Joshua Crofts" <joshua.crofts1@gmail.com> writes:

> On Tue, 25 Aug 2026 10:27:45 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> There is no need to acquire data->lock when calling mma8452_read(), and
>> dropping that makes it less likely to end up in an AB-BA deadlock
>> situation.
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/iio/accel/mma8452.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 7ef1a9a91c31..9ae2c3e60576 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -504,9 +504,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>>  		if (!iio_device_claim_direct(indio_dev))
>>  			return -EBUSY;
>>
>> -		mutex_lock(&data->lock);
>>  		ret = mma8452_read(data, buffer);
>> -		mutex_unlock(&data->lock);
>>  		iio_device_release_direct(indio_dev);
>>  		if (ret < 0)
>>  			return ret;
>>
>
> Sashiko has something to say and I tend to agree at the moment:
>
> Could removing this lock expose mma8452_read() to race conditions with PM
> auto-suspend and event configuration?

Yes. I tend to agree as well.

> mma8452_read() can be interrupted by the PM auto-suspend worker, which puts
> the device in STANDBY and disables regulators while mma8452_drdy() is actively
> polling over I2C. This can lead to I/O timeouts or errors.

Yes, and causing pain for other I2C devices on same bus :(

> Additionally, concurrent sysfs writes to event configurations invoke
> mma8452_change_config(), which puts the hardware into STANDBY to modify
> registers. The Standby transition flushes the hardware FIFO. If this occurs
> between the mma8452_drdy() check and the i2c_smbus_read_i2c_block_data() in
> mma8452_read(), the block read will fetch flushed or stale data.

Argh. Yet another level of trouble.

Maybe we should extend the use of data->lock instead. Holding it
1. whenever doing read-modify-write actions
2. while holding the device in standby mode for changing registers
3. while doing suspend/resume

I was just adding support for a open-drain mode irq sharing you know.
While sashiko-bot definitely is catching lots of valid problems, and
fixing them is a good thing, this is starting to feel like I opened
Pandoras box by touching this driver :D

I will try to wrap up a patch with the above described extended usage of
data->lock, but hope we can find a way to get the irq sharing and open
drain mode support merged, without necessarily having to fixing all and every
possible existing bugs as a pre-condition, but maybe delay some work to
later work.

/Esben

Re: [PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read

[Joshua Crofts]

On Tue, 25 Aug 2026 15:35:17 +0200
Esben Haabendal <esben@geanix.com> wrote:

> "Joshua Crofts" <joshua.crofts1@gmail.com> writes:
> 
> > On Tue, 25 Aug 2026 10:27:45 +0200
> > Esben Haabendal <esben@geanix.com> wrote:
> >  
> >> There is no need to acquire data->lock when calling mma8452_read(), and
> >> dropping that makes it less likely to end up in an AB-BA deadlock
> >> situation.
> >>
> >> Signed-off-by: Esben Haabendal <esben@geanix.com>
> >> ---
> >>  drivers/iio/accel/mma8452.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> >> index 7ef1a9a91c31..9ae2c3e60576 100644
> >> --- a/drivers/iio/accel/mma8452.c
> >> +++ b/drivers/iio/accel/mma8452.c
> >> @@ -504,9 +504,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
> >>  		if (!iio_device_claim_direct(indio_dev))
> >>  			return -EBUSY;
> >>
> >> -		mutex_lock(&data->lock);
> >>  		ret = mma8452_read(data, buffer);
> >> -		mutex_unlock(&data->lock);
> >>  		iio_device_release_direct(indio_dev);
> >>  		if (ret < 0)
> >>  			return ret;
> >>  
> >
> > Sashiko has something to say and I tend to agree at the moment:
> >
> > Could removing this lock expose mma8452_read() to race conditions with PM
> > auto-suspend and event configuration?  
> 
> Yes. I tend to agree as well.
> 
> > mma8452_read() can be interrupted by the PM auto-suspend worker, which puts
> > the device in STANDBY and disables regulators while mma8452_drdy() is actively
> > polling over I2C. This can lead to I/O timeouts or errors.  
> 
> Yes, and causing pain for other I2C devices on same bus :(
> 
> > Additionally, concurrent sysfs writes to event configurations invoke
> > mma8452_change_config(), which puts the hardware into STANDBY to modify
> > registers. The Standby transition flushes the hardware FIFO. If this occurs
> > between the mma8452_drdy() check and the i2c_smbus_read_i2c_block_data() in
> > mma8452_read(), the block read will fetch flushed or stale data.  
> 
> Argh. Yet another level of trouble.
> 
> Maybe we should extend the use of data->lock instead. Holding it
> 1. whenever doing read-modify-write actions
> 2. while holding the device in standby mode for changing registers
> 3. while doing suspend/resume
> 
> I was just adding support for a open-drain mode irq sharing you know.
> While sashiko-bot definitely is catching lots of valid problems, and
> fixing them is a good thing, this is starting to feel like I opened
> Pandoras box by touching this driver :D
> 
> I will try to wrap up a patch with the above described extended usage of
> data->lock, but hope we can find a way to get the irq sharing and open
> drain mode support merged, without necessarily having to fixing all and every
> possible existing bugs as a pre-condition, but maybe delay some work to
> later work.
> 

If they're pre-existing conditions then it's not required to fix those in the
same patch series, you can come back to it another time or someone else can fix
those.

Nevertheless this is an issue that will be caused directly by this patch if
applied. If your only goal is to add support for open-drain irq sharing, you
can probably just drop this for the time being and just focus on that.

-- 
Kind regards,
Joshua Crofts

Re: [PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read

[Esben Haabendal]

"Joshua Crofts" <joshua.crofts1@gmail.com> writes:

> On Tue, 25 Aug 2026 15:35:17 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> "Joshua Crofts" <joshua.crofts1@gmail.com> writes:
>>
>> > On Tue, 25 Aug 2026 10:27:45 +0200
>> > Esben Haabendal <esben@geanix.com> wrote:
>> >
>> >> There is no need to acquire data->lock when calling mma8452_read(), and
>> >> dropping that makes it less likely to end up in an AB-BA deadlock
>> >> situation.
>> >>
>> >> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> >> ---
>> >>  drivers/iio/accel/mma8452.c | 2 --
>> >>  1 file changed, 2 deletions(-)
>> >>
>> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> >> index 7ef1a9a91c31..9ae2c3e60576 100644
>> >> --- a/drivers/iio/accel/mma8452.c
>> >> +++ b/drivers/iio/accel/mma8452.c
>> >> @@ -504,9 +504,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>> >>  		if (!iio_device_claim_direct(indio_dev))
>> >>  			return -EBUSY;
>> >>
>> >> -		mutex_lock(&data->lock);
>> >>  		ret = mma8452_read(data, buffer);
>> >> -		mutex_unlock(&data->lock);
>> >>  		iio_device_release_direct(indio_dev);
>> >>  		if (ret < 0)
>> >>  			return ret;
>> >>
>> >
>> > Sashiko has something to say and I tend to agree at the moment:
>> >
>> > Could removing this lock expose mma8452_read() to race conditions with PM
>> > auto-suspend and event configuration?
>>
>> Yes. I tend to agree as well.
>>
>> > mma8452_read() can be interrupted by the PM auto-suspend worker, which puts
>> > the device in STANDBY and disables regulators while mma8452_drdy() is actively
>> > polling over I2C. This can lead to I/O timeouts or errors.
>>
>> Yes, and causing pain for other I2C devices on same bus :(
>>
>> > Additionally, concurrent sysfs writes to event configurations invoke
>> > mma8452_change_config(), which puts the hardware into STANDBY to modify
>> > registers. The Standby transition flushes the hardware FIFO. If this occurs
>> > between the mma8452_drdy() check and the i2c_smbus_read_i2c_block_data() in
>> > mma8452_read(), the block read will fetch flushed or stale data.
>>
>> Argh. Yet another level of trouble.
>>
>> Maybe we should extend the use of data->lock instead. Holding it
>> 1. whenever doing read-modify-write actions
>> 2. while holding the device in standby mode for changing registers
>> 3. while doing suspend/resume
>>
>> I was just adding support for a open-drain mode irq sharing you know.
>> While sashiko-bot definitely is catching lots of valid problems, and
>> fixing them is a good thing, this is starting to feel like I opened
>> Pandoras box by touching this driver :D
>>
>> I will try to wrap up a patch with the above described extended usage of
>> data->lock, but hope we can find a way to get the irq sharing and open
>> drain mode support merged, without necessarily having to fixing all and every
>> possible existing bugs as a pre-condition, but maybe delay some work to
>> later work.
>>
>
> If they're pre-existing conditions then it's not required to fix those in the
> same patch series, you can come back to it another time or someone else can fix
> those.

Great. I have created a local mma8452-next branch where I will continue
the work on some of these issues.

> Nevertheless this is an issue that will be caused directly by this patch if
> applied. If your only goal is to add support for open-drain irq sharing, you
> can probably just drop this for the time being and just focus on that.

I will drop this particular patch for this series, and work on a proper
fix for all these race conditions in my mma8452-next branch.

/Esben

[PATCH v6 8/9] iio: accel: mma8452: Fix use-after-free bug in error error path

[Esben Haabendal]

If mma8452_probe() fails in iio_device_register() or later, we could end up
with runtime suspend callback being called with a now freed device pointer.

Fixes: 96c0cb2bbfe0 ("iio: mma8452: add support for runtime power management")
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 9ae2c3e60576..4a1eb196589a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1743,7 +1743,7 @@ static int mma8452_probe(struct i2c_client *client)
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
-		goto free_irq;
+		goto runtime_suspend;
 
 	ret = mma8452_set_freefall_mode(data, false);
 	if (ret < 0)
@@ -1754,6 +1754,10 @@ static int mma8452_probe(struct i2c_client *client)
 unregister_device:
 	iio_device_unregister(indio_dev);
 
+runtime_suspend:
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+
 free_irq:
 	if (client->irq)
 		free_irq(client->irq, indio_dev);

-- 
2.55.0

[PATCH v6 9/9] iio: accel: mma8452: Use proper error code when missing device model

[Esben Haabendal]

The device is there, but we don't have data describing how to use it.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 4a1eb196589a..42e3371cdb1d 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1595,7 +1595,7 @@ static int mma8452_probe(struct i2c_client *client)
 
 	data->chip_info = i2c_get_match_data(client);
 	if (!data->chip_info)
-		return dev_err_probe(dev, -ENODEV, "unknown device model\n");
+		return dev_err_probe(dev, -ENODATA, "unknown device model\n");
 
 	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)

-- 
2.55.0

Re: [PATCH v6 9/9] iio: accel: mma8452: Use proper error code when missing device model

[Joshua Crofts]

On Tue, 25 Aug 2026 10:27:47 +0200
Esben Haabendal <esben@geanix.com> wrote:

> The device is there, but we don't have data describing how to use it.

A bit of a weird commit message IMO, I'd do

Switch -ENODEV error on i2c_get_match_data() failure to -ENODATA to
satisfy the IIO coding style. (but this is only my opinion).

(We recently had a few conversations about -ENODEV vs. -ENODATA and
while there are a lot of uses of -ENODEV in IIO they should be replaced
with -ENODATA when checking *_get_match_data() results).

> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/iio/accel/mma8452.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 4a1eb196589a..42e3371cdb1d 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1595,7 +1595,7 @@ static int mma8452_probe(struct i2c_client *client)
>  
>  	data->chip_info = i2c_get_match_data(client);
>  	if (!data->chip_info)
> -		return dev_err_probe(dev, -ENODEV, "unknown device model\n");
> +		return dev_err_probe(dev, -ENODATA, "unknown device model\n");
>  
>  	ret = iio_read_mount_matrix(dev, &data->orientation);
>  	if (ret)
> 

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>

-- 
Kind regards,
Joshua Crofts

Re: [PATCH v6 9/9] iio: accel: mma8452: Use proper error code when missing device model

[Esben Haabendal]

"Joshua Crofts" <joshua.crofts1@gmail.com> writes:

> On Tue, 25 Aug 2026 10:27:47 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> The device is there, but we don't have data describing how to use it.
>
> A bit of a weird commit message IMO,

:)

> I'd do
>
> Switch -ENODEV error on i2c_get_match_data() failure to -ENODATA to
> satisfy the IIO coding style. (but this is only my opinion).

Sounds good to me. I will update for next version, if needed. But feel
free to make the change when merging.

/Esben

> (We recently had a few conversations about -ENODEV vs. -ENODATA and
> while there are a lot of uses of -ENODEV in IIO they should be replaced
> with -ENODATA when checking *_get_match_data() results).
>
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/iio/accel/mma8452.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 4a1eb196589a..42e3371cdb1d 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -1595,7 +1595,7 @@ static int mma8452_probe(struct i2c_client *client)
>>
>>  	data->chip_info = i2c_get_match_data(client);
>>  	if (!data->chip_info)
>> -		return dev_err_probe(dev, -ENODEV, "unknown device model\n");
>> +		return dev_err_probe(dev, -ENODATA, "unknown device model\n");
>>
>>  	ret = iio_read_mount_matrix(dev, &data->orientation);
>>  	if (ret)
>>
>
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
>
> --
> Kind regards,
> Joshua Crofts