[PATCH v9 00/10] io: accel: mma8452: Allow open drain interrupt pin configuration

[PATCH v9 00/10] 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 v9:
- Fix documentation of voltage regulator field(s) in struct mma8452_data
  kernel-doc block.
- Fix a couple of nitpicks in struct mma8452_data kernel-doc block.
- Switch to STANDBY mode before writing to CTRL_REG4.
- Add patch fixing runtime PM bugs in mma8452_read().
- Add comment about handling of -EINVAL return value from
  pm_runtime_get_if_active() in mma8452_interrupt().
- Fix for race between mma8452_remove() and mma8452_interrupt(), by calling
  free_irq() before pm_runtime_disable() instead of after.
- Fix mixing of guards and goto in mma8452_runtime_suspend(). Using guards
  will be re-introduced later on, when mixing can be avoided.
- Link to v8: https://patch.msgid.link/20260907-mma8452-open-drain-v8-0-c17407e22118@geanix.com

Changes in v8:
- Move patch 7 (with Fixes: ...) to go before the non-fixes patches.
- Rebased from v7.3-rc1 to current iio/togreg branch.
- Drop patch 8 (identical patch from Sanjay Chitroda already in togreg
  branch).
- Add new patch fixing a strangely extra indentation of a comment block.
- Add new patch adding documentation for struct mma8452_data.
- Move patch 4 to last in series, and rework the synchronization between
  suspend/resume and interrupt handler.
- Link to v7: https://patch.msgid.link/20260831-mma8452-open-drain-v7-0-22946812c928@geanix.com

Changes in v7:
- Use dev pointer directly in mma8452_runtime_suspend() also.
- Move struct i2c_client *client definition to patch 4.
- Drop patch 7 (Drop unneeded lock acquire on read).
- Link to v6: https://patch.msgid.link/20260825-mma8452-open-drain-v6-0-9b252804ee80@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 (10):
      dt-bindings: iio: accel: mma8452: Add drive-open-drain
      iio: accel: mma8452: Fix use-after-free bug in error error path
      iio: accel: mma8452: Fix runtime PM bugs in mma8452_read()
      iio: accel: mma8452: Optimize struct mma8452_data member orders
      iio: accel: mma8452: Only apply trigger type when not set by firmware
      iio: accel: mma8452: Fix unintended comment indent
      iio: accel: mma8452: Add comment block for struct mma8452_data
      iio: accel: mma8452: Allow open drain interrupt pin configuration
      iio: accel: mma8452: Use proper error code when missing device model
      iio: accel: mma8452: Support interrupt sharing

 .../devicetree/bindings/iio/accel/fsl,mma8452.yaml |   6 +
 drivers/iio/accel/mma8452.c                        | 201 +++++++++++++++++----
 2 files changed, 167 insertions(+), 40 deletions(-)
---
base-commit: 746ce4dd0ea41825a1dfa5e9d8c22e46a4cf7101
change-id: 20250401-mma8452-open-drain-81577c41375c

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

[PATCH v9 01/10] 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 v9 02/10] 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
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
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 fe62a903f0e2..a937cbd84f30 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1681,7 +1681,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)
@@ -1692,6 +1692,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 v9 03/10] iio: accel: mma8452: Fix runtime PM bugs in mma8452_read()

[Esben Haabendal]

When runtime PM support was added [1], the implementation in mma8452_read()
was done incorrectly, as it calls mma8452_drdy() before ensuring runtime pm
has set the device active, so it can cause read from STATUS register while
device is suspended/off. At the same time, the return value from
i2c_smbus_read_i2c_block_data() was discarded, and the function would
return the value returned from pm_runtime_put_autosuspend() instead.

When the latter bug described above was fixed [2], another bug was
introduced, as mma8452_read() would now leak the runtime PM reference
counter when i2c_smbus_read_i2c_block_data() failed.

This commit corrects this mess.

[1] commit 96c0cb2bbfe0 ("iio: mma8452: add support for runtime power management")
[2] commit 5bdff291d20c ("iio: accel: mma8452: handle I2C read error(s) in mma8452_read()")

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 | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index a937cbd84f30..a77c4b88b61d 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -242,21 +242,25 @@ static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
 
 static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
 {
-	int ret = mma8452_drdy(data);
-
-	if (ret < 0)
-		return ret;
+	int ret;
 
 	ret = mma8452_set_runtime_pm_state(data->client, true);
 	if (ret)
 		return ret;
 
+	ret = mma8452_drdy(data);
+	if (ret < 0)
+		goto out_runtime_put;
+
 	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
 					    3 * sizeof(__be16), (u8 *)buf);
 	if (ret < 0)
-		return ret;
+		goto out_runtime_put;
+
+	return mma8452_set_runtime_pm_state(data->client, false);
 
-	ret = mma8452_set_runtime_pm_state(data->client, false);
+out_runtime_put:
+	mma8452_set_runtime_pm_state(data->client, false);
 
 	return ret;
 }

-- 
2.55.0

[PATCH v9 04/10] 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 a77c4b88b61d..8d70fe89110c 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -108,10 +108,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_bulk_data regs[2];
 
 	/* Ensure correct alignment of time stamp when present */
@@ -119,6 +116,10 @@ struct mma8452_data {
 		__be16 channels[3];
 		aligned_s64 ts;
 	} buffer;
