* [PATCH 1/5] platform/chrome: Centralize cros_ec_device allocation
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
@ 2025-08-28 8:35 ` Tzung-Bi Shih
2025-08-28 8:35 ` [PATCH 2/5] platform/chrome: Centralize common cros_ec_device initialization Tzung-Bi Shih
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-08-28 8:35 UTC (permalink / raw)
To: Dmitry Torokhov, Benson Leung; +Cc: tzungbi, linux-input, chrome-platform
Introduce a helper function, cros_ec_device_alloc(), to centralize the
allocation of the struct cros_ec_device. Convert all protocol device
drivers to use this new function.
This is a preparatory step for separating common initialization logic
out of device drivers' probe() and cros_ec_register().
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec.c | 12 ++++++++++++
drivers/platform/chrome/cros_ec.h | 3 +++
drivers/platform/chrome/cros_ec_i2c.c | 4 ++--
drivers/platform/chrome/cros_ec_ishtp.c | 2 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/chrome/cros_ec_rpmsg.c | 2 +-
drivers/platform/chrome/cros_ec_spi.c | 2 +-
drivers/platform/chrome/cros_ec_uart.c | 2 +-
8 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index b1730362e399..25283a148ab9 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -30,6 +30,18 @@ static struct cros_ec_platform pd_p = {
.cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX),
};
+struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
+{
+ struct cros_ec_device *ec_dev;
+
+ ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ if (!ec_dev)
+ return NULL;
+
+ return ec_dev;
+}
+EXPORT_SYMBOL(cros_ec_device_alloc);
+
/**
* cros_ec_irq_handler() - top half part of the interrupt handler
* @irq: IRQ id
diff --git a/drivers/platform/chrome/cros_ec.h b/drivers/platform/chrome/cros_ec.h
index 6b95f1e0bace..cd4643bc5367 100644
--- a/drivers/platform/chrome/cros_ec.h
+++ b/drivers/platform/chrome/cros_ec.h
@@ -11,6 +11,9 @@
#include <linux/interrupt.h>
struct cros_ec_device;
+struct device;
+
+struct cros_ec_device *cros_ec_device_alloc(struct device *dev);
int cros_ec_register(struct cros_ec_device *ec_dev);
void cros_ec_unregister(struct cros_ec_device *ec_dev);
diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index 38af97cdaab2..ee3c5130ec3f 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -289,10 +289,10 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
static int cros_ec_i2c_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct cros_ec_device *ec_dev = NULL;
+ struct cros_ec_device *ec_dev;
int err;
- ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ ec_dev = cros_ec_device_alloc(dev);
if (!ec_dev)
return -ENOMEM;
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index 7e7190b30cbb..c102a796670c 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -543,7 +543,7 @@ static int cros_ec_dev_init(struct ishtp_cl_data *client_data)
struct cros_ec_device *ec_dev;
struct device *dev = cl_data_to_dev(client_data);
- ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ ec_dev = cros_ec_device_alloc(dev);
if (!ec_dev)
return -ENOMEM;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 7d9a78289c96..30fa89b81666 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -637,7 +637,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
}
}
- ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ ec_dev = cros_ec_device_alloc(dev);
if (!ec_dev)
return -ENOMEM;
diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
index bc2666491db1..9ac2b923db6d 100644
--- a/drivers/platform/chrome/cros_ec_rpmsg.c
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -216,7 +216,7 @@ static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
struct cros_ec_device *ec_dev;
int ret;
- ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ ec_dev = cros_ec_device_alloc(dev);
if (!ec_dev)
return -ENOMEM;
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8ca0f854e7ac..c778300a4145 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -749,7 +749,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
if (ec_spi == NULL)
return -ENOMEM;
ec_spi->spi = spi;
- ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ ec_dev = cros_ec_device_alloc(dev);
if (!ec_dev)
return -ENOMEM;
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 19c179d49c90..1a7511b1bbe3 100644
--- a/drivers/platform/chrome/cros_ec_uart.c
+++ b/drivers/platform/chrome/cros_ec_uart.c
@@ -259,7 +259,7 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
if (!ec_uart)
return -ENOMEM;
- ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ ec_dev = cros_ec_device_alloc(dev);
if (!ec_dev)
return -ENOMEM;
--
2.51.0.268.g9569e192d0-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 2/5] platform/chrome: Centralize common cros_ec_device initialization
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
2025-08-28 8:35 ` [PATCH 1/5] platform/chrome: Centralize cros_ec_device allocation Tzung-Bi Shih
@ 2025-08-28 8:35 ` Tzung-Bi Shih
2025-08-28 8:35 ` [PATCH 3/5] platform/chrome: cros_ec: Separate initialization from cros_ec_register() Tzung-Bi Shih
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-08-28 8:35 UTC (permalink / raw)
To: Dmitry Torokhov, Benson Leung; +Cc: tzungbi, linux-input, chrome-platform
Move the common initialization from protocol device drivers into central
cros_ec_device_alloc().
This removes duplicated code from each driver's probe function.
The buffer sizes are now calculated once, using the maximum possible
overhead required by any of the transport protocols, ensuring the
allocated buffers are sufficient for all cases.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec.c | 9 +++++++++
drivers/platform/chrome/cros_ec_i2c.c | 5 -----
drivers/platform/chrome/cros_ec_ishtp.c | 4 ----
drivers/platform/chrome/cros_ec_lpc.c | 4 ----
drivers/platform/chrome/cros_ec_rpmsg.c | 4 ----
drivers/platform/chrome/cros_ec_spi.c | 5 -----
drivers/platform/chrome/cros_ec_uart.c | 4 ----
include/linux/platform_data/cros_ec_proto.h | 14 ++++++++++----
8 files changed, 19 insertions(+), 30 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 25283a148ab9..da049068b6e9 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -38,6 +38,15 @@ struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
if (!ec_dev)
return NULL;
+ ec_dev->din_size = sizeof(struct ec_host_response) +
+ sizeof(struct ec_response_get_protocol_info) +
+ EC_MAX_RESPONSE_OVERHEAD;
+ ec_dev->dout_size = sizeof(struct ec_host_request) +
+ sizeof(struct ec_params_rwsig_action) +
+ EC_MAX_REQUEST_OVERHEAD;
+
+ ec_dev->dev = dev;
+
return ec_dev;
}
EXPORT_SYMBOL(cros_ec_device_alloc);
diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index ee3c5130ec3f..def1144a077e 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -297,16 +297,11 @@ static int cros_ec_i2c_probe(struct i2c_client *client)
return -ENOMEM;
i2c_set_clientdata(client, ec_dev);
- ec_dev->dev = dev;
ec_dev->priv = client;
ec_dev->irq = client->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_i2c;
ec_dev->phys_name = client->adapter->name;
- ec_dev->din_size = sizeof(struct ec_host_response_i2c) +
- sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct ec_host_request_i2c) +
- sizeof(struct ec_params_rwsig_action);
err = cros_ec_register(ec_dev);
if (err) {
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index c102a796670c..4e74e702c5a2 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -550,14 +550,10 @@ static int cros_ec_dev_init(struct ishtp_cl_data *client_data)
client_data->ec_dev = ec_dev;
dev->driver_data = ec_dev;
- ec_dev->dev = dev;
ec_dev->priv = client_data->cros_ish_cl;
ec_dev->cmd_xfer = NULL;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_ish;
ec_dev->phys_name = dev_name(dev);
- ec_dev->din_size = sizeof(struct cros_ish_in_msg) +
- sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct cros_ish_out_msg) + sizeof(struct ec_params_rwsig_action);
return cros_ec_register(ec_dev);
}
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 30fa89b81666..78cfff80cdea 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -642,14 +642,10 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
return -ENOMEM;
platform_set_drvdata(pdev, ec_dev);
- ec_dev->dev = dev;
ec_dev->phys_name = dev_name(dev);
ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_lpc;
ec_dev->cmd_readmem = cros_ec_lpc_readmem;
- ec_dev->din_size = sizeof(struct ec_host_response) +
- sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct ec_host_request) + sizeof(struct ec_params_rwsig_action);
ec_dev->priv = ec_lpc;
/*
diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
index 9ac2b923db6d..09bd9e49464e 100644
--- a/drivers/platform/chrome/cros_ec_rpmsg.c
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -224,14 +224,10 @@ static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
if (!ec_rpmsg)
return -ENOMEM;
- ec_dev->dev = dev;
ec_dev->priv = ec_rpmsg;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_rpmsg;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_rpmsg;
ec_dev->phys_name = dev_name(&rpdev->dev);
- ec_dev->din_size = sizeof(struct ec_host_response) +
- sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct ec_host_request) + sizeof(struct ec_params_rwsig_action);
dev_set_drvdata(dev, ec_dev);
ec_rpmsg->rpdev = rpdev;
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index c778300a4145..28fa82f8cb07 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -757,16 +757,11 @@ static int cros_ec_spi_probe(struct spi_device *spi)
cros_ec_spi_dt_probe(ec_spi, dev);
spi_set_drvdata(spi, ec_dev);
- ec_dev->dev = dev;
ec_dev->priv = ec_spi;
ec_dev->irq = spi->irq;
ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
ec_dev->pkt_xfer = cros_ec_pkt_xfer_spi;
ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
- ec_dev->din_size = EC_MSG_PREAMBLE_COUNT +
- sizeof(struct ec_host_response) +
- sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct ec_host_request) + sizeof(struct ec_params_rwsig_action);
ec_spi->last_transfer_ns = ktime_get_ns();
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 1a7511b1bbe3..d5b37414ff12 100644
--- a/drivers/platform/chrome/cros_ec_uart.c
+++ b/drivers/platform/chrome/cros_ec_uart.c
@@ -276,14 +276,10 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
/* Initialize ec_dev for cros_ec */
ec_dev->phys_name = dev_name(dev);
- ec_dev->dev = dev;
ec_dev->priv = ec_uart;
ec_dev->irq = ec_uart->irq;
ec_dev->cmd_xfer = NULL;
ec_dev->pkt_xfer = cros_ec_uart_pkt_xfer;
- ec_dev->din_size = sizeof(struct ec_host_response) +
- sizeof(struct ec_response_get_protocol_info);
- ec_dev->dout_size = sizeof(struct ec_host_request) + sizeof(struct ec_params_rwsig_action);
serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 3ec24f445c29..4d96cffbb9e3 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -33,12 +33,18 @@
/*
* Max bus-specific overhead incurred by request/responses.
- * I2C requires 1 additional byte for requests.
- * I2C requires 2 additional bytes for responses.
- * SPI requires up to 32 additional bytes for responses.
+ *
+ * Request:
+ * - I2C requires 1 byte (see struct ec_host_request_i2c).
+ * - ISHTP requires 4 bytes (see struct cros_ish_out_msg).
+ *
+ * Response:
+ * - I2C requires 2 bytes (see struct ec_host_response_i2c).
+ * - ISHTP requires 4 bytes (see struct cros_ish_in_msg).
+ * - SPI requires 32 bytes (see EC_MSG_PREAMBLE_COUNT).
*/
#define EC_PROTO_VERSION_UNKNOWN 0
-#define EC_MAX_REQUEST_OVERHEAD 1
+#define EC_MAX_REQUEST_OVERHEAD 4
#define EC_MAX_RESPONSE_OVERHEAD 32
/*
--
2.51.0.268.g9569e192d0-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/5] platform/chrome: cros_ec: Separate initialization from cros_ec_register()
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
2025-08-28 8:35 ` [PATCH 1/5] platform/chrome: Centralize cros_ec_device allocation Tzung-Bi Shih
2025-08-28 8:35 ` [PATCH 2/5] platform/chrome: Centralize common cros_ec_device initialization Tzung-Bi Shih
@ 2025-08-28 8:35 ` Tzung-Bi Shih
2025-08-28 8:36 ` [PATCH 4/5] platform/chrome: cros_ec: Add a flag to track registration state Tzung-Bi Shih
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-08-28 8:35 UTC (permalink / raw)
To: Dmitry Torokhov, Benson Leung; +Cc: tzungbi, linux-input, chrome-platform
Move the initialization of the `struct cros_ec_device` from
cros_ec_register() into cros_ec_device_alloc().
This decouples device initialization from registration. By doing so,
the per-device lock is now available immediately after allocation,
allowing it to be used safely even before the device is fully
registered.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec.c | 57 ++++++++++++++++---------------
1 file changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index da049068b6e9..61bcef8741db 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -30,6 +30,14 @@ static struct cros_ec_platform pd_p = {
.cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX),
};
+static void cros_ec_device_free(void *data)
+{
+ struct cros_ec_device *ec_dev = data;
+
+ mutex_destroy(&ec_dev->lock);
+ lockdep_unregister_key(&ec_dev->lockdep_key);
+}
+
struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
{
struct cros_ec_device *ec_dev;
@@ -45,7 +53,28 @@ struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
sizeof(struct ec_params_rwsig_action) +
EC_MAX_REQUEST_OVERHEAD;
+ ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
+ if (!ec_dev->din)
+ return NULL;
+
+ ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
+ if (!ec_dev->dout)
+ return NULL;
+
ec_dev->dev = dev;
+ ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
+ ec_dev->max_request = sizeof(struct ec_params_rwsig_action);
+ ec_dev->suspend_timeout_ms = EC_HOST_SLEEP_TIMEOUT_DEFAULT;
+
+ BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
+ BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->panic_notifier);
+
+ lockdep_register_key(&ec_dev->lockdep_key);
+ mutex_init(&ec_dev->lock);
+ lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
+
+ if (devm_add_action_or_reset(dev, cros_ec_device_free, ec_dev))
+ return NULL;
return ec_dev;
}
@@ -200,29 +229,7 @@ static int cros_ec_ready_event(struct notifier_block *nb,
int cros_ec_register(struct cros_ec_device *ec_dev)
{
struct device *dev = ec_dev->dev;
- int err = 0;
-
- BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
- BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->panic_notifier);
-
- ec_dev->max_request = sizeof(struct ec_params_hello);
- ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
- ec_dev->max_passthru = 0;
- ec_dev->ec = NULL;
- ec_dev->pd = NULL;
- ec_dev->suspend_timeout_ms = EC_HOST_SLEEP_TIMEOUT_DEFAULT;
-
- ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
- if (!ec_dev->din)
- return -ENOMEM;
-
- ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
- if (!ec_dev->dout)
- return -ENOMEM;
-
- lockdep_register_key(&ec_dev->lockdep_key);
- mutex_init(&ec_dev->lock);
- lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
+ int err;
/* Send RWSIG continue to jump to RW for devices using RWSIG. */
err = cros_ec_rwsig_continue(ec_dev);
@@ -322,8 +329,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
exit:
platform_device_unregister(ec_dev->ec);
platform_device_unregister(ec_dev->pd);
- mutex_destroy(&ec_dev->lock);
- lockdep_unregister_key(&ec_dev->lockdep_key);
return err;
}
EXPORT_SYMBOL(cros_ec_register);
@@ -343,8 +348,6 @@ void cros_ec_unregister(struct cros_ec_device *ec_dev)
&ec_dev->notifier_ready);
platform_device_unregister(ec_dev->pd);
platform_device_unregister(ec_dev->ec);
- mutex_destroy(&ec_dev->lock);
- lockdep_unregister_key(&ec_dev->lockdep_key);
}
EXPORT_SYMBOL(cros_ec_unregister);
--
2.51.0.268.g9569e192d0-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 4/5] platform/chrome: cros_ec: Add a flag to track registration state
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
` (2 preceding siblings ...)
2025-08-28 8:35 ` [PATCH 3/5] platform/chrome: cros_ec: Separate initialization from cros_ec_register() Tzung-Bi Shih
@ 2025-08-28 8:36 ` Tzung-Bi Shih
2025-08-28 8:36 ` [PATCH 5/5] Input: cros_ec_keyb - Defer probe until parent EC device is registered Tzung-Bi Shih
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-08-28 8:36 UTC (permalink / raw)
To: Dmitry Torokhov, Benson Leung; +Cc: tzungbi, linux-input, chrome-platform
Introduce a `registered` flag to the `struct cros_ec_device` to allow
callers to determine if the device has been fully registered and is
ready for use.
This is a preparatory step to prevent race conditions where other drivers
might try to access the device before it is fully registered or after
it has been unregistered.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec.c | 7 +++++++
drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
include/linux/platform_data/cros_ec_proto.h | 4 ++++
3 files changed, 26 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 61bcef8741db..1da79e3d215b 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -9,6 +9,7 @@
* battery charging and regulator control, firmware update.
*/
+#include <linux/cleanup.h>
#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/of_platform.h>
@@ -316,6 +317,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
goto exit;
}
+ scoped_guard(mutex, &ec_dev->lock)
+ ec_dev->registered = true;
+
dev_info(dev, "Chrome EC device registered\n");
/*
@@ -343,6 +347,9 @@ EXPORT_SYMBOL(cros_ec_register);
*/
void cros_ec_unregister(struct cros_ec_device *ec_dev)
{
+ scoped_guard(mutex, &ec_dev->lock)
+ ec_dev->registered = false;
+
if (ec_dev->mkbp_event_supported)
blocking_notifier_chain_unregister(&ec_dev->event_notifier,
&ec_dev->notifier_ready);
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3e94a0a82173..1d8d9168ec1a 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -3,6 +3,7 @@
//
// Copyright (C) 2015 Google, Inc
+#include <linux/cleanup.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/limits.h>
@@ -1153,5 +1154,19 @@ int cros_ec_get_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd)
}
EXPORT_SYMBOL_GPL(cros_ec_get_cmd_versions);
+/**
+ * cros_ec_device_registered - Return if the ec_dev is registered.
+ *
+ * @ec_dev: EC device
+ *
+ * Return: true if registered. Otherwise, false.
+ */
+bool cros_ec_device_registered(struct cros_ec_device *ec_dev)
+{
+ guard(mutex)(&ec_dev->lock);
+ return ec_dev->registered;
+}
+EXPORT_SYMBOL_GPL(cros_ec_device_registered);
+
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("ChromeOS EC communication protocol helpers");
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 4d96cffbb9e3..de14923720a5 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -128,6 +128,7 @@ struct cros_ec_command {
* @dout_size: Size of dout buffer to allocate (zero to use static dout).
* @wake_enabled: True if this device can wake the system from sleep.
* @suspended: True if this device had been suspended.
+ * @registered: True if this device had been registered.
* @cmd_xfer: Send command to EC and get response.
* Returns the number of bytes received if the communication
* succeeded, but that doesn't mean the EC was happy with the
@@ -186,6 +187,7 @@ struct cros_ec_device {
int dout_size;
bool wake_enabled;
bool suspended;
+ bool registered;
int (*cmd_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
int (*pkt_xfer)(struct cros_ec_device *ec,
@@ -278,6 +280,8 @@ int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void
int cros_ec_get_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd);
+bool cros_ec_device_registered(struct cros_ec_device *ec_dev);
+
/**
* cros_ec_get_time_ns() - Return time in ns.
*
--
2.51.0.268.g9569e192d0-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 5/5] Input: cros_ec_keyb - Defer probe until parent EC device is registered
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
` (3 preceding siblings ...)
2025-08-28 8:36 ` [PATCH 4/5] platform/chrome: cros_ec: Add a flag to track registration state Tzung-Bi Shih
@ 2025-08-28 8:36 ` Tzung-Bi Shih
2025-09-14 1:06 ` Dmitry Torokhov
2025-08-29 11:28 ` [PATCH 0/5] platform/chrome: Fix a race when probing drivers Dmitry Torokhov
2025-09-14 3:47 ` Tzung-Bi Shih
6 siblings, 1 reply; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-08-28 8:36 UTC (permalink / raw)
To: Dmitry Torokhov, Benson Leung; +Cc: tzungbi, linux-input, chrome-platform
The `cros_ec_keyb` driver can be probed before the cros_ec_device has
completed the registration. This creates a race condition where
`cros_ec_keyb` might access uninitialized data.
Fix this by calling `cros_ec_device_registered()` to check the parent's
status. If the device is not yet ready, return -EPROBE_DEFER to ensure
the probe is retried later.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/input/keyboard/cros_ec_keyb.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index c1e53d87c8a7..f7209c8ebbcc 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -705,6 +705,12 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
ec = dev_get_drvdata(pdev->dev.parent);
if (!ec)
return -EPROBE_DEFER;
+ /*
+ * Even if the cros_ec_device pointer is available, still need to check
+ * if the device is fully registered before using it.
+ */
+ if (!cros_ec_device_registered(ec))
+ return -EPROBE_DEFER;
ckdev = devm_kzalloc(dev, sizeof(*ckdev), GFP_KERNEL);
if (!ckdev)
--
2.51.0.268.g9569e192d0-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
` (4 preceding siblings ...)
2025-08-28 8:36 ` [PATCH 5/5] Input: cros_ec_keyb - Defer probe until parent EC device is registered Tzung-Bi Shih
@ 2025-08-29 11:28 ` Dmitry Torokhov
2025-08-29 12:50 ` Tzung-Bi Shih
2025-09-14 3:47 ` Tzung-Bi Shih
6 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2025-08-29 11:28 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, linux-input, chrome-platform
Hi Tzung-Bi,
On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> probed at the same time.
>
> Example:
>
> + -----------------------------------------------------------------+
> | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> + -----------------------------------------------------------------+
> | Probe cros-ec-keyb. | |
> | - Decide to defer[1]. | |
> | | A device bound to a driver[2]. |
> | Probe cros_ec_lpc. | |
> | - Init the struct[3]. | |
> | | Retry cros-ec-keyb from the |
> | | deferred list[4]. |
> | | - Won't defer again as [3]. |
> | | - Access uninitialized data in |
> | | the struct. |
> | - Register the device. | |
> + -----------------------------------------------------------------+
>
> [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
>
> Note that the device link[5] can't help as in the observed environment,
> the devices are already added via device_add()[6].
>
> [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
>
> The series fixes the issue by ensuring the struct is ready for accessing
> before continuing to probe cros-ec-keyb.
Why is the keyboard platform device instantiated before the transport
(cros_ec_lpc) is done initializing? I think this is the root of the
issue...
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-08-29 11:28 ` [PATCH 0/5] platform/chrome: Fix a race when probing drivers Dmitry Torokhov
@ 2025-08-29 12:50 ` Tzung-Bi Shih
2025-09-02 13:18 ` Tzung-Bi Shih
0 siblings, 1 reply; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-08-29 12:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Benson Leung, linux-input, chrome-platform
On Fri, Aug 29, 2025 at 11:28:55AM +0000, Dmitry Torokhov wrote:
> On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> > A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> > modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> > probed at the same time.
> >
> > Example:
> >
> > + -----------------------------------------------------------------+
> > | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> > + -----------------------------------------------------------------+
> > | Probe cros-ec-keyb. | |
> > | - Decide to defer[1]. | |
> > | | A device bound to a driver[2]. |
> > | Probe cros_ec_lpc. | |
> > | - Init the struct[3]. | |
> > | | Retry cros-ec-keyb from the |
> > | | deferred list[4]. |
> > | | - Won't defer again as [3]. |
> > | | - Access uninitialized data in |
> > | | the struct. |
> > | - Register the device. | |
> > + -----------------------------------------------------------------+
> >
> > [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> > [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> > [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> > [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
> >
> > Note that the device link[5] can't help as in the observed environment,
> > the devices are already added via device_add()[6].
> >
> > [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> > [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
> >
> > The series fixes the issue by ensuring the struct is ready for accessing
> > before continuing to probe cros-ec-keyb.
>
> Why is the keyboard platform device instantiated before the transport
> (cros_ec_lpc) is done initializing? I think this is the root of the
> issue...
I may misunderstand but it seems to me:
- The ACPI bus enumerated and instantiated the platform devices[6] first.
- The keyboard platform device was probed when `cros_ec_keyb_driver`
registered. It deferred as its parent's drvdata was NULL[1].
- The transport platform device was probed when `cros_ec_lpc_driver`
registered. It set the drvdata[3].
- The keyboard platform device was probed again from retrying the deferred
list, by another thread `deferred_probe_work_func`. The parent's drvdata
wasn't NULL and cros_ec_register() for the transport device weren't
finished. The race happened.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-08-29 12:50 ` Tzung-Bi Shih
@ 2025-09-02 13:18 ` Tzung-Bi Shih
2025-09-04 14:06 ` Dmitry Torokhov
0 siblings, 1 reply; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-09-02 13:18 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Benson Leung, linux-input, chrome-platform
On Fri, Aug 29, 2025 at 08:50:01PM +0800, Tzung-Bi Shih wrote:
> On Fri, Aug 29, 2025 at 11:28:55AM +0000, Dmitry Torokhov wrote:
> > On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> > > A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> > > modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> > > probed at the same time.
> > >
> > > Example:
> > >
> > > + -----------------------------------------------------------------+
> > > | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> > > + -----------------------------------------------------------------+
> > > | Probe cros-ec-keyb. | |
> > > | - Decide to defer[1]. | |
> > > | | A device bound to a driver[2]. |
> > > | Probe cros_ec_lpc. | |
> > > | - Init the struct[3]. | |
> > > | | Retry cros-ec-keyb from the |
> > > | | deferred list[4]. |
> > > | | - Won't defer again as [3]. |
> > > | | - Access uninitialized data in |
> > > | | the struct. |
> > > | - Register the device. | |
> > > + -----------------------------------------------------------------+
> > >
> > > [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> > > [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> > > [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> > > [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
> > >
> > > Note that the device link[5] can't help as in the observed environment,
> > > the devices are already added via device_add()[6].
> > >
> > > [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> > > [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
> > >
> > > The series fixes the issue by ensuring the struct is ready for accessing
> > > before continuing to probe cros-ec-keyb.
> >
> > Why is the keyboard platform device instantiated before the transport
> > (cros_ec_lpc) is done initializing? I think this is the root of the
> > issue...
>
> I may misunderstand but it seems to me:
>
> - The ACPI bus enumerated and instantiated the platform devices[6] first.
>
> - The keyboard platform device was probed when `cros_ec_keyb_driver`
> registered. It deferred as its parent's drvdata was NULL[1].
>
> - The transport platform device was probed when `cros_ec_lpc_driver`
> registered. It set the drvdata[3].
>
> - The keyboard platform device was probed again from retrying the deferred
> list, by another thread `deferred_probe_work_func`. The parent's drvdata
> wasn't NULL and cros_ec_register() for the transport device weren't
> finished. The race happened.
Hi Dmitry,
Does it make sense to you?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-09-02 13:18 ` Tzung-Bi Shih
@ 2025-09-04 14:06 ` Dmitry Torokhov
2025-09-05 8:38 ` Tzung-Bi Shih
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2025-09-04 14:06 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, linux-input, chrome-platform
Hi Tzung-Bi,
On Tue, Sep 02, 2025 at 09:18:47PM +0800, Tzung-Bi Shih wrote:
> On Fri, Aug 29, 2025 at 08:50:01PM +0800, Tzung-Bi Shih wrote:
> > On Fri, Aug 29, 2025 at 11:28:55AM +0000, Dmitry Torokhov wrote:
> > > On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> > > > A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> > > > modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> > > > probed at the same time.
> > > >
> > > > Example:
> > > >
> > > > + -----------------------------------------------------------------+
> > > > | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> > > > + -----------------------------------------------------------------+
> > > > | Probe cros-ec-keyb. | |
> > > > | - Decide to defer[1]. | |
> > > > | | A device bound to a driver[2]. |
> > > > | Probe cros_ec_lpc. | |
> > > > | - Init the struct[3]. | |
> > > > | | Retry cros-ec-keyb from the |
> > > > | | deferred list[4]. |
> > > > | | - Won't defer again as [3]. |
> > > > | | - Access uninitialized data in |
> > > > | | the struct. |
> > > > | - Register the device. | |
> > > > + -----------------------------------------------------------------+
> > > >
> > > > [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> > > > [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> > > > [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> > > > [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
> > > >
> > > > Note that the device link[5] can't help as in the observed environment,
> > > > the devices are already added via device_add()[6].
> > > >
> > > > [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> > > > [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
> > > >
> > > > The series fixes the issue by ensuring the struct is ready for accessing
> > > > before continuing to probe cros-ec-keyb.
> > >
> > > Why is the keyboard platform device instantiated before the transport
> > > (cros_ec_lpc) is done initializing? I think this is the root of the
> > > issue...
> >
> > I may misunderstand but it seems to me:
> >
> > - The ACPI bus enumerated and instantiated the platform devices[6] first.
> >
> > - The keyboard platform device was probed when `cros_ec_keyb_driver`
> > registered. It deferred as its parent's drvdata was NULL[1].
> >
> > - The transport platform device was probed when `cros_ec_lpc_driver`
> > registered. It set the drvdata[3].
> >
> > - The keyboard platform device was probed again from retrying the deferred
> > list, by another thread `deferred_probe_work_func`. The parent's drvdata
> > wasn't NULL and cros_ec_register() for the transport device weren't
> > finished. The race happened.
>
> Hi Dmitry,
>
> Does it make sense to you?
I'll have to research how MFD mixes up statically described and
DT-described platform devices and makes sure that children are not
probed before the parent is ready - I think we need to make cros_ec
behave the same way.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-09-04 14:06 ` Dmitry Torokhov
@ 2025-09-05 8:38 ` Tzung-Bi Shih
2025-09-14 1:08 ` Dmitry Torokhov
0 siblings, 1 reply; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-09-05 8:38 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Benson Leung, linux-input, chrome-platform
On Thu, Sep 04, 2025 at 07:06:23AM -0700, Dmitry Torokhov wrote:
> On Tue, Sep 02, 2025 at 09:18:47PM +0800, Tzung-Bi Shih wrote:
> > On Fri, Aug 29, 2025 at 08:50:01PM +0800, Tzung-Bi Shih wrote:
> > > On Fri, Aug 29, 2025 at 11:28:55AM +0000, Dmitry Torokhov wrote:
> > > > On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> > > > > A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> > > > > modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> > > > > probed at the same time.
> > > > >
> > > > > Example:
> > > > >
> > > > > + -----------------------------------------------------------------+
> > > > > | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> > > > > + -----------------------------------------------------------------+
> > > > > | Probe cros-ec-keyb. | |
> > > > > | - Decide to defer[1]. | |
> > > > > | | A device bound to a driver[2]. |
> > > > > | Probe cros_ec_lpc. | |
> > > > > | - Init the struct[3]. | |
> > > > > | | Retry cros-ec-keyb from the |
> > > > > | | deferred list[4]. |
> > > > > | | - Won't defer again as [3]. |
> > > > > | | - Access uninitialized data in |
> > > > > | | the struct. |
> > > > > | - Register the device. | |
> > > > > + -----------------------------------------------------------------+
> > > > >
> > > > > [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> > > > > [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> > > > > [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> > > > > [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
> > > > >
> > > > > Note that the device link[5] can't help as in the observed environment,
> > > > > the devices are already added via device_add()[6].
> > > > >
> > > > > [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> > > > > [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
> > > > >
> > > > > The series fixes the issue by ensuring the struct is ready for accessing
> > > > > before continuing to probe cros-ec-keyb.
> > > >
> > > > Why is the keyboard platform device instantiated before the transport
> > > > (cros_ec_lpc) is done initializing? I think this is the root of the
> > > > issue...
> > >
> > > I may misunderstand but it seems to me:
> > >
> > > - The ACPI bus enumerated and instantiated the platform devices[6] first.
> > >
> > > - The keyboard platform device was probed when `cros_ec_keyb_driver`
> > > registered. It deferred as its parent's drvdata was NULL[1].
> > >
> > > - The transport platform device was probed when `cros_ec_lpc_driver`
> > > registered. It set the drvdata[3].
> > >
> > > - The keyboard platform device was probed again from retrying the deferred
> > > list, by another thread `deferred_probe_work_func`. The parent's drvdata
> > > wasn't NULL and cros_ec_register() for the transport device weren't
> > > finished. The race happened.
> >
> > Hi Dmitry,
> >
> > Does it make sense to you?
>
> I'll have to research how MFD mixes up statically described and
> DT-described platform devices and makes sure that children are not
> probed before the parent is ready - I think we need to make cros_ec
> behave the same way.
I may misunderstand but FWIW:
I failed to find relevant code in MFD [7] that guarantees the probe order.
Also, I'm curious about wouldn't code at [7] results in duplicate platform
devices? E.g., 1 populated from OF; 1 created by MFD.
Note: current cros_ec_dev.c doesn't use `of_compatible` in struct mfd_cell.
If we're looking at how cros_ec_dev.c guarantees the order:
- The transport platfrom device is probed first. It calls cros_ec_register().
- In cros_ec_register(), it registers the MFD device "cros-ec-dev". And the
children devices are added via mfd_add_devices().
Back to the issue we observed:
- The platform devices are created when it scans the tree in ACPI[6]. We
probably have no way to prevent the devices from adding unless specifying
`enumeration_by_parent`[8].
- When some of them are modules, the driver registrations are tied to the
module insertion. They can be arrived by anytime unless we use something
similar to soft dependency[9]. A Kconfig dependency will also
be needed to prevent cros_ec_lpcs=m but cros_ec_keyb=y. However,
cros_ec_keyb would need to specify 2 possible dependencies "cros_ec_lpcs"
and "cros_ec_spi"[10]. I'm not sure what would be happening if a system
has no cros_ec_spi module at all.
[7] https://elixir.bootlin.com/linux/v6.16/source/drivers/mfd/mfd-core.c#L184
[8] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/scan.c#L2204
[9] https://elixir.bootlin.com/linux/v6.16/source/include/linux/module.h#L176
[10] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/Kconfig#L743
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-09-05 8:38 ` Tzung-Bi Shih
@ 2025-09-14 1:08 ` Dmitry Torokhov
2025-09-14 3:26 ` Tzung-Bi Shih
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2025-09-14 1:08 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, linux-input, chrome-platform
On Fri, Sep 05, 2025 at 08:38:10AM +0000, Tzung-Bi Shih wrote:
> On Thu, Sep 04, 2025 at 07:06:23AM -0700, Dmitry Torokhov wrote:
> > On Tue, Sep 02, 2025 at 09:18:47PM +0800, Tzung-Bi Shih wrote:
> > > On Fri, Aug 29, 2025 at 08:50:01PM +0800, Tzung-Bi Shih wrote:
> > > > On Fri, Aug 29, 2025 at 11:28:55AM +0000, Dmitry Torokhov wrote:
> > > > > On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> > > > > > A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> > > > > > modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> > > > > > probed at the same time.
> > > > > >
> > > > > > Example:
> > > > > >
> > > > > > + -----------------------------------------------------------------+
> > > > > > | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> > > > > > + -----------------------------------------------------------------+
> > > > > > | Probe cros-ec-keyb. | |
> > > > > > | - Decide to defer[1]. | |
> > > > > > | | A device bound to a driver[2]. |
> > > > > > | Probe cros_ec_lpc. | |
> > > > > > | - Init the struct[3]. | |
> > > > > > | | Retry cros-ec-keyb from the |
> > > > > > | | deferred list[4]. |
> > > > > > | | - Won't defer again as [3]. |
> > > > > > | | - Access uninitialized data in |
> > > > > > | | the struct. |
> > > > > > | - Register the device. | |
> > > > > > + -----------------------------------------------------------------+
> > > > > >
> > > > > > [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> > > > > > [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> > > > > > [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> > > > > > [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
> > > > > >
> > > > > > Note that the device link[5] can't help as in the observed environment,
> > > > > > the devices are already added via device_add()[6].
> > > > > >
> > > > > > [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> > > > > > [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
> > > > > >
> > > > > > The series fixes the issue by ensuring the struct is ready for accessing
> > > > > > before continuing to probe cros-ec-keyb.
> > > > >
> > > > > Why is the keyboard platform device instantiated before the transport
> > > > > (cros_ec_lpc) is done initializing? I think this is the root of the
> > > > > issue...
> > > >
> > > > I may misunderstand but it seems to me:
> > > >
> > > > - The ACPI bus enumerated and instantiated the platform devices[6] first.
> > > >
> > > > - The keyboard platform device was probed when `cros_ec_keyb_driver`
> > > > registered. It deferred as its parent's drvdata was NULL[1].
> > > >
> > > > - The transport platform device was probed when `cros_ec_lpc_driver`
> > > > registered. It set the drvdata[3].
> > > >
> > > > - The keyboard platform device was probed again from retrying the deferred
> > > > list, by another thread `deferred_probe_work_func`. The parent's drvdata
> > > > wasn't NULL and cros_ec_register() for the transport device weren't
> > > > finished. The race happened.
> > >
> > > Hi Dmitry,
> > >
> > > Does it make sense to you?
> >
> > I'll have to research how MFD mixes up statically described and
> > DT-described platform devices and makes sure that children are not
> > probed before the parent is ready - I think we need to make cros_ec
> > behave the same way.
>
> I may misunderstand but FWIW:
>
> I failed to find relevant code in MFD [7] that guarantees the probe order.
> Also, I'm curious about wouldn't code at [7] results in duplicate platform
> devices? E.g., 1 populated from OF; 1 created by MFD.
>
> Note: current cros_ec_dev.c doesn't use `of_compatible` in struct mfd_cell.
>
> If we're looking at how cros_ec_dev.c guarantees the order:
>
> - The transport platfrom device is probed first. It calls cros_ec_register().
> - In cros_ec_register(), it registers the MFD device "cros-ec-dev". And the
> children devices are added via mfd_add_devices().
>
>
> Back to the issue we observed:
>
> - The platform devices are created when it scans the tree in ACPI[6]. We
> probably have no way to prevent the devices from adding unless specifying
> `enumeration_by_parent`[8].
>
> - When some of them are modules, the driver registrations are tied to the
> module insertion. They can be arrived by anytime unless we use something
> similar to soft dependency[9]. A Kconfig dependency will also
> be needed to prevent cros_ec_lpcs=m but cros_ec_keyb=y. However,
> cros_ec_keyb would need to specify 2 possible dependencies "cros_ec_lpcs"
> and "cros_ec_spi"[10]. I'm not sure what would be happening if a system
> has no cros_ec_spi module at all.
Sorry for the delay. I think we need to figure out how to delay device
registration until after the transport device is registered and probed.
Unfortunately I have not had time to look at this properly so I acked
the changes to cros_ec_keyb so that we can close the hole for now and
later we can try to see if there is a reasonable better solution.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-09-14 1:08 ` Dmitry Torokhov
@ 2025-09-14 3:26 ` Tzung-Bi Shih
0 siblings, 0 replies; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-09-14 3:26 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Benson Leung, linux-input, chrome-platform
On Sat, Sep 13, 2025 at 06:08:48PM -0700, Dmitry Torokhov wrote:
> On Fri, Sep 05, 2025 at 08:38:10AM +0000, Tzung-Bi Shih wrote:
> > On Thu, Sep 04, 2025 at 07:06:23AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Sep 02, 2025 at 09:18:47PM +0800, Tzung-Bi Shih wrote:
> > > > On Fri, Aug 29, 2025 at 08:50:01PM +0800, Tzung-Bi Shih wrote:
> > > > > On Fri, Aug 29, 2025 at 11:28:55AM +0000, Dmitry Torokhov wrote:
> > > > > > On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> > > > > > > A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> > > > > > > modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> > > > > > > probed at the same time.
> > > > > > >
> > > > > > > Example:
> > > > > > >
> > > > > > > + -----------------------------------------------------------------+
> > > > > > > | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> > > > > > > + -----------------------------------------------------------------+
> > > > > > > | Probe cros-ec-keyb. | |
> > > > > > > | - Decide to defer[1]. | |
> > > > > > > | | A device bound to a driver[2]. |
> > > > > > > | Probe cros_ec_lpc. | |
> > > > > > > | - Init the struct[3]. | |
> > > > > > > | | Retry cros-ec-keyb from the |
> > > > > > > | | deferred list[4]. |
> > > > > > > | | - Won't defer again as [3]. |
> > > > > > > | | - Access uninitialized data in |
> > > > > > > | | the struct. |
> > > > > > > | - Register the device. | |
> > > > > > > + -----------------------------------------------------------------+
> > > > > > >
> > > > > > > [1] https://elixir.bootlin.com/linux/v6.16/source/drivers/input/keyboard/cros_ec_keyb.c#L707
> > > > > > > [2] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L405
> > > > > > > [3] https://elixir.bootlin.com/linux/v6.16/source/drivers/platform/chrome/cros_ec_lpc.c#L644
> > > > > > > [4] https://elixir.bootlin.com/linux/v6.16/source/drivers/base/dd.c#L418
> > > > > > >
> > > > > > > Note that the device link[5] can't help as in the observed environment,
> > > > > > > the devices are already added via device_add()[6].
> > > > > > >
> > > > > > > [5] https://www.kernel.org/doc/html/latest/driver-api/device_link.html#usage
> > > > > > > [6] https://elixir.bootlin.com/linux/v6.16/source/drivers/acpi/acpi_platform.c#L177
> > > > > > >
> > > > > > > The series fixes the issue by ensuring the struct is ready for accessing
> > > > > > > before continuing to probe cros-ec-keyb.
> > > > > >
> > > > > > Why is the keyboard platform device instantiated before the transport
> > > > > > (cros_ec_lpc) is done initializing? I think this is the root of the
> > > > > > issue...
> > > > >
> > > > > I may misunderstand but it seems to me:
> > > > >
> > > > > - The ACPI bus enumerated and instantiated the platform devices[6] first.
> > > > >
> > > > > - The keyboard platform device was probed when `cros_ec_keyb_driver`
> > > > > registered. It deferred as its parent's drvdata was NULL[1].
> > > > >
> > > > > - The transport platform device was probed when `cros_ec_lpc_driver`
> > > > > registered. It set the drvdata[3].
> > > > >
> > > > > - The keyboard platform device was probed again from retrying the deferred
> > > > > list, by another thread `deferred_probe_work_func`. The parent's drvdata
> > > > > wasn't NULL and cros_ec_register() for the transport device weren't
> > > > > finished. The race happened.
> > > >
> > > > Hi Dmitry,
> > > >
> > > > Does it make sense to you?
> > >
> > > I'll have to research how MFD mixes up statically described and
> > > DT-described platform devices and makes sure that children are not
> > > probed before the parent is ready - I think we need to make cros_ec
> > > behave the same way.
> >
> > I may misunderstand but FWIW:
> >
> > I failed to find relevant code in MFD [7] that guarantees the probe order.
> > Also, I'm curious about wouldn't code at [7] results in duplicate platform
> > devices? E.g., 1 populated from OF; 1 created by MFD.
> >
> > Note: current cros_ec_dev.c doesn't use `of_compatible` in struct mfd_cell.
> >
> > If we're looking at how cros_ec_dev.c guarantees the order:
> >
> > - The transport platfrom device is probed first. It calls cros_ec_register().
> > - In cros_ec_register(), it registers the MFD device "cros-ec-dev". And the
> > children devices are added via mfd_add_devices().
> >
> >
> > Back to the issue we observed:
> >
> > - The platform devices are created when it scans the tree in ACPI[6]. We
> > probably have no way to prevent the devices from adding unless specifying
> > `enumeration_by_parent`[8].
> >
> > - When some of them are modules, the driver registrations are tied to the
> > module insertion. They can be arrived by anytime unless we use something
> > similar to soft dependency[9]. A Kconfig dependency will also
> > be needed to prevent cros_ec_lpcs=m but cros_ec_keyb=y. However,
> > cros_ec_keyb would need to specify 2 possible dependencies "cros_ec_lpcs"
> > and "cros_ec_spi"[10]. I'm not sure what would be happening if a system
> > has no cros_ec_spi module at all.
>
> Sorry for the delay. I think we need to figure out how to delay device
> registration until after the transport device is registered and probed.
> Unfortunately I have not had time to look at this properly so I acked
> the changes to cros_ec_keyb so that we can close the hole for now and
> later we can try to see if there is a reasonable better solution.
Thanks. Let's move forward with the current fix, even though we know it's
suboptimal.
My suspicion is that we lack a proper in-kernel dependency mechanism. This
is on my radar, and I'll investigate a better approach.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] platform/chrome: Fix a race when probing drivers
2025-08-28 8:35 [PATCH 0/5] platform/chrome: Fix a race when probing drivers Tzung-Bi Shih
` (5 preceding siblings ...)
2025-08-29 11:28 ` [PATCH 0/5] platform/chrome: Fix a race when probing drivers Dmitry Torokhov
@ 2025-09-14 3:47 ` Tzung-Bi Shih
6 siblings, 0 replies; 15+ messages in thread
From: Tzung-Bi Shih @ 2025-09-14 3:47 UTC (permalink / raw)
To: Dmitry Torokhov, Benson Leung; +Cc: linux-input, chrome-platform
On Thu, Aug 28, 2025 at 08:35:56AM +0000, Tzung-Bi Shih wrote:
> A race is observed when cros_ec_lpc and cros-ec-keyb are all built as
> modules. cros_ec_lpc is cros-ec-keyb's parent. However, they can be
> probed at the same time.
>
> Example:
>
> + -----------------------------------------------------------------+
> | Some init process (e.g. udevd) | deferred_probe_work_func worker |
> + -----------------------------------------------------------------+
> | Probe cros-ec-keyb. | |
> | - Decide to defer[1]. | |
> | | A device bound to a driver[2]. |
> | Probe cros_ec_lpc. | |
> | - Init the struct[3]. | |
> | | Retry cros-ec-keyb from the |
> | | deferred list[4]. |
> | | - Won't defer again as [3]. |
> | | - Access uninitialized data in |
> | | the struct. |
> | - Register the device. | |
> + -----------------------------------------------------------------+
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
[1/5] platform/chrome: Centralize cros_ec_device allocation
commit: 918856986014142271a70a334d300994b9c41720
[2/5] platform/chrome: Centralize common cros_ec_device initialization
commit: e19ceeb1c0f63e3e15b197c5f34797134b51ba0e
[3/5] platform/chrome: cros_ec: Separate initialization from cros_ec_register()
commit: 7a79b0bfd8b3995a39d25bffcf57273635c0e542
[4/5] platform/chrome: cros_ec: Add a flag to track registration state
commit: 56cb557279d70397cefb497e0f06bdd6fd685f8e
[5/5] Input: cros_ec_keyb - Defer probe until parent EC device is registered
commit: 48633acccf38d706d7b368400647bb9db9caf1ae
Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread