[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

[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 6/9] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Undocumented scope modification and suboptimal pointer dereferencing.

Pre-existing issues:
- [High] Device teardown accesses powered-off hardware and causes unbalanced regulator disables.
- [Low] Unconditional assignment of PM callbacks causes dead code when PM is disabled.
--

--- Patch [6]: [PATCH v6 6/9] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe() ---
commit 04da87f5e80c506bb658d3dd5161087ce55fa6b0
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

This commit refactors mma8452_probe() to use a local 'dev' variable instead
of repeatedly referencing &client->dev. It aims to improve code readability.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index d1e8eb2a4ad3a..7ef1a9a91c31f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

> @@ -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");

[Severity: Low]
The commit message states the changes are restricted to reusing the dev
pointer in mma8452_probe(), but this modifies mma8452_runtime_suspend().
Should the commit message be updated to reflect this scope change?

Also, since the function already receives a struct device *dev parameter,
and &client->dev evaluates to that exact same pointer, wouldn't it be 
simpler to use dev directly here rather than dereferencing client?

>  		return -EAGAIN;
>  	}


As a related note, looking at the device teardown path in mma8452_remove():

static void mma8452_remove(struct i2c_client *client)
{
[ ... ]
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);
[ ... ]
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
}

[Severity: High]
This is a pre-existing issue, but does this teardown sequence access
potentially powered-off hardware?

If the device is in the RPM_SUSPENDED state when remove() is called, its
regulators have already been disabled by mma8452_runtime_suspend(). The
call to mma8452_standby() attempts an I2C write to the powered-off device,
which might hang the I2C bus or cause transaction timeouts.

Furthermore, unconditionally calling regulator_disable() drops the usage
counts below zero, triggering a kernel warning for unbalanced disables.

Should pm_runtime_get_sync(&client->dev) be called at the beginning of
mma8452_remove() to ensure the hardware is active before teardown?


Finally, regarding the PM ops assignment in the driver struct:

[Severity: Low]
This isn't a bug introduced by this patch, but does the unconditional
assignment of &mma8452_pm_ops to the .pm field in mma8452_driver produce
dead code when CONFIG_PM is disabled?

According to the power management subsystem guidelines, this pointer should
be wrapped in pm_ptr(&mma8452_pm_ops) to allow the compiler to eliminate
the dead code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=6

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Short I2C reads lead to an uninitialized kernel stack memory leak to userspace.
- [Medium] Runtime PM reference leak on I2C communication failure.
- [Medium] Unbalanced regulator disable and unpowered I2C access during module removal.
- [Medium] Data race during read-modify-write of event configuration registers.
--

--- Patch [2]: [PATCH v6 2/9] iio: accel: mma8452: Optimize struct mma8452_data member orders ---
commit 26f60bee6a5ce9626d25a99fe80c0216eb010682
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Optimize struct mma8452_data member orders
    
Reorder struct mma8452_data members to avoid holes.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..f645a5c6fd1c2 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;
>  };

[Severity: Medium]
This is a pre-existing issue, but does this early return leak the runtime PM
reference count?

drivers/iio/accel/mma8452.c:mma8452_read() {
...
	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;
...
}

If i2c_smbus_read_i2c_block_data() fails, the function returns directly,
skipping the mma8452_set_runtime_pm_state() call.

A similar issue appears in mma8452_data_rdy_trigger_set_state() where a
failure during i2c_smbus_read_byte_data() skips dropping the autosuspend
reference if state was true.

[Severity: Medium]
This isn't a bug introduced by this patch, but during module removal, does
mma8452_remove() execute operations on an already suspended device?

drivers/iio/accel/mma8452.c:mma8452_remove() {
...
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);

	if (client->irq)
		free_irq(client->irq, indio_dev);

	iio_triggered_buffer_cleanup(indio_dev);
	mma8452_trigger_cleanup(indio_dev);
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
...
}

If the module is removed while the device is in a runtime-suspended state,
pm_runtime_disable() does not wake up the device. Subsequent calls to
mma8452_standby() and regulator_disable() are then executed on an unpowered
device.

[Severity: Medium]
This is a pre-existing issue, but can concurrent sysfs writes cause a data
race when updating event configuration registers?

drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
...
	case IIO_EV_DIR_RISING:
		val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
		if (val < 0)
			return val;
...
		return mma8452_change_config(data, ev_regs->ev_cfg, val);
...
}

The read-modify-write sequence is performed without acquiring the internal
driver lock.

If two threads concurrently write to different event attributes in sysfs,
could the initial read and bitwise modifications race, overwriting each
other's configuration bits?

[Severity: High]
This isn't a bug introduced by this patch, but can a short I2C read result
in uninitialized stack memory being leaked to userspace?