+
+	int sleep_val;
+	u8 ctrl_reg1;
+	u8 data_cfg;
 };
 
  /**

-- 
2.55.0

[PATCH v9 05/10] 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 8d70fe89110c..72b4ab32775a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1669,9 +1669,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 v9 06/10] iio: accel: mma8452: Fix unintended comment indent

[Esben Haabendal]

The extra space this block was indented with looks quite untraditional, and
does not align with the common style used in the kernel.

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

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 72b4ab32775a..b70bc8638128 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -122,21 +122,21 @@ struct mma8452_data {
 	u8 data_cfg;
 };
 
- /**
-  * struct mma8452_event_regs - chip specific data related to events
-  * @ev_cfg:			event config register address
-  * @ev_cfg_ele:			latch bit in event config register
-  * @ev_cfg_chan_shift:		number of the bit to enable events in X
-  *				direction; in event config register
-  * @ev_src:			event source register address
-  * @ev_ths:			event threshold register address
-  * @ev_ths_mask:		mask for the threshold value
-  * @ev_count:			event count (period) register address
-  *
-  * Since not all chips supported by the driver support comparing high pass
-  * filtered data for events (interrupts), different interrupt sources are
-  * used for different chips and the relevant registers are included here.
-  */
+/**
+ * struct mma8452_event_regs - chip specific data related to events
+ * @ev_cfg:			event config register address
+ * @ev_cfg_ele:			latch bit in event config register
+ * @ev_cfg_chan_shift:		number of the bit to enable events in X
+ *				direction; in event config register
+ * @ev_src:			event source register address
+ * @ev_ths:			event threshold register address
+ * @ev_ths_mask:		mask for the threshold value
+ * @ev_count:			event count (period) register address
+ *
+ * Since not all chips supported by the driver support comparing high pass
+ * filtered data for events (interrupts), different interrupt sources are
+ * used for different chips and the relevant registers are included here.
+ */
 struct mma8452_event_regs {
 	u8 ev_cfg;
 	u8 ev_cfg_ele;

-- 
2.55.0

[PATCH v9 07/10] iio: accel: mma8452: Add comment block for struct mma8452_data

[Esben Haabendal]

The struct mma8452_data is central for this driver, and it makes sense to
have a description of the fields in it to make it easier to work with the
driver.

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

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index b70bc8638128..223bef4f7baf 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -104,6 +104,21 @@
 
 #define MMA8452_AUTO_SUSPEND_DELAY_MS		2000
 
+/**
+ * struct mma8452_data - IIO device private data structure
+ * @client:			the I2C client object
+ * @lock:			mutex for synchronziation of register
+ *				read-modify-write sequences and holding chip in
+ *				STANDBY mode while writing to registers
+ * @orientation:		mounting matrix, flipped axis etc.
+ * @chip_info:			chip specific data
+ * @regs:			reference to voltage regulators
+ * @buffer:			triggered buffer
+ * @sleep_val:			time in ms to sleep while waiting for drdy
+ * @ctrl_reg1:			CTRL_REG1 register shadow value
+ * @data_cfg:			DATA_CFG register shadow value
+ * @open_drain:			true for irq pin in open-drain mode
+ */
 struct mma8452_data {
 	struct i2c_client *client;
 	struct mutex lock;

-- 
2.55.0

[PATCH v9 08/10] 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 223bef4f7baf..5d44f0066b1e 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -83,6 +83,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
@@ -135,6 +137,7 @@ struct mma8452_data {
 	int sleep_val;
 	u8 ctrl_reg1;
 	u8 data_cfg;
+	bool open_drain;
 };
 
 /**
@@ -662,6 +665,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)
 {
@@ -1668,6 +1687,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);
 
@@ -1795,6 +1819,10 @@ static int mma8452_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	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 v9 09/10] iio: accel: mma8452: Use proper error code when missing device model

[Esben Haabendal]

Switch -ENODEV error on i2c_get_match_data() failure to -ENODATA to
satisfy the IIO coding style.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 5d44f0066b1e..5aea6bf3312a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1594,7 +1594,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

[PATCH v9 10/10] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal]

Adding support for sharing interrupt line with other device requires the
interrupt handler to handle runtime PM suspension properly, ignoring the
irq if the device is suspended (maybe even off). And while at it, we use
the PM reference to ensure we do not get suspended while processing an irq.

In order to prevent the chip from raising irq while suspended (that is when
using fixed regulator, where suspend just means setting the device in
STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
then restores the value again when resuming.

The scoped_guard in mma8452_runtime_suspend() is changed to a plain
mutex_lock() instead, both to prevent mixing guards and goto, but also to
ensure that we stay in STANDBY mode while writing to CTRL_REG4 and all the
way up to disabling the device as much as possible.

With that in place, it is safe to add the IRQF_SHARED flag.

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.

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

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 5aea6bf3312a..2a980c0c2e03 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -119,6 +119,7 @@
  * @sleep_val:			time in ms to sleep while waiting for drdy
  * @ctrl_reg1:			CTRL_REG1 register shadow value
  * @data_cfg:			DATA_CFG register shadow value
+ * @ctrl_reg4:			CTRL_REG4 register value to restore on resume
  * @open_drain:			true for irq pin in open-drain mode
  */
 struct mma8452_data {
@@ -137,6 +138,7 @@ struct mma8452_data {
 	int sleep_val;
 	u8 ctrl_reg1;
 	u8 data_cfg;
+	u8 ctrl_reg4;
 	bool open_drain;
 };
 
@@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 {
 	struct iio_dev *indio_dev = p;
 	struct mma8452_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
 	irqreturn_t ret = IRQ_NONE;
+	int pm_status;
 	int src;
 
+	pm_status = pm_runtime_get_if_active(dev);
+	if (pm_status == 0)
+		return IRQ_NONE; /* device is powered down */
+
+	/*
+	 * pm_status is now 1 or -EINVAL. If pm_status==1, runtime PM is enabled
+	 * and device is RPM_ACTIVE. If pm_status==-EINVAL, runtime PM is
+	 * disabled (e.g. CONFIG_PM not enabled), and we can/must assume device
+	 * is active.
+	 */
+
 	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);
@@ -1120,6 +1135,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 		ret = IRQ_HANDLED;
 	}
 
+out_runtime_put:
+	if (pm_status > 0)
+		pm_runtime_put_autosuspend(dev);
+
 	return ret;
 }
 
@@ -1715,7 +1734,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)
@@ -1771,11 +1790,12 @@ static void mma8452_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 
-	pm_runtime_disable(dev);
-	pm_runtime_set_suspended(dev);
-
 	if (client->irq)
 		free_irq(client->irq, indio_dev);
