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

[PATCH] 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, so it always
unregisters the slot that cros_ec_sensors_core_register() registered.

Signed-off-by: Satyam Gupta <sgsatyam@google.com>
---
Tested on an Android desktop device (6.18, Intel) with two
accelerometers and a lid angle sensor: each sensor was unbound and
bound 20 times while a reader kept EC commands in flight, then FIFO
samples were streamed. Also tested on a ChromeOS device (6.6) with an
RGB light sensor, together with downstream changes to that driver.

 drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 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..4d019132cd78 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
@@ -216,11 +216,11 @@ 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 cros_ec_sensor_platform *sensor_platform =
+		dev_get_platdata(&pdev->dev);
 
-	cros_ec_sensorhub_unregister_push_data(sensor_hub, sensor_num);
+	cros_ec_sensorhub_unregister_push_data(sensor_hub,
+					       sensor_platform->sensor_num);
 }
 
 /**
-- 
2.56.0.398.g882cbad275-goog

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

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 07:31:07AM +0000, Satyam Gupta wrote:
> 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, so it always
> unregisters the slot that cros_ec_sensors_core_register() registered.

Sounds like a Fixes tag is good to have.

> Signed-off-by: Satyam Gupta <sgsatyam@google.com>
> ---
> Tested on an Android desktop device (6.18, Intel) with two
> accelerometers and a lid angle sensor: each sensor was unbound and
> bound 20 times while a reader kept EC commands in flight, then FIFO
> samples were streamed. Also tested on a ChromeOS device (6.6) with an
> RGB light sensor, together with downstream changes to that driver.
> 
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 8 ++++----
> @@ -216,11 +216,11 @@ 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 cros_ec_sensor_platform *sensor_platform =
> +		dev_get_platdata(&pdev->dev);

With context I see it would be better written in the following form:

	struct platform_device *pdev = (struct platform_device *)arg;
	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,
> +					       sensor_pdata->sensor_num);

	cros_ec_sensorhub_unregister_push_data(sensor_hub, pdata->sensor_num);

>  }

-- 
With Best Regards,
Andy Shevchenko

[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

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

[Joshua Crofts]

On Fri,  6 Nov 2026 07:19:04 +0000
Satyam Gupta <sgsatyam@google.com> wrote:

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

No, don't send a new version of a patch as a reply to the
previous version, it may break tooling like b4 for example.

-- 
Joshua Crofts

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

[Satyam Gupta]

On Fri, 6 Nov 2026 10:22:17 +0100, Joshua Crofts wrote:
> No, don't send a new version of a patch as a reply to the
> previous version, it may break tooling like b4 for example.

Got it, thanks for pointing that out. I will make sure future revisions
start a new thread instead of using --in-reply-to.

Thanks,
Satyam