drivers/iio/accel/mma8452.c:mma8452_read() {
...
	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;

	ret = mma8452_set_runtime_pm_state(data->client, false);

	return ret;
}

If i2c_smbus_read_i2c_block_data() encounters a short read, it returns a
positive number less than the requested size. The condition ret < 0 is false,
so the code proceeds and overwrites ret with the return value of
mma8452_set_runtime_pm_state(), which is 0.

When mma8452_read() returns 0 instead of propagating the short read:

drivers/iio/accel/mma8452.c:mma8452_read_raw() {
...
		ret = mma8452_read(data, buffer);
...
		*val = sign_extend32(be16_to_cpu(
			buffer[chan->scan_index]) >> chan->scan_type.shift,
			chan->scan_type.realbits - 1);
...
}

Does the caller process and return the uninitialized stack memory from buffer
via *val?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `mma8452_remove` function fails to wake up the device before interacting with it, leading to I2C timeouts and unbalanced regulator disables if the device is currently runtime-suspended.
- [High] Numerous sysfs accessor callbacks perform I2C operations without ensuring the device is runtime active, leading to I2C errors when the device is suspended.
--

--- Patch [9]: [PATCH v6 9/9] iio: accel: mma8452: Use proper error code when missing device model ---
commit 1a9b4790dc03b13bece48bd3facb79fda0b2f907
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Use proper error code when missing device model

The device is there, but we don't have data describing how to use it.
This patch updates the error code returned during probe from -ENODEV to
-ENODATA when the device model is unknown.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 4a1eb196589ab..42e3371cdb1d7 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");

[Severity: High]
This isn't a bug introduced by this patch, but the mma8452_remove function
fails to wake up the device before interacting with it, which can cause I2C
timeouts and unbalanced regulator disables if the device is currently
runtime-suspended.

drivers/iio/accel/mma8452.c:mma8452_remove() {
	...
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);
	...
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
}

Should mma8452_remove() wake up the device via pm_runtime_get_sync() before
disabling runtime PM and issuing I2C writes to the powered-off chip in
mma8452_standby()?

Does disabling regulators here cause a regulator use-count underflow since
the regulators are already disabled when the device is autosuspended?

[Severity: High]
This is a pre-existing issue, but numerous sysfs accessor callbacks perform
I2C operations without ensuring the device is runtime active.

drivers/iio/accel/mma8452.c:mma8452_read() {
	...
	int ret = mma8452_drdy(data);

	if (ret < 0)
		return ret;

	ret = mma8452_set_runtime_pm_state(data->client, true);
	...
}

Will this cause I2C transaction failures when userspace attempts to read or
write sysfs attributes while the device is suspended?

The data ready check in mma8452_read() performs an I2C read before the device
is resumed from runtime suspend. Direct I2C reads and writes are similarly
issued in mma8452_read_raw() for calibration bias and in __mma8452_write_raw()
via mma8452_change_config() without resuming the device.

>  
>  	ret = iio_read_mount_matrix(dev, &data->orientation);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=9

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The device is left in ACTIVE mode on probe failure paths, potentially draining power and asserting interrupts indefinitely.
- [High] The driver leaks the PM runtime usage count on multiple error paths when I2C reads/writes fail after acquiring a runtime PM reference.
- [High] In `mma8452_runtime_suspend()`, if disabling the second regulator fails, the first regulator remains disabled but the PM state is left as RPM_ACTIVE.
- [High] Time-of-check to time-of-use (TOCTOU) race condition during read-modify-write of I2C event configuration registers.
--

--- Patch [8]: [PATCH v6 8/9] iio: accel: mma8452: Fix use-after-free bug in error error path ---
commit bad43bbf94c93f7ecf6436d8c780a1207e0288f4
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Fix use-after-free bug in error error path

This commit fixes a use-after-free in the mma8452_probe error path.
By adding a runtime_suspend label, it ensures the runtime PM disable step
happens before the IRQ is freed, avoiding callbacks with a freed device.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 9ae2c3e605761..4a1eb196589ab 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);

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race in
mma8452_set_freefall_mode() called here?

It looks like mma8452_set_freefall_mode() reads the configuration register
locklessly before modifying it:

mma8452_set_freefall_mode() {
	...
	val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
	...
	return mma8452_change_config(data, MMA8452_FF_MT_CFG, val);
}

Since mma8452_change_config() only acquires data->lock during the write,
could concurrent modifications via sysfs overwrite and discard each other's
changes? A similar pattern appears to exist in mma8452_write_event_config().

>  	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);
> +

[Severity: High]
This is a pre-existing issue, but does this error path skip placing the
hardware back into standby mode?