+	/* No irq will fire beyond this point */
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
 	mma8452_trigger_cleanup(indio_dev);
@@ -1787,29 +1807,67 @@ 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;
 
-	scoped_guard(mutex, &data->lock)
-		ret = mma8452_standby(data);
+	mutex_lock(&data->lock);
+
+	ret = mma8452_standby(data);
 	if (ret < 0) {
-		dev_err(dev, "powering off device failed\n");
-		return -EAGAIN;
+		dev_err(dev, "transition to STANDBY mode failed\n");
+		ret = -EAGAIN;
+		goto out_unlock;
 	}
 
+	ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG4);
+	if (ret < 0) {
+		dev_warn(dev, "backing up CTRL_REG4 failed\n");
+		ret = -EAGAIN;
+		goto out_active;
+	} else
+		data->ctrl_reg4 = ret;
+
+	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, 0);
+	if (ret) {
+		dev_warn(dev, "disabling interrupt sources (CTRL_REG4) failed\n");
+		ret = -EAGAIN;
+		goto out_active;
+	}
+
+	/*
+	 * Interrupt line should be deasserted now, so we just need ensure any
+	 * mid-flight irq is completed (will return IRQ_NONE due to
+	 * pm_status==0).
+	 */
+	if (client->irq)
+		synchronize_irq(client->irq);
+
 	ret = regulator_bulk_disable(ARRAY_SIZE(data->regs), data->regs);
 	if (ret) {
 		dev_err(dev, "failed to disable regulators\n");
-		return ret;
+		goto out_restore_ctrl_reg4;
 	}
 
+	mutex_unlock(&data->lock);
 	return 0;
+
+out_restore_ctrl_reg4:
+	if (i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, data->ctrl_reg4))
+		dev_warn(dev, "restoring CTRL_REG4 failed\n");
+out_active:
+	if (mma8452_active(data))
+		dev_warn(dev, "failed to switch back to ACTIVE mode\n");
+out_unlock:
+	mutex_unlock(&data->lock);
+	return ret;
 }
 
 static int mma8452_runtime_resume(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, sleep_val;
 
@@ -1823,6 +1881,10 @@ static int mma8452_runtime_resume(struct device *dev)
 	if (ret)
 		goto runtime_resume_failed;
 
+	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, data->ctrl_reg4);
+	if (ret)
+		goto runtime_resume_failed;
+
 	ret = mma8452_active(data);
 	if (ret < 0)
 		goto runtime_resume_failed;

-- 
2.55.0

Re: [PATCH v9 07/10] iio: accel: mma8452: Add comment block for struct mma8452_data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] doc: typo in mma8452_data kernel-doc comment
--

commit 984d9aedf92a8a6dbe4c1ae78737aa03bd9dfc22
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Add comment block for struct mma8452_data

The struct mma8452_data is central for this driver, and it makes sense to
have a description of the fields in it to make it easier to work with the
driver.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index b70bc86381280..223bef4f7baf8 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -104,6 +104,21 @@
>  
>  #define MMA8452_AUTO_SUSPEND_DELAY_MS		2000
>  
> +/**
> + * struct mma8452_data - IIO device private data structure
> + * @client:			the I2C client object
> + * @lock:			mutex for synchronziation of register

[Severity: Low]
This isn't a bug, but there's a typo here (synchronziation).

> + *				read-modify-write sequences and holding chip in
> + *				STANDBY mode while writing to registers
> + * @orientation:		mounting matrix, flipped axis etc.
> + * @chip_info:			chip specific data
> + * @regs:			reference to voltage regulators
> + * @buffer:			triggered buffer
> + * @sleep_val:			time in ms to sleep while waiting for drdy
> + * @ctrl_reg1:			CTRL_REG1 register shadow value
> + * @data_cfg:			DATA_CFG register shadow value
> + * @open_drain:			true for irq pin in open-drain mode
> + */
>  struct mma8452_data {
>  	struct i2c_client *client;
>  	struct mutex lock;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-mma8452-open-drain-v9-0-0dd24fa2f3d7@geanix.com?part=7

Re: [PATCH v9 05/10] iio: accel: mma8452: Only apply trigger type when not set by firmware

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: accel: mma8452: mismatch between host interrupt config and sensor polarity
--

commit d5f9cc43dbeb055ea3109fb839dcfa1634da10c0
Author: Esben Haabendal <esben@geanix.com>

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

This commit updates the probe function to use the interrupt trigger type
provided by firmware instead of unconditionally hardcoding it to active-low.
If the firmware does not specify a trigger type, it falls back to the
default active-low configuration.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 8d70fe89110cd..72b4ab32775a6 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1669,9 +1669,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: High]
If the firmware specifies a non-default interrupt polarity such as
active-high, could this result in an interrupt storm?

