[PATCH v2] iio: cros_ec: Use platform sensor_num to unregister FIFO callback

[PATCH v2] iio: cros_ec: Use platform sensor_num to unregister FIFO callback

[Satyam Gupta]

cros_ec_sensors_core_register() registers the FIFO callback for the
sensor number found in the platform data. The matching cleanup action,
cros_ec_sensors_core_clean(), reads the number back from
st->param.info.sensor_num instead.

st->param is the EC host command buffer. Drivers fill it in under
cmd_lock before each command, while the cleanup action reads it without
that lock. It is also set only for physical sensors. If the field holds
another value when the device is unbound, the cleanup clears the wrong
slot and leaves the callback in place, still pointing to the iio_dev
that is about to be freed.

No in-tree driver changes the field after probe, so mainline is not
known to hit this. The ChromeOS RGB light sensor support does: it
changes the field while a command is in flight, and unbinding the
device then leaves a stale callback. Found by an AI code review agent.

Use the platform data in the cleanup action too, so it always
unregisters the slot that cros_ec_sensors_core_register() registered.

Fixes: aa984f1ba4a4 ("iio: cros_ec: Register to cros_ec_sensorhub when EC supports FIFO")
Signed-off-by: Satyam Gupta <sgsatyam@google.com>
---
Changes in v2:
- Add a Fixes tag (Andy).
- Use a local struct device pointer and name the platform data pdata,
  so the unregister call fits on one line (Andy).
- Say what triggers the problem and how it was found.

v1: https://lore.kernel.org/r/20261105073107.172121-1-sgsatyam@google.com

The patch was drafted with an LLM-based coding assistant (Gemini); I
reviewed and tested it.