If mma8452_probe() fails and jumps to this runtime_suspend label, the
software resources are disabled, but the device might be left in ACTIVE
mode. Could this result in the device indefinitely asserting a shared
interrupt line if it was previously activated, since the IRQ handler is freed
shortly after?

Should there be a call to mma8452_standby() in this error path?

>  free_irq:
>  	if (client->irq)
>  		free_irq(client->irq, indio_dev);

[Severity: High]
This is a pre-existing issue, but does mma8452_read() leak a runtime PM
usage count on I2C errors?

If mma8452_set_runtime_pm_state(..., true) succeeds, but the subsequent I2C
block read fails:

mma8452_read() {
	...
	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;
	...
}

The function returns immediately without rolling back the PM reference.
I noticed similar early returns in mma8452_write_event_config() and
mma8452_data_rdy_trigger_set_state() that might leave the device permanently
active.

[Severity: High]
This is also a pre-existing issue, but is the regulator state left unbalanced
if runtime suspend fails?

In mma8452_runtime_suspend():

mma8452_runtime_suspend() {
	...
	regulator_disable(data->vddio_reg);

	ret = regulator_disable(data->vdd_reg);
	if (ret) {
		dev_err(dev, "failed to disable VDD regulator\n");
		return ret;
	}
	...
}

If disabling vdd_reg fails, the function returns an error without
re-enabling vddio_reg. Since the PM core keeps the device in RPM_ACTIVE
on failure, won't subsequent suspend attempts call
regulator_disable(data->vddio_reg) again and underflow the enable count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=8

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] If regulator_disable() fails in mma8452_runtime_suspend(), the data->suspended flag is not rolled back, leading to a permanent IRQ storm on subsequent interrupts.

Pre-existing issues:
- [High] mma8452_remove() does not wake up the device before disabling runtime PM, leading to unbalanced regulator disables (underflow) and illegal I2C access while powered off.
- [High] Registers are not restored after runtime resume, causing the device to lose critical configuration.
--

--- Patch [4]: [PATCH v6 4/9] iio: accel: mma8452: Support interrupt sharing ---
commit 5bc641da9d60875f38db9937a3a1dae1afd9a94e
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Support interrupt sharing

This commit adds handling of runtime PM suspension in the interrupt handler
to allow sharing the interrupt with other devices. It introduces a suspended
flag and uses synchronize_irq() to protect against race conditions during
runtime suspend and device removal.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 1fb43c5b0b72f..8eb97e6793d6f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -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;
> +	}

[Severity: High]
Is the READ_ONCE(data->suspended) check reachable during normal suspend?

Since pm_runtime_get_if_active() returns 0 when the device state is
RPM_SUSPENDING, it seems the handler will return early before reaching
the data->suspended check.

[ ... ]
> @@ -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");
>  		return ret;
>  	}

[Severity: High]
If regulator_disable() fails here, the function returns without resetting
data->suspended to false.

If this happens, the PM core leaves the device in RPM_ACTIVE. Would this
cause a permanent IRQ storm since the next interrupt will see pm_status > 0,
but data->suspended is true, causing it to continually return IRQ_NONE
without clearing the level-triggered interrupt?

[ ... ]
> @@ -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;

[Severity: High]
This is a pre-existing issue, but does mma8452_runtime_resume() need to
restore other device registers like MMA8452_CTRL_REG4 and MMA8452_DATA_CFG?

When the regulators are disabled during suspend, the device loses power and
these registers are reset. If they aren't restored here, will the device
lose its configuration and permanently disable hardware interrupts since
CTRL_REG4 defaults to 0?

[ ... ]
> @@ -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);
>  

[Severity: High]
This is a pre-existing issue, but if the device is unbound via sysfs while
runtime-suspended, will mma8452_remove() run into issues?

drivers/iio/accel/mma8452.c:mma8452_remove() {
    ...
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);
    ...
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
}

Since pm_runtime_disable() leaves the device suspended, won't
mma8452_standby() attempt an I2C write on an unpowered chip? Also, won't
calling regulator_disable() here cause an underflow since the regulators
were already disabled during runtime suspend? Does this need a
pm_runtime_get_sync() before teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=4

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Critical device register configurations are lost after a runtime suspend/resume cycle because `mma8452_runtime_resume` fails to restore them.
- [High] Sysfs read/write callbacks and write_raw paths access the device hardware over I2C without waking it from runtime suspend.
- [High] The `mma8452_remove()` function accesses unpowered I2C registers and causes unbalanced regulator disables if the device is runtime suspended.
- [High] `mma8452_runtime_resume` modifies device active state and configuration registers without holding `data->lock`, causing a TOCTOU race with `mma8452_change_config`.
- [High] Read-modify-write operations on I2C registers in sysfs operations are performed without holding `data->lock`.
--