While this code programs the host interrupt controller with the requested
polarity, does the sensor's hardware polarity also need to be updated? 
The sensor's default active-low configuration means its inactive state is
logic high. If the host controller is configured for active-high, wouldn't 
it constantly trigger on this logic high, return IRQ_NONE from the handler,
and endlessly re-trigger?

Should the sensor's CTRL_REG3 IPOL bit be programmed to match the requested
irq_flags, ensuring the device output aligns with the host controller's
expectations?

>  		if (ret)
>  			goto buffer_cleanup;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-mma8452-open-drain-v9-0-0dd24fa2f3d7@geanix.com?part=5

Re: [PATCH v9 10/10] iio: accel: mma8452: Support interrupt sharing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] iio: accel: mma8452: Interrupt storm livelock when IRQ fires during RPM_SUSPENDING
- [High] iio: accel: mma8452: Invalid I2C access during system suspend due to mishandled -EINVAL
- [Medium] iio: accel: mma8452: Race condition in probe error path due to pm_runtime_disable() before free_irq()
--

commit bf19c63685f6fff116a3a5358196fd66dff6090a
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Support interrupt sharing

This commit adds support for shared interrupts to the mma8452 driver. It
modifies the interrupt handler to check the runtime PM status and ignores
the interrupt if the device is suspended. It also updates the runtime
suspend and resume paths to properly disable and restore interrupt sources.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 5aea6bf3312af..2a980c0c2e03a 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

> @@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>  {
>  	struct iio_dev *indio_dev = p;
>  	struct mma8452_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
>  	irqreturn_t ret = IRQ_NONE;
> +	int pm_status;
>  	int src;
>  
> +	pm_status = pm_runtime_get_if_active(dev);
> +	if (pm_status == 0)
> +		return IRQ_NONE; /* device is powered down */

[Severity: Critical]
If the device asserts its level-triggered interrupt when the PM core is
transitioning the device to RPM_SUSPENDING (e.g., when the autosuspend
timer expires), pm_runtime_get_if_active() will return 0.

Does returning IRQ_NONE without clearing the hardware interrupt here cause
a permanent interrupt storm? Since the IRQ line is shared and unmasked upon
return, the still-asserted hardware interrupt might immediately re-trigger
the handler, potentially starving the worker thread that executes
mma8452_runtime_suspend() and hard-locking the system.

[Severity: High]
During system suspend, pm_runtime_force_suspend() disables runtime PM,
which increments the disable depth. If a shared interrupt fires during this
window, pm_runtime_get_if_active() returns -EINVAL.

Since this check only catches pm_status == 0, does this allow the handler
to proceed and perform I2C reads on unpowered hardware? Per subsystem
guidelines, should this check for pm_status <= 0 instead?

> +
> +	/*
> +	 * pm_status is now 1 or -EINVAL. If pm_status==1, runtime PM is enabled
> +	 * and device is RPM_ACTIVE. If pm_status==-EINVAL, runtime PM is
> +	 * disabled (e.g. CONFIG_PM not enabled), and we can/must assume device
> +	 * is active.
> +	 */
> +
>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);

[ ... ]

> @@ -1771,11 +1790,12 @@ static void mma8452_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(indio_dev);
>  
> -	pm_runtime_disable(dev);
> -	pm_runtime_set_suspended(dev);
> -
>  	if (client->irq)
>  		free_irq(client->irq, indio_dev);
> +	/* No irq will fire beyond this point */
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);

[Severity: Medium]
This correctly fixes the race in mma8452_remove() by moving free_irq()
before pm_runtime_disable(), but does the same race still exist in the
mma8452_probe() error path?

If mma8452_probe() fails after pm_runtime_enable(), it jumps to the
runtime_suspend label:

  mma8452_probe() {
      /* ... */
  runtime_suspend:
      pm_runtime_disable(dev);
      pm_runtime_set_suspended(dev);

  free_irq:
      if (client->irq)
          free_irq(client->irq, indio_dev);
      /* ... */
  }

If a shared interrupt fires between pm_runtime_disable() and free_irq(),
could the handler attempt to access unpowered hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-mma8452-open-drain-v9-0-0dd24fa2f3d7@geanix.com?part=10