Tested on downstream kernels, together with fixes to the out-of-tree
RGB light sensor support: ChromeOS 6.6 (Intel) and Android desktop 6.12
(MediaTek) and 6.18 (Intel). Unbinding and binding the light sensor
while its RGB channel is read in a loop failed within 1 to 46 cycles
without the fixes (the stale callback slot makes the next probe fail
with -EINVAL) and passed 200 cycles with them on all three. On the 6.12
device the accelerometer and gyroscope were also unbound and bound
repeatedly and still streamed FIFO samples. Mainline itself was only
build-tested (W=1 and sparse, x86_64 and arm64).

 .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c  | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index d03e40c91582..632036eea4f3 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -214,13 +214,11 @@ EXPORT_SYMBOL_GPL(cros_ec_sensors_push_data);
 static void cros_ec_sensors_core_clean(void *arg)
 {
 	struct platform_device *pdev = (struct platform_device *)arg;
-	struct cros_ec_sensorhub *sensor_hub =
-		dev_get_drvdata(pdev->dev.parent);
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
-	u8 sensor_num = st->param.info.sensor_num;
+	struct device *dev = &pdev->dev;
+	struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
+	struct cros_ec_sensor_platform *pdata = dev_get_platdata(dev);
 
-	cros_ec_sensorhub_unregister_push_data(sensor_hub, sensor_num);
+	cros_ec_sensorhub_unregister_push_data(sensor_hub, pdata->sensor_num);
 }
 
 /**

base-commit: 1390ca4953a7400ca932cb49635f02c727f885a6
-- 
2.56.0.398.g882cbad275-goog

Re: [PATCH v2] iio: cros_ec: Use platform sensor_num to unregister FIFO callback

[Tzung-Bi Shih]

On Thu, Nov 05, 2026 at 01:20:57PM +0000, Satyam Gupta wrote:
> No in-tree driver changes the field after probe, so mainline is not
> known to hit this. The ChromeOS RGB light sensor support does: it
> changes the field while a command is in flight, and unbinding the
> device then leaves a stale callback. Found by an AI code review agent.

I think this paragraph can be moved after "---" so that it isn't part of
the commit message.

>  static void cros_ec_sensors_core_clean(void *arg)
>  {
>  	struct platform_device *pdev = (struct platform_device *)arg;
> -	struct cros_ec_sensorhub *sensor_hub =
> -		dev_get_drvdata(pdev->dev.parent);
> -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> -	u8 sensor_num = st->param.info.sensor_num;
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
> +	struct cros_ec_sensor_platform *pdata = dev_get_platdata(dev);
>  
> -	cros_ec_sensorhub_unregister_push_data(sensor_hub, sensor_num);
> +	cros_ec_sensorhub_unregister_push_data(sensor_hub, pdata->sensor_num);

So now `pdev` is only used to carry `dev`.  How about just passing `dev`
to this function in [1] so that it can be simplified?

[1] https://elixir.bootlin.com/linux/v7.3/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L394

[PATCH v3] iio: cros_ec: Use platform sensor_num to unregister FIFO callback

[Satyam Gupta]

cros_ec_sensors_core_register() registers the FIFO callback for the
sensor number found in the platform data. The matching cleanup action,
cros_ec_sensors_core_clean(), reads the number back from
st->param.info.sensor_num instead.

st->param is the EC host command buffer. Drivers fill it in under
cmd_lock before each command, while the cleanup action reads it without
that lock. It is also set only for physical sensors. If the field holds
another value when the device is unbound, the cleanup clears the wrong
slot and leaves the callback in place, still pointing to the iio_dev
that is about to be freed.

Use the platform data in the cleanup action too, and pass the struct
device pointer directly to devm_add_action_or_reset(), so it always
unregisters the slot that cros_ec_sensors_core_register() registered.

Fixes: aa984f1ba4a4 ("iio: cros_ec: Register to cros_ec_sensorhub when EC supports FIFO")
Signed-off-by: Satyam Gupta <sgsatyam@google.com>
---
Changes in v3:
- Pass dev instead of pdev to devm_add_action_or_reset() and drop the
  unused pdev local variable (Tzung-Bi).
- Move the note about out-of-tree triggers and how the issue was found
  below '---' (Tzung-Bi).

Changes in v2:
- Add a Fixes tag (Andy).
- Use a local struct device pointer and name the platform data pdata,
  so the unregister call fits on one line (Andy).
- Say what triggers the problem and how it was found.

v2: https://lore.kernel.org/r/20261105132057.2429385-1-sgsatyam@google.com
v1: https://lore.kernel.org/r/20261105073107.172121-1-sgsatyam@google.com

No in-tree driver changes st->param.info.sensor_num after probe, so
mainline is not known to hit this. The ChromeOS out-of-tree RGB light
sensor support does: it changes the field while a command is in flight,
and unbinding the device then leaves a stale callback. Found by an AI
code review agent.

The patch was drafted with an LLM-based coding assistant (Gemini); I
reviewed and tested it.

Tested on downstream kernels, together with fixes to the out-of-tree
RGB light sensor support: ChromeOS 6.6 (Intel) and 5.10 (MediaTek), and
Android desktop 6.12 (MediaTek) and 6.18 (Intel). Unbinding and binding
the light sensor while its RGB channel is read in a loop failed within
1 to 46 cycles without the fixes (the stale callback slot makes the
next probe fail with -EINVAL) and passed 200 cycles with them on all
four. On the 6.12 device the accelerometer and gyroscope were also
unbound and bound repeatedly and still streamed FIFO samples. Mainline
itself was only build-tested (W=1 and sparse, x86_64 and arm64).

 .../common/cros_ec_sensors/cros_ec_sensors_core.c  | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index d03e40c91582..67fb53071684 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -213,14 +213,11 @@ EXPORT_SYMBOL_GPL(cros_ec_sensors_push_data);
 
 static void cros_ec_sensors_core_clean(void *arg)
 {
-	struct platform_device *pdev = (struct platform_device *)arg;
-	struct cros_ec_sensorhub *sensor_hub =
-		dev_get_drvdata(pdev->dev.parent);
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
-	u8 sensor_num = st->param.info.sensor_num;
+	struct device *dev = arg;
+	struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
+	struct cros_ec_sensor_platform *pdata = dev_get_platdata(dev);
 
-	cros_ec_sensorhub_unregister_push_data(sensor_hub, sensor_num);
+	cros_ec_sensorhub_unregister_push_data(sensor_hub, pdata->sensor_num);
 }
 
 /**
@@ -372,7 +369,6 @@ int cros_ec_sensors_core_register(struct device *dev,
 {
 	struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
 	struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
-	struct platform_device *pdev = to_platform_device(dev);
 	struct cros_ec_dev *ec = sensor_hub->ec;
 	int ret;
 
@@ -391,7 +387,7 @@ int cros_ec_sensors_core_register(struct device *dev,
 		return ret;
 
 	return devm_add_action_or_reset(
-			dev, cros_ec_sensors_core_clean, pdev);
+			dev, cros_ec_sensors_core_clean, dev);
 }
 EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
 

base-commit: 1390ca4953a7400ca932cb49635f02c727f885a6
-- 
2.56.0.455.g6c5a503b38-goog