--- Patch [5]: [PATCH v6 5/9] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
commit 0eb3a6b1fa62842d47690cc795da7896eb5b7153
Author: Esben Haabendal <esben@geanix.com>
Subject: iio: accel: mma8452: Allow open drain interrupt pin configuration

This commit introduces the ability to configure the interrupt pin of mma8452
chips in open-drain mode. It reads the "drive-open-drain" property during
probe and updates the hardware configuration accordingly.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 8eb97e6793d6f..d1e8eb2a4ad3a 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -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;

[Severity: High]
This is a pre-existing issue, but does mma8452_runtime_resume() fail to
restore critical configurations?

When the device autosuspends, mma8452_runtime_suspend() disables the VDD
regulator, which cuts power to the chip and loses volatile register state.
Upon resume, only CTRL_REG1 and CTRL_REG3 are restored.

Are essential configurations set during probe (like DATA_CFG, TRANSIENT_THS,
CTRL_REG4 for interrupt enables, and CTRL_REG5) lost after the first
autosuspend?


[Severity: High]
This is a pre-existing issue, but does mma8452_runtime_resume() introduce a
race condition by not holding data->lock?

The device state is modified to ACTIVE outside of the mutex lock here. If a
concurrent sysfs write checks mma8452_is_active() and sees STANDBY, could
this activation occur just before the I2C write executes?

Would this cause the hardware to silently ignore the write since the device
was unexpectedly transitioned to ACTIVE mode?


[Severity: High]
This is a pre-existing issue, but do the sysfs read/write callbacks and
write_raw paths access the device hardware over I2C without waking it from
runtime suspend?

drivers/iio/accel/mma8452.c:mma8452_write_raw() {
    ...
	if (!iio_device_claim_direct(indio_dev))
		return -EBUSY;

	ret = __mma8452_write_raw(indio_dev, chan, val, val2, mask);
    ...
}

Functions like mma8452_write_raw() and mma8452_show_hp_cutoff_avail() do
direct I2C accesses without wrapping the operation in
pm_runtime_resume_and_get() and pm_runtime_put().

If the device is autosuspended, will the unpowered chip fail to ACK the
transfers and return -EREMOTEIO?


[Severity: High]
This is a pre-existing issue, but does mma8452_remove() access unpowered I2C
registers and cause unbalanced regulator disables?

drivers/iio/accel/mma8452.c:mma8452_remove() {
    ...
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
    ...
}

If the device is runtime suspended, the vdd and vddio regulators are already
disabled. Since pm_runtime_disable() does not wake the device,
mma8452_remove() unconditionally calls mma8452_standby() which will time out
on an unpowered bus.

Will the subsequent unconditional regulator_disable() calls drive the usage
counts below zero?


[Severity: High]
This is a pre-existing issue, but are read-modify-write operations on I2C
registers in sysfs operations performed without holding data->lock?

drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
    ...
	case IIO_EV_DIR_RISING:
		val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
		if (val < 0)
			return val;
    ...
}

In functions like mma8452_write_event_config(), an I2C read of the
configuration register happens completely outside data->lock.

If two threads execute this concurrently, could they both read the old
state, modify different bits, and serialize on mma8452_change_config(),
causing the second thread to unconditionally overwrite the first thread's
modifications?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=5

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Hardware-Software Polarity Mismatch

Pre-existing issues:
- [High] PM runtime usage counter leaks permanently on I2C read/write errors.
- [High] Hardware state is accessed over I2C before acquiring runtime PM references, failing if the device is suspended.
--

--- Patch [3]: [PATCH v6 3/9] iio: accel: mma8452: Only apply trigger type when not set by firmware ---
commit b862ff26c69f226387993dc22a2537b8dc72d9fd
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Only apply trigger type when not set by firmware

This patch modifies the mma8452 driver to only apply a default active-low
trigger type if the firmware does not specify one. This aims to improve
backward compatibility while respecting firmware configuration.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index f645a5c6fd1c2..1fb43c5b0b72f 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);

[Severity: Medium]
Does this configuration leave the hardware and software out of sync?

The code reads the firmware's IRQ trigger type and requests the IRQ with
it, but the actual MMA8452 sensor hardware polarity doesn't appear to be
updated. If the firmware requests an active-high or rising-edge trigger,
the host interrupt controller is configured for active-high, but the sensor
would continue to drive its default active-low signal.

Unless there is an external hardware inverter on the board, could this 
mismatch break interrupts? Should the driver read the requested trigger type
and configure the IPOL bit in CTRL_REG3 to match?