Re: [PATCH v9 10/10] 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:
> - [Critical] iio: accel: mma8452: Interrupt storm livelock when IRQ fires during RPM_SUSPENDING
> - [High] iio: accel: mma8452: Invalid I2C access during system suspend due to mishandled -EINVAL
> - [Medium] iio: accel: mma8452: Race condition in probe error path due to pm_runtime_disable() before free_irq()
> --
>
> commit bf19c63685f6fff116a3a5358196fd66dff6090a
> Author: Esben Haabendal <esben@geanix.com>
>
> iio: accel: mma8452: Support interrupt sharing
>
> This commit adds support for shared interrupts to the mma8452 driver. It
> modifies the interrupt handler to check the runtime PM status and ignores
> the interrupt if the device is suspended. It also updates the runtime
> suspend and resume paths to properly disable and restore interrupt sources.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 5aea6bf3312af..2a980c0c2e03a 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>
> [ ... ]
>
>> @@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>>  {
>>  	struct iio_dev *indio_dev = p;
>>  	struct mma8452_data *data = iio_priv(indio_dev);
>> +	struct device *dev = &data->client->dev;
>>  	irqreturn_t ret = IRQ_NONE;
>> +	int pm_status;
>>  	int src;
>>
>> +	pm_status = pm_runtime_get_if_active(dev);
>> +	if (pm_status == 0)
>> +		return IRQ_NONE; /* device is powered down */
>
> [Severity: Critical]
> If the device asserts its level-triggered interrupt when the PM core is
> transitioning the device to RPM_SUSPENDING (e.g., when the autosuspend
> timer expires), pm_runtime_get_if_active() will return 0.
>
> Does returning IRQ_NONE without clearing the hardware interrupt here cause
> a permanent interrupt storm? Since the IRQ line is shared and unmasked upon
> return, the still-asserted hardware interrupt might immediately re-trigger
> the handler, potentially starving the worker thread that executes
> mma8452_runtime_suspend() and hard-locking the system.

This should be handled by the disable of all interrupt sources (clearing
CTRL_REG4) in mma8452_runtime_suspend().

> [Severity: High]
> During system suspend, pm_runtime_force_suspend() disables runtime PM,
> which increments the disable depth. If a shared interrupt fires during this
> window, pm_runtime_get_if_active() returns -EINVAL.
>
> Since this check only catches pm_status == 0, does this allow the handler
> to proceed and perform I2C reads on unpowered hardware? Per subsystem
> guidelines, should this check for pm_status <= 0 instead?

If we bail out on pm_status <= 0, the driver would not work on systems
with CONFIG_PM not enabled.

In order to handle the difference betwen the different reasons for
-EINVAL, it would seem like we would need some #ifdef CONFIG_PM in here.
Do we really want to do that?

>> +
>> +	/*
>> +	 * pm_status is now 1 or -EINVAL. If pm_status==1, runtime PM is enabled
>> +	 * and device is RPM_ACTIVE. If pm_status==-EINVAL, runtime PM is
>> +	 * disabled (e.g. CONFIG_PM not enabled), and we can/must assume device
>> +	 * is active.
>> +	 */
>> +
>>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>
> [ ... ]
>
>> @@ -1771,11 +1790,12 @@ static void mma8452_remove(struct i2c_client *client)
>>
>>  	iio_device_unregister(indio_dev);
>>
>> -	pm_runtime_disable(dev);
>> -	pm_runtime_set_suspended(dev);
>> -
>>  	if (client->irq)
>>  		free_irq(client->irq, indio_dev);
>> +	/* No irq will fire beyond this point */
>> +
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_suspended(dev);
>
> [Severity: Medium]
> This correctly fixes the race in mma8452_remove() by moving free_irq()
> before pm_runtime_disable(), but does the same race still exist in the
> mma8452_probe() error path?
>
> If mma8452_probe() fails after pm_runtime_enable(), it jumps to the
> runtime_suspend label:
>
>   mma8452_probe() {
>       /* ... */
>   runtime_suspend:
>       pm_runtime_disable(dev);
>       pm_runtime_set_suspended(dev);
>
>   free_irq:
>       if (client->irq)
>           free_irq(client->irq, indio_dev);
>       /* ... */
>   }
>
> If a shared interrupt fires between pm_runtime_disable() and free_irq(),
> could the handler attempt to access unpowered hardware?

I will apply the same reordering in mma8452_probe().

/Esben

Re: [PATCH v9 10/10] iio: accel: mma8452: Support interrupt sharing

[Jonathan Cameron]

On Thu, 17 Sep 2026 08:40:06 +0200
Esben Haabendal <esben@geanix.com> wrote:

> <sashiko-bot@kernel.org> writes:
> 
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > - [Critical] iio: accel: mma8452: Interrupt storm livelock when IRQ fires during RPM_SUSPENDING
> > - [High] iio: accel: mma8452: Invalid I2C access during system suspend due to mishandled -EINVAL
> > - [Medium] iio: accel: mma8452: Race condition in probe error path due to pm_runtime_disable() before free_irq()
> > --
> >
So far we haven't enabled sashiko emails to the linux-iio list
(will probably move to that fairly soon) so fun side effect is this
reply was shouting into the void - except that b4 picks it up.

+CC linux-iio. I'm too lazy to add everyone by hand who was on original thread.

Key here is looks like you already plan a v10.


> > commit bf19c63685f6fff116a3a5358196fd66dff6090a
> > Author: Esben Haabendal <esben@geanix.com>
> >
> > iio: accel: mma8452: Support interrupt sharing
> >
> > This commit adds support for shared interrupts to the mma8452 driver. It
> > modifies the interrupt handler to check the runtime PM status and ignores
> > the interrupt if the device is suspended. It also updates the runtime
> > suspend and resume paths to properly disable and restore interrupt sources.
> >  
> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> >> index 5aea6bf3312af..2a980c0c2e03a 100644
> >> --- a/drivers/iio/accel/mma8452.c
> >> +++ b/drivers/iio/accel/mma8452.c  
> >
> > [ ... ]
> >  
> >> @@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
> >>  {
> >>  	struct iio_dev *indio_dev = p;
> >>  	struct mma8452_data *data = iio_priv(indio_dev);
> >> +	struct device *dev = &data->client->dev;
> >>  	irqreturn_t ret = IRQ_NONE;
> >> +	int pm_status;
> >>  	int src;
> >>
> >> +	pm_status = pm_runtime_get_if_active(dev);
> >> +	if (pm_status == 0)
> >> +		return IRQ_NONE; /* device is powered down */  
> >
> > [Severity: Critical]
> > If the device asserts its level-triggered interrupt when the PM core is
> > transitioning the device to RPM_SUSPENDING (e.g., when the autosuspend
> > timer expires), pm_runtime_get_if_active() will return 0.
> >
> > Does returning IRQ_NONE without clearing the hardware interrupt here cause
> > a permanent interrupt storm? Since the IRQ line is shared and unmasked upon
> > return, the still-asserted hardware interrupt might immediately re-trigger
> > the handler, potentially starving the worker thread that executes
> > mma8452_runtime_suspend() and hard-locking the system.  
> 
> This should be handled by the disable of all interrupt sources (clearing
> CTRL_REG4) in mma8452_runtime_suspend().
> 
> > [Severity: High]
> > During system suspend, pm_runtime_force_suspend() disables runtime PM,
> > which increments the disable depth. If a shared interrupt fires during this
> > window, pm_runtime_get_if_active() returns -EINVAL.
> >
> > Since this check only catches pm_status == 0, does this allow the handler
> > to proceed and perform I2C reads on unpowered hardware? Per subsystem
> > guidelines, should this check for pm_status <= 0 instead?  
> 
> If we bail out on pm_status <= 0, the driver would not work on systems
> with CONFIG_PM not enabled.
> 
> In order to handle the difference betwen the different reasons for
> -EINVAL, it would seem like we would need some #ifdef CONFIG_PM in here.
> Do we really want to do that?

That if_enabled() function is a pain.  Definitely not ifdef but
how about

 if (IS_ENABLED(CONFIG_PM) && ret < 0) or something like that?

> 
> >> +
> >> +	/*
> >> +	 * pm_status is now 1 or -EINVAL. If pm_status==1, runtime PM is enabled
> >> +	 * and device is RPM_ACTIVE. If pm_status==-EINVAL, runtime PM is
> >> +	 * disabled (e.g. CONFIG_PM not enabled), and we can/must assume device
> >> +	 * is active.
> >> +	 */
> >> +
> >>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);  
> >
> > [ ... ]
> >  
> >> @@ -1771,11 +1790,12 @@ static void mma8452_remove(struct i2c_client *client)
> >>
> >>  	iio_device_unregister(indio_dev);
> >>
> >> -	pm_runtime_disable(dev);
> >> -	pm_runtime_set_suspended(dev);
> >> -
> >>  	if (client->irq)
> >>  		free_irq(client->irq, indio_dev);
> >> +	/* No irq will fire beyond this point */
> >> +
> >> +	pm_runtime_disable(dev);
> >> +	pm_runtime_set_suspended(dev);  
> >
> > [Severity: Medium]
> > This correctly fixes the race in mma8452_remove() by moving free_irq()
> > before pm_runtime_disable(), but does the same race still exist in the
> > mma8452_probe() error path?
> >
> > If mma8452_probe() fails after pm_runtime_enable(), it jumps to the
> > runtime_suspend label:
> >
> >   mma8452_probe() {
> >       /* ... */
> >   runtime_suspend:
> >       pm_runtime_disable(dev);
> >       pm_runtime_set_suspended(dev);
> >
> >   free_irq:
> >       if (client->irq)
> >           free_irq(client->irq, indio_dev);
> >       /* ... */
> >   }
> >
> > If a shared interrupt fires between pm_runtime_disable() and free_irq(),
> > could the handler attempt to access unpowered hardware?  
> 
> I will apply the same reordering in mma8452_probe().

Sounds good to me

Jonathan

> 
> /Esben

Re: [PATCH v9 07/10] iio: accel: mma8452: Add comment block for struct mma8452_data

[Jonathan Cameron]

> The struct mma8452_data is central for this driver, and it makes sense to
> have a description of the fields in it to make it easier to work with the
> driver.
> 
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index b70bc8638128..223bef4f7baf 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -104,6 +104,21 @@
>  
>  #define MMA8452_AUTO_SUSPEND_DELAY_MS		2000
>  
> +/**
> + * struct mma8452_data - IIO device private data structure
> + * @client:			the I2C client object
> + * @lock:			mutex for synchronziation of register

From sashiko.
[Severity: Low]
This isn't a bug, but there's a typo here (synchronziation).
-
If nothing else shows up I'll fix it up whilst applying.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v9 07/10] iio: accel: mma8452: Add comment block for struct mma8452_data

[Esben Haabendal]

"Jonathan Cameron" <jic23@kernel.org> writes:

>> The struct mma8452_data is central for this driver, and it makes sense to
>> have a description of the fields in it to make it easier to work with the
>> driver.
>>
>> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index b70bc8638128..223bef4f7baf 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -104,6 +104,21 @@
>>
>>  #define MMA8452_AUTO_SUSPEND_DELAY_MS		2000
>>
>> +/**
>> + * struct mma8452_data - IIO device private data structure
>> + * @client:			the I2C client object
>> + * @lock:			mutex for synchronziation of register
>
> From sashiko.
> [Severity: Low]
> This isn't a bug, but there's a typo here (synchronziation).
> -
> If nothing else shows up I'll fix it up whilst applying.

I will fix it in v10.

/Esben

Re: [PATCH v9 10/10] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal]

"Jonathan Cameron" <jic23@kernel.org> writes:

> On Thu, 17 Sep 2026 08:40:06 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> <sashiko-bot@kernel.org> writes:
>>
>> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>> > - [Critical] iio: accel: mma8452: Interrupt storm livelock when IRQ fires during RPM_SUSPENDING
>> > - [High] iio: accel: mma8452: Invalid I2C access during system suspend due to mishandled -EINVAL
>> > - [Medium] iio: accel: mma8452: Race condition in probe error path due to pm_runtime_disable() before free_irq()
>> > --
>> >
> So far we haven't enabled sashiko emails to the linux-iio list
> (will probably move to that fairly soon) so fun side effect is this
> reply was shouting into the void - except that b4 picks it up.
>
> +CC linux-iio. I'm too lazy to add everyone by hand who was on original thread.
>
> Key here is looks like you already plan a v10.

Yes, sashiko-bot clearly catches a lot of valid problems. Also flags
some things that is not valid. And for a patch series with many
versions, like this one, these invalid findings keeps getting repeated.

Are there some guidelines how to handle this? Is it enough to write a
reply to the list(s) explaining why the finding is invalid on the first
report by sashiko-bot, or do we have to repeat every time that
sashiko-bot repeats the rebuted finding?

>> > commit bf19c63685f6fff116a3a5358196fd66dff6090a
>> > Author: Esben Haabendal <esben@geanix.com>
>> >
>> > iio: accel: mma8452: Support interrupt sharing
>> >
>> > This commit adds support for shared interrupts to the mma8452 driver. It
>> > modifies the interrupt handler to check the runtime PM status and ignores
>> > the interrupt if the device is suspended. It also updates the runtime
>> > suspend and resume paths to properly disable and restore interrupt sources.
>> >
>> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> >> index 5aea6bf3312af..2a980c0c2e03a 100644
>> >> --- a/drivers/iio/accel/mma8452.c
>> >> +++ b/drivers/iio/accel/mma8452.c
>> >
>> > [ ... ]
>> >
>> >> @@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>> >>  {
>> >>  	struct iio_dev *indio_dev = p;
>> >>  	struct mma8452_data *data = iio_priv(indio_dev);
>> >> +	struct device *dev = &data->client->dev;
>> >>  	irqreturn_t ret = IRQ_NONE;
>> >> +	int pm_status;
>> >>  	int src;
>> >>
>> >> +	pm_status = pm_runtime_get_if_active(dev);
>> >> +	if (pm_status == 0)
>> >> +		return IRQ_NONE; /* device is powered down */
>> >
>> > [Severity: Critical]
>> > If the device asserts its level-triggered interrupt when the PM core is
>> > transitioning the device to RPM_SUSPENDING (e.g., when the autosuspend
>> > timer expires), pm_runtime_get_if_active() will return 0.
>> >
>> > Does returning IRQ_NONE without clearing the hardware interrupt here cause
>> > a permanent interrupt storm? Since the IRQ line is shared and unmasked upon
>> > return, the still-asserted hardware interrupt might immediately re-trigger
>> > the handler, potentially starving the worker thread that executes
>> > mma8452_runtime_suspend() and hard-locking the system.
>>
>> This should be handled by the disable of all interrupt sources (clearing
>> CTRL_REG4) in mma8452_runtime_suspend().
>>
>> > [Severity: High]
>> > During system suspend, pm_runtime_force_suspend() disables runtime PM,
>> > which increments the disable depth. If a shared interrupt fires during this
>> > window, pm_runtime_get_if_active() returns -EINVAL.
>> >
>> > Since this check only catches pm_status == 0, does this allow the handler
>> > to proceed and perform I2C reads on unpowered hardware? Per subsystem
>> > guidelines, should this check for pm_status <= 0 instead?
>>
>> If we bail out on pm_status <= 0, the driver would not work on systems
>> with CONFIG_PM not enabled.
>>
>> In order to handle the difference betwen the different reasons for
>> -EINVAL, it would seem like we would need some #ifdef CONFIG_PM in here.
>> Do we really want to do that?
>
> That if_enabled() function is a pain.  Definitely not ifdef but
> how about
>
>  if (IS_ENABLED(CONFIG_PM) && ret < 0) or something like that?

That should work. Now, in this case, do we want to report IRQ_HANDLED or
IRQ_NONE? We obviosly did not really handle it, but we also don't know
if the irq was for this device.

>> >> +
>> >> +	/*
>> >> +	 * pm_status is now 1 or -EINVAL. If pm_status==1, runtime PM is enabled
>> >> +	 * and device is RPM_ACTIVE. If pm_status==-EINVAL, runtime PM is
>> >> +	 * disabled (e.g. CONFIG_PM not enabled), and we can/must assume device
>> >> +	 * is active.
>> >> +	 */
>> >> +
>> >>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>> >
>> > [ ... ]
>> >
>> >> @@ -1771,11 +1790,12 @@ static void mma8452_remove(struct i2c_client *client)
>> >>
>> >>  	iio_device_unregister(indio_dev);
>> >>
>> >> -	pm_runtime_disable(dev);
>> >> -	pm_runtime_set_suspended(dev);
>> >> -
>> >>  	if (client->irq)
>> >>  		free_irq(client->irq, indio_dev);
>> >> +	/* No irq will fire beyond this point */
>> >> +
>> >> +	pm_runtime_disable(dev);
>> >> +	pm_runtime_set_suspended(dev);
>> >
>> > [Severity: Medium]
>> > This correctly fixes the race in mma8452_remove() by moving free_irq()
>> > before pm_runtime_disable(), but does the same race still exist in the
>> > mma8452_probe() error path?
>> >
>> > If mma8452_probe() fails after pm_runtime_enable(), it jumps to the
>> > runtime_suspend label:
>> >
>> >   mma8452_probe() {
>> >       /* ... */
>> >   runtime_suspend:
>> >       pm_runtime_disable(dev);
>> >       pm_runtime_set_suspended(dev);
>> >
>> >   free_irq:
>> >       if (client->irq)
>> >           free_irq(client->irq, indio_dev);
>> >       /* ... */
>> >   }
>> >
>> > If a shared interrupt fires between pm_runtime_disable() and free_irq(),
>> > could the handler attempt to access unpowered hardware?
>>
>> I will apply the same reordering in mma8452_probe().
>
> Sounds good to me

I will send it when I have the above question resolved.

/Esben

Re: [PATCH v9 10/10] iio: accel: mma8452: Support interrupt sharing

[Jonathan Cameron]

On Mon, 21 Sep 2026 07:35:11 +0200
Esben Haabendal <esben@geanix.com> wrote:

> "Jonathan Cameron" <jic23@kernel.org> writes:
> 
> > On Thu, 17 Sep 2026 08:40:06 +0200
> > Esben Haabendal <esben@geanix.com> wrote:
> >  
> >> <sashiko-bot@kernel.org> writes:
> >>  
> >> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >> > - [Critical] iio: accel: mma8452: Interrupt storm livelock when IRQ fires during RPM_SUSPENDING
> >> > - [High] iio: accel: mma8452: Invalid I2C access during system suspend due to mishandled -EINVAL
> >> > - [Medium] iio: accel: mma8452: Race condition in probe error path due to pm_runtime_disable() before free_irq()
> >> > --
> >> >  
> > So far we haven't enabled sashiko emails to the linux-iio list
> > (will probably move to that fairly soon) so fun side effect is this
> > reply was shouting into the void - except that b4 picks it up.
> >
> > +CC linux-iio. I'm too lazy to add everyone by hand who was on original thread.
> >
> > Key here is looks like you already plan a v10.  
> 
> Yes, sashiko-bot clearly catches a lot of valid problems. Also flags
> some things that is not valid. And for a patch series with many
> versions, like this one, these invalid findings keeps getting repeated.
> 
> Are there some guidelines how to handle this? Is it enough to write a
> reply to the list(s) explaining why the finding is invalid on the first
> report by sashiko-bot, or do we have to repeat every time that
> sashiko-bot repeats the rebuted finding?

No need to repeat.  One reply as you say and then add a note either
under the --- cut mark in the commit description or in the
cover letter.


> 
> >> > commit bf19c63685f6fff116a3a5358196fd66dff6090a
> >> > Author: Esben Haabendal <esben@geanix.com>
> >> >
> >> > iio: accel: mma8452: Support interrupt sharing
> >> >
> >> > This commit adds support for shared interrupts to the mma8452 driver. It
> >> > modifies the interrupt handler to check the runtime PM status and ignores
> >> > the interrupt if the device is suspended. It also updates the runtime
> >> > suspend and resume paths to properly disable and restore interrupt sources.
> >> >  
> >> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> >> >> index 5aea6bf3312af..2a980c0c2e03a 100644
> >> >> --- a/drivers/iio/accel/mma8452.c
> >> >> +++ b/drivers/iio/accel/mma8452.c  
> >> >
> >> > [ ... ]
> >> >  
> >> >> @@ -1086,15 +1088,28 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
> >> >>  {
> >> >>  	struct iio_dev *indio_dev = p;
> >> >>  	struct mma8452_data *data = iio_priv(indio_dev);
> >> >> +	struct device *dev = &data->client->dev;
> >> >>  	irqreturn_t ret = IRQ_NONE;
> >> >> +	int pm_status;
> >> >>  	int src;
> >> >>
> >> >> +	pm_status = pm_runtime_get_if_active(dev);
> >> >> +	if (pm_status == 0)
> >> >> +		return IRQ_NONE; /* device is powered down */  
> >> >
> >> > [Severity: Critical]
> >> > If the device asserts its level-triggered interrupt when the PM core is
> >> > transitioning the device to RPM_SUSPENDING (e.g., when the autosuspend
> >> > timer expires), pm_runtime_get_if_active() will return 0.
> >> >
> >> > Does returning IRQ_NONE without clearing the hardware interrupt here cause
> >> > a permanent interrupt storm? Since the IRQ line is shared and unmasked upon
> >> > return, the still-asserted hardware interrupt might immediately re-trigger
> >> > the handler, potentially starving the worker thread that executes
> >> > mma8452_runtime_suspend() and hard-locking the system.  
> >>
> >> This should be handled by the disable of all interrupt sources (clearing
> >> CTRL_REG4) in mma8452_runtime_suspend().
> >>  
> >> > [Severity: High]
> >> > During system suspend, pm_runtime_force_suspend() disables runtime PM,
> >> > which increments the disable depth. If a shared interrupt fires during this
> >> > window, pm_runtime_get_if_active() returns -EINVAL.
> >> >
> >> > Since this check only catches pm_status == 0, does this allow the handler
> >> > to proceed and perform I2C reads on unpowered hardware? Per subsystem
> >> > guidelines, should this check for pm_status <= 0 instead?  
> >>
> >> If we bail out on pm_status <= 0, the driver would not work on systems
> >> with CONFIG_PM not enabled.
> >>
> >> In order to handle the difference betwen the different reasons for
> >> -EINVAL, it would seem like we would need some #ifdef CONFIG_PM in here.
> >> Do we really want to do that?  
> >
> > That if_enabled() function is a pain.  Definitely not ifdef but
> > how about
> >
> >  if (IS_ENABLED(CONFIG_PM) && ret < 0) or something like that?  
> 
> That should work. Now, in this case, do we want to report IRQ_HANDLED or
> IRQ_NONE? We obviosly did not really handle it, but we also don't know
> if the irq was for this device.

We never get consensus on this but I tend to go with IRQ_HANDLED when
we don't know it wasn't ours.

Jonathan

>