>  		if (ret)
>  			goto buffer_cleanup;
>  	}


While reviewing the I2C handling in this driver, I also noticed a couple of
other issues.

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak of the runtime
PM usage counter when handling I2C errors.

In mma8452_read(), the device is woken up, incrementing the PM counter:

mma8452_read() {
    ...
    ret = mma8452_set_runtime_pm_state(data->client, true);
    ...
    ret = i2c_smbus_read_i2c_block_data(data->client, ...);
    if (ret < 0)
        return ret;
}

If the I2C read fails, the function returns immediately and bypasses the
matching mma8452_set_runtime_pm_state() false call. Could this permanently
prevent the device from re-entering autosuspend?

A similar pattern appears to exist in mma8452_write_event_config() where
an early return on a failed I2C read skips restoring the PM state.


[Severity: High]
This is a pre-existing issue, but could some I2C accesses fail if the
device is currently suspended?

In mma8452_read(), the code checks for data readiness before waking up the
device:

mma8452_read() {
    ...
    ret = mma8452_drdy(data);
    if (ret < 0)
        return ret;

    ret = mma8452_set_runtime_pm_state(data->client, true);
}

Because mma8452_drdy() executes an I2C read against the MMA8452_STATUS
register, it happens before the power regulators are enabled. If the device
has already autosuspended (after 2 seconds of inactivity), will this I2C
read hit powered-off silicon and fail?

A similar direct I2C read without a runtime PM reference seems to happen
in mma8452_read_raw() for IIO_CHAN_INFO_CALIBBIAS.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Removing data->lock exposes mma8452_read() to race conditions with PM auto-suspend and event configuration.

Pre-existing issues:
- [High] Polling for DRDY before PM runtime wake-up leads to I/O timeouts and errors.
- [High] PM runtime usage count leak on error paths.
- [High] Device is permanently locked in STANDBY mode on I2C write failure.
- [Medium] Unprotected read-modify-write operations on event configuration registers.
- [Medium] Kernel stack information leak due to mishandling of short I2C block reads.
--

--- Patch [7]: [PATCH v6 7/9] iio: accel: mma8452: Drop unneeded lock acquire on read ---
commit 9c2b3fc538cb28ca3a39bb1585927926bb25f284
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Drop unneeded lock acquire on read

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.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7ef1a9a91c31f..9ae2c3e605761 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);

[Severity: High]
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.

> -		mutex_unlock(&data->lock);
>  		iio_device_release_direct(indio_dev);
>  		if (ret < 0)
>  			return ret;

[Severity: High]
This is a pre-existing issue, but will polling for DRDY before PM runtime
wake-up lead to I/O timeouts and errors?

In mma8452_read():

	int ret = mma8452_drdy(data);

	if (ret < 0)
		return ret;

	ret = mma8452_set_runtime_pm_state(data->client, true);

If the device is in auto-suspend, its regulators are turned off by
mma8452_runtime_suspend().

The I2C read in mma8452_drdy() will communicate with a powered-off device,
resulting in immediate I/O errors or timeouts.

Should the PM runtime resume be performed before any hardware register polling?

[Severity: High]
This is a pre-existing issue, but is there a PM runtime usage count leak on
error paths?

In mma8452_read():

	ret = mma8452_set_runtime_pm_state(data->client, true);
	if (ret)
		return ret;

	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;

If i2c_smbus_read_i2c_block_data() fails, it returns the error code
immediately. This bypasses the subsequent call to
mma8452_set_runtime_pm_state(data->client, false), permanently leaking the
usage count.

A similar leak occurs in mma8452_write_event_config():

	ret = mma8452_set_runtime_pm_state(data->client, state);
	if (ret)
		return ret;

	switch (dir) {
	case IIO_EV_DIR_FALLING:
        ...
	case IIO_EV_DIR_RISING:
		val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
		if (val < 0)
			return val;

If this I2C read fails and returns early, the PM runtime decrement is also
missed.

[Severity: Medium]
This is a pre-existing issue, but are the read-modify-write operations on
event configuration registers properly protected?

In mma8452_write_event_config():

	case IIO_EV_DIR_RISING:
		val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
		if (val < 0)
			return val;

The initial I2C read is performed without holding data->lock, while the write
is performed by mma8452_change_config() which does hold the lock.

Since IIO event sysfs attributes do not hold the IIO mlock, concurrent writes
to different event attributes can interleave.

Could two threads perform the unprotected read simultaneously, resulting in the
second thread silently overwriting the first thread's register modifications?

[Severity: High]
This is a pre-existing issue, but can an I2C write failure leave the device
permanently locked in STANDBY mode?

In mma8452_change_config():

	/* config can only be changed when in standby */
	if (is_active > 0) {
		ret = mma8452_standby(data);
		if (ret < 0)
			goto fail;
	}

	ret = i2c_smbus_write_byte_data(data->client, reg, val);
	if (ret < 0)
		goto fail;

	if (is_active > 0) {
		ret = mma8452_active(data);
...
fail:
	mutex_unlock(&data->lock);

	return ret;

If the I2C write fails, the code jumps to the fail label, bypassing the call to
mma8452_active().

Does this leave the device stuck in STANDBY mode, which breaks all future data
measurements?

[Severity: Medium]
This is a pre-existing issue, but might the mishandling of short I2C block
reads leak uninitialized kernel stack memory?

In mma8452_read():

	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;

	ret = mma8452_set_runtime_pm_state(data->client, false);

	return ret;

i2c_smbus_read_i2c_block_data() returns the number of bytes successfully read.
If a short read occurs, it returns a positive value and passes the if (ret < 0)
check.

The subsequent call to mma8452_set_runtime_pm_state() will overwrite ret with 0
on success.

mma8452_read_raw() assumes the stack array buffer is fully initialized and
processes the uninitialized bytes, which are then returned to userspace.

Should the check verify if exactly the requested number of bytes were read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=7

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 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

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

[Esben Haabendal]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Low] Undocumented scope modification and suboptimal pointer dereferencing.
>
> Pre-existing issues:
> - [High] Device teardown accesses powered-off hardware and causes unbalanced regulator disables.
> - [Low] Unconditional assignment of PM callbacks causes dead code when PM is disabled.
> --
>
> --- Patch [6]: [PATCH v6 6/9] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe() ---
> commit 04da87f5e80c506bb658d3dd5161087ce55fa6b0
> Author: Esben Haabendal <esben@geanix.com>
>
> iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()
>
> This commit refactors mma8452_probe() to use a local 'dev' variable instead
> of repeatedly referencing &client->dev. It aims to improve code readability.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index d1e8eb2a4ad3a..7ef1a9a91c31f 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>
> [ ... ]
>
>> @@ -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");
>
> [Severity: Low]
> The commit message states the changes are restricted to reusing the dev
> pointer in mma8452_probe(), but this modifies mma8452_runtime_suspend().
> Should the commit message be updated to reflect this scope change?
>
> Also, since the function already receives a struct device *dev parameter,
> and &client->dev evaluates to that exact same pointer, wouldn't it be
> simpler to use dev directly here rather than dereferencing client?

Yes. Using the dev pointer directly is the right thing to do.  Added for
next version.

And also, the change that adds the client pointer will be rebased into
patch 4, where the client pointer is used. Sorry about that.

>>  		return -EAGAIN;
>>  	}
>
>
> As a related note, looking at the device teardown path in mma8452_remove():
>
> static void mma8452_remove(struct i2c_client *client)
> {
> [ ... ]
> 	pm_runtime_disable(&client->dev);
> 	pm_runtime_set_suspended(&client->dev);
> [ ... ]
> 	mma8452_standby(iio_priv(indio_dev));
>
> 	regulator_disable(data->vddio_reg);
> 	regulator_disable(data->vdd_reg);
> }
>
> [Severity: High]
> This is a pre-existing issue, but does this teardown sequence access
> potentially powered-off hardware?
>
> If the device is in the RPM_SUSPENDED state when remove() is called, its
> regulators have already been disabled by mma8452_runtime_suspend(). The
> call to mma8452_standby() attempts an I2C write to the powered-off device,
> which might hang the I2C bus or cause transaction timeouts.
>
> Furthermore, unconditionally calling regulator_disable() drops the usage
> counts below zero, triggering a kernel warning for unbalanced disables.
>
> Should pm_runtime_get_sync(&client->dev) be called at the beginning of
> mma8452_remove() to ensure the hardware is active before teardown?
>
>
> Finally, regarding the PM ops assignment in the driver struct:
>
> [Severity: Low]
> This isn't a bug introduced by this patch, but does the unconditional
> assignment of &mma8452_pm_ops to the .pm field in mma8452_driver produce
> dead code when CONFIG_PM is disabled?
>
> According to the power management subsystem guidelines, this pointer should
> be wrapped in pm_ptr(&mma8452_pm_ops) to allow the compiler to eliminate
> the dead code.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=6

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

[Esben Haabendal]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] If regulator_disable() fails in mma8452_runtime_suspend(), the data->suspended flag is not rolled back, leading to a permanent IRQ storm on subsequent interrupts.
>
> Pre-existing issues:
> - [High] mma8452_remove() does not wake up the device before disabling runtime PM, leading to unbalanced regulator disables (underflow) and illegal I2C access while powered off.
> - [High] Registers are not restored after runtime resume, causing the device to lose critical configuration.
> --
>
> --- Patch [4]: [PATCH v6 4/9] iio: accel: mma8452: Support interrupt sharing ---
> commit 5bc641da9d60875f38db9937a3a1dae1afd9a94e
> Author: Esben Haabendal <esben@geanix.com>
>
> iio: accel: mma8452: Support interrupt sharing
>
> This commit adds handling of runtime PM suspension in the interrupt handler
> to allow sharing the interrupt with other devices. It introduces a suspended
> flag and uses synchronize_irq() to protect against race conditions during
> runtime suspend and device removal.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 1fb43c5b0b72f..8eb97e6793d6f 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
> [ ... ]
>> @@ -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;
>> +	}
>
> [Severity: High]
> Is the READ_ONCE(data->suspended) check reachable during normal suspend?
>
> Since pm_runtime_get_if_active() returns 0 when the device state is
> RPM_SUSPENDING, it seems the handler will return early before reaching
> the data->suspended check.

Yes. That is on purpose. If pm_runtime_get_if_active() returns 0,
runtime PM is enabled and the device is not RPM_ACTIVE. We don't want to
process irq for the device, and we must not call pm_runtime_put() as
that would cause counter underflow.

> [ ... ]
>> @@ -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");
>>  		return ret;
>>  	}
>
> [Severity: High]
> If regulator_disable() fails here, the function returns without resetting
> data->suspended to false.
>
> If this happens, the PM core leaves the device in RPM_ACTIVE. Would this
> cause a permanent IRQ storm since the next interrupt will see pm_status > 0,
> but data->suspended is true, causing it to continually return IRQ_NONE
> without clearing the level-triggered interrupt?

If regulator_disable() fails here, the device will be in STANDBY mode,
and interrupts should not happen. The INT_SOURCE (MMA8452_INT_SRC)
register is reset when switching from ACTIVE to STANDBY mode, and the
analog subsystem is disabled, so no irq should be asserted by the chip.

> [ ... ]
>> @@ -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;
>
> [Severity: High]
> This is a pre-existing issue, but does mma8452_runtime_resume() need to
> restore other device registers like MMA8452_CTRL_REG4 and
> MMA8452_DATA_CFG?
>
> When the regulators are disabled during suspend, the device loses power and
> these registers are reset. If they aren't restored here, will the device
> lose its configuration and permanently disable hardware interrupts since
> CTRL_REG4 defaults to 0?

According to data sheet, both CTRL_REG4 (0x2d) and XYZ_DATA_CFG (0x0e)
is preserved on transition from ACTIVE to STANDBY. But yes, we probably
need to restore these when regulator have been disabled.

Is it okay if I address this in a follow-up patch?

> [ ... ]
>> @@ -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);
>>
>
> [Severity: High]
> This is a pre-existing issue, but if the device is unbound via sysfs while
> runtime-suspended, will mma8452_remove() run into issues?
>
> drivers/iio/accel/mma8452.c:mma8452_remove() {
>     ...
> 	pm_runtime_disable(&client->dev);
> 	pm_runtime_set_suspended(&client->dev);
>     ...
> 	mma8452_standby(iio_priv(indio_dev));
>
> 	regulator_disable(data->vddio_reg);
> 	regulator_disable(data->vdd_reg);
> }
>
> Since pm_runtime_disable() leaves the device suspended, won't
> mma8452_standby() attempt an I2C write on an unpowered chip? Also, won't
> calling regulator_disable() here cause an underflow since the regulators
> were already disabled during runtime suspend? Does this need a
> pm_runtime_get_sync() before teardown?

I don't know if that is the correct approach. But it does look like an
easy fix.

/Esben

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

[Esben Haabendal]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Critical device register configurations are lost after a runtime suspend/resume cycle because `mma8452_runtime_resume` fails to restore them.
> - [High] Sysfs read/write callbacks and write_raw paths access the device hardware over I2C without waking it from runtime suspend.
> - [High] The `mma8452_remove()` function accesses unpowered I2C registers and causes unbalanced regulator disables if the device is runtime suspended.
> - [High] `mma8452_runtime_resume` modifies device active state and configuration registers without holding `data->lock`, causing a TOCTOU race with `mma8452_change_config`.
> - [High] Read-modify-write operations on I2C registers in sysfs operations are performed without holding `data->lock`.
> --
>
> --- Patch [5]: [PATCH v6 5/9] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
> commit 0eb3a6b1fa62842d47690cc795da7896eb5b7153
> Author: Esben Haabendal <esben@geanix.com>
> Subject: iio: accel: mma8452: Allow open drain interrupt pin configuration
>
> This commit introduces the ability to configure the interrupt pin of mma8452
> chips in open-drain mode. It reads the "drive-open-drain" property during
> probe and updates the hardware configuration accordingly.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 8eb97e6793d6f..d1e8eb2a4ad3a 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
> [ ... ]
>> @@ -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;
>
> [Severity: High]
> This is a pre-existing issue, but does mma8452_runtime_resume() fail to
> restore critical configurations?
>
> When the device autosuspends, mma8452_runtime_suspend() disables the VDD
> regulator, which cuts power to the chip and loses volatile register state.
> Upon resume, only CTRL_REG1 and CTRL_REG3 are restored.
>
> Are essential configurations set during probe (like DATA_CFG, TRANSIENT_THS,
> CTRL_REG4 for interrupt enables, and CTRL_REG5) lost after the first
> autosuspend?

Yes, I think they are lost. Looks like suspend/resume is not fully
implemented in this driver.

> [Severity: High]
> This is a pre-existing issue, but does mma8452_runtime_resume() introduce a
> race condition by not holding data->lock?
>
> The device state is modified to ACTIVE outside of the mutex lock here. If a
> concurrent sysfs write checks mma8452_is_active() and sees STANDBY, could
> this activation occur just before the I2C write executes?
>
> Would this cause the hardware to silently ignore the write since the device
> was unexpectedly transitioned to ACTIVE mode?

Yes. That sounds like something that could happen. But even more, all
the sysfs attributes does not even try to resume the device if it is
suspended. Which is also your next comment/issue.

So as mentioned above, suspend/resume implementation in this driver
seems very much incomplete.

> [Severity: High]
> This is a pre-existing issue, but do the sysfs read/write callbacks and
> write_raw paths access the device hardware over I2C without waking it from
> runtime suspend?
>
> drivers/iio/accel/mma8452.c:mma8452_write_raw() {
>     ...
> 	if (!iio_device_claim_direct(indio_dev))
> 		return -EBUSY;
>
> 	ret = __mma8452_write_raw(indio_dev, chan, val, val2, mask);
>     ...
> }
>
> Functions like mma8452_write_raw() and mma8452_show_hp_cutoff_avail() do
> direct I2C accesses without wrapping the operation in
> pm_runtime_resume_and_get() and pm_runtime_put().
>
> If the device is autosuspended, will the unpowered chip fail to ACK the
> transfers and return -EREMOTEIO?

Probably will.

> [Severity: High]
> This is a pre-existing issue, but does mma8452_remove() access unpowered I2C
> registers and cause unbalanced regulator disables?
>
> drivers/iio/accel/mma8452.c:mma8452_remove() {
>     ...
> 	mma8452_standby(iio_priv(indio_dev));
>
> 	regulator_disable(data->vddio_reg);
> 	regulator_disable(data->vdd_reg);
>     ...
> }
>
> If the device is runtime suspended, the vdd and vddio regulators are already
> disabled. Since pm_runtime_disable() does not wake the device,
> mma8452_remove() unconditionally calls mma8452_standby() which will time out
> on an unpowered bus.
>
> Will the subsequent unconditional regulator_disable() calls drive the usage
> counts below zero?

Yes and yes.

> [Severity: High]
> This is a pre-existing issue, but are read-modify-write operations on I2C
> registers in sysfs operations performed without holding data->lock?

Yes. But I don't think the data->lock is supposed to be used for this
purpose. It looks like the purpose is to protect the ACTIVE/STANDBY
state handling in mma8452_change_config().

>
> drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
>     ...
> 	case IIO_EV_DIR_RISING:
> 		val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
> 		if (val < 0)
> 			return val;
>     ...
> }
>
> In functions like mma8452_write_event_config(), an I2C read of the
> configuration register happens completely outside data->lock.
>
> If two threads execute this concurrently, could they both read the old
> state, modify different bits, and serialize on mma8452_change_config(),
> causing the second thread to unconditionally overwrite the first thread's
> modifications?

Looks like a bug. I will try to write up a fix for this and all the
other similar race conditions that this driver seems to have quite a few
of.

/Esben

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=5

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

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

[Andy Shevchenko]

On Tue, Aug 25, 2026 at 10:27:44AM +0200, Esben Haabendal wrote:
> 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.

...

>  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);

This change is unrelated and should be as simple as

	struct iio_dev *indio_dev = dev_get_drvdata(dev);

OTOH, this might require the full device.h to be included...
Either way, it's not for this patch.

>  	struct mma8452_data *data = iio_priv(indio_dev);
>  	int ret;

>  	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;
>  	}

Why? Just use given 'dev' as is.

-- 
With Best Regards,
Andy Shevchenko