[PATCH v3 0/8] platform/chrome: cros_ec_chardev: Fix a possible UAF

[PATCH v3 0/8] platform/chrome: cros_ec_chardev: Fix a possible UAF

[Tzung-Bi Shih]

This is a follow-up series of [1].  It tries to fix a possible UAF in the
fops of cros_ec_chardev after the underlying protocol device has gone by
using kref in struct cros_ec_device.

The 1st and 2nd patches are clean-ups.

The 3rd patch removes the dependencies of fops to struct cros_ec_dev.  Thus,
it doesn't need to hold a reference to struct cros_ec_dev.

The 4th patch returns an error when trying to send commands through an
underlying protocol device which either hasn't been registered or has gone.

The 5th patch introduces a new helper cros_ec_device_alloc() for allocating
struct cros_ec_device properly (including to initialize the kref).

The 6th patch moves initialization of common utilities of struct cros_ec_device
to cros_ec_device_alloc() instead of staying in cros_ec_register() as the
object is still valid after unregistered.

The 7th patch lets the fops hold the kref when the file is opening.

The 8th patch starts to manage struct cros_ec_device's lifecycle by the kref.

[1] https://patchwork.kernel.org/project/chrome-platform/patch/20250703113509.2511758-3-tzungbi@kernel.org/

Changes from v2 (https://patchwork.kernel.org/project/chrome-platform/cover/20250708080034.3425427-1-tzungbi@kernel.org/):
- The 2nd and 6th patches of the series are new.

Tzung-Bi Shih (8):
  platform/chrome: cros_ec_chardev: Remove redundant struct field
  platform/chrome: cros_ec: Unregister notifier in cros_ec_unregister()
  platform/chrome: cros_ec_chardev: Decouple fops from struct
    cros_ec_dev
  platform/chrome: Disallow sending commands through unregistered ec_dev
  platform/chrome: Introduce cros_ec_device_alloc()
  platform/chrome: Don't initialize common utilities when registering
  platform/chrome: cros_ec_chardev: Hold refcount of struct
    cros_ec_device
  platform/chrome: Manage struct cros_ec_device lifecycle by its
    refcount

 drivers/platform/chrome/cros_ec.c           | 25 +++----
 drivers/platform/chrome/cros_ec_chardev.c   | 74 ++++++++++-----------
 drivers/platform/chrome/cros_ec_i2c.c       |  4 +-
 drivers/platform/chrome/cros_ec_ishtp.c     |  9 ++-
 drivers/platform/chrome/cros_ec_lpc.c       | 11 +--
 drivers/platform/chrome/cros_ec_proto.c     | 72 ++++++++++++++++++++
 drivers/platform/chrome/cros_ec_rpmsg.c     | 20 ++++--
 drivers/platform/chrome/cros_ec_spi.c       | 10 +--
 drivers/platform/chrome/cros_ec_uart.c      | 17 +++--
 include/linux/platform_data/cros_ec_proto.h | 10 +++
 10 files changed, 175 insertions(+), 77 deletions(-)

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v3 1/8] platform/chrome: cros_ec_chardev: Remove redundant struct field

[Tzung-Bi Shih]

`ec_dev` in the `struct chardev_data` is unused.  Remove the field
and the whole struct as well.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
No changes since v1 (https://patchwork.kernel.org/project/chrome-platform/patch/20250703113509.2511758-2-tzungbi@kernel.org/).

 drivers/platform/chrome/cros_ec_chardev.c | 28 +++++++++--------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index 21a484385fc5..5c858d30dd52 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -31,11 +31,6 @@
 /* Arbitrary bounded size for the event queue */
 #define CROS_MAX_EVENT_LEN	PAGE_SIZE
 
-struct chardev_data {
-	struct cros_ec_dev *ec_dev;
-	struct miscdevice misc;
-};
-
 struct chardev_priv {
 	struct cros_ec_dev *ec_dev;
 	struct notifier_block notifier;
@@ -379,29 +374,28 @@ static int cros_ec_chardev_probe(struct platform_device *pdev)
 {
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
 	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);
-	struct chardev_data *data;
+	struct miscdevice *misc;
 
 	/* Create a char device: we want to create it anew */
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
+	misc = devm_kzalloc(&pdev->dev, sizeof(*misc), GFP_KERNEL);
+	if (!misc)
 		return -ENOMEM;
 
-	data->ec_dev = ec_dev;
-	data->misc.minor = MISC_DYNAMIC_MINOR;
-	data->misc.fops = &chardev_fops;
-	data->misc.name = ec_platform->ec_name;
-	data->misc.parent = pdev->dev.parent;
+	misc->minor = MISC_DYNAMIC_MINOR;
+	misc->fops = &chardev_fops;
+	misc->name = ec_platform->ec_name;
+	misc->parent = pdev->dev.parent;
 
-	dev_set_drvdata(&pdev->dev, data);
+	dev_set_drvdata(&pdev->dev, misc);
 
-	return misc_register(&data->misc);
+	return misc_register(misc);
 }
 
 static void cros_ec_chardev_remove(struct platform_device *pdev)
 {
-	struct chardev_data *data = dev_get_drvdata(&pdev->dev);
+	struct miscdevice *misc = dev_get_drvdata(&pdev->dev);
 
-	misc_deregister(&data->misc);
+	misc_deregister(misc);
 }
 
 static const struct platform_device_id cros_ec_chardev_id[] = {
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v3 2/8] platform/chrome: cros_ec: Unregister notifier in cros_ec_unregister()

[Tzung-Bi Shih]

The blocking notifier is registered in cros_ec_register(); however, it
isn't unregistered in cros_ec_unregister().

Fix it.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
New patch in the v3 series.

 drivers/platform/chrome/cros_ec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 110771a8645e..fd58781a2fb7 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -318,6 +318,9 @@ EXPORT_SYMBOL(cros_ec_register);
  */
 void cros_ec_unregister(struct cros_ec_device *ec_dev)
 {
+	if (ec_dev->mkbp_event_supported)
+		blocking_notifier_chain_unregister(&ec_dev->event_notifier,
+						   &ec_dev->notifier_ready);
 	platform_device_unregister(ec_dev->pd);
 	platform_device_unregister(ec_dev->ec);
 	mutex_destroy(&ec_dev->lock);
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v3 2/8] platform/chrome: cros_ec: Unregister notifier in cros_ec_unregister()

[Greg KH]

On Mon, Jul 21, 2025 at 04:44:50AM +0000, Tzung-Bi Shih wrote:
> The blocking notifier is registered in cros_ec_register(); however, it
> isn't unregistered in cros_ec_unregister().
> 
> Fix it.

Shouldn't this be an independent patch, with the proper Fixes: and cc:
stable on it, so it can be applied now and backported correctly?  Why is
a bugfix in the middle of a new feature series?

thanks,

greg k-h

Re: [PATCH v3 2/8] platform/chrome: cros_ec: Unregister notifier in cros_ec_unregister()

[Tzung-Bi Shih]

On Mon, Jul 21, 2025 at 08:13:02AM +0200, Greg KH wrote:
> On Mon, Jul 21, 2025 at 04:44:50AM +0000, Tzung-Bi Shih wrote:
> > The blocking notifier is registered in cros_ec_register(); however, it
> > isn't unregistered in cros_ec_unregister().
> > 
> > Fix it.
> 
> Shouldn't this be an independent patch, with the proper Fixes: and cc:
> stable on it, so it can be applied now and backported correctly?  Why is
> a bugfix in the middle of a new feature series?

Sure, I'll separate the patch from the series.

[PATCH v3 3/8] platform/chrome: cros_ec_chardev: Decouple fops from struct cros_ec_dev

[Tzung-Bi Shih]

The fops doesn't really need to hold a reference to struct cros_ec_dev.
Remove the references from the fops.

No functional changes.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
No changes since v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-3-tzungbi@kernel.org/).

 drivers/platform/chrome/cros_ec_chardev.c | 44 +++++++++++------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index 5c858d30dd52..c9d80ad5b57e 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -32,12 +32,13 @@
 #define CROS_MAX_EVENT_LEN	PAGE_SIZE
 
 struct chardev_priv {
-	struct cros_ec_dev *ec_dev;
+	struct cros_ec_device *ec_dev;
 	struct notifier_block notifier;
 	wait_queue_head_t wait_event;
 	unsigned long event_mask;
 	struct list_head events;
 	size_t event_len;
+	u16 cmd_offset;
 };
 
 struct ec_event {
@@ -47,7 +48,7 @@ struct ec_event {
 	u8 data[];
 };
 
-static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
+static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen)
 {
 	static const char * const current_image_name[] = {
 		"unknown", "read-only", "read-write", "invalid",
@@ -60,10 +61,10 @@ static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
 	if (!msg)
 		return -ENOMEM;
 
-	msg->command = EC_CMD_GET_VERSION + ec->cmd_offset;
+	msg->command = EC_CMD_GET_VERSION + priv->cmd_offset;
 	msg->insize = sizeof(*resp);
 
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(priv->ec_dev, msg);
 	if (ret < 0) {
 		snprintf(str, maxlen,
 			 "Unknown EC version, returned error: %d\n",
@@ -91,7 +92,7 @@ static int cros_ec_chardev_mkbp_event(struct notifier_block *nb,
 {
 	struct chardev_priv *priv = container_of(nb, struct chardev_priv,
 						 notifier);
-	struct cros_ec_device *ec_dev = priv->ec_dev->ec_dev;
+	struct cros_ec_device *ec_dev = priv->ec_dev;
 	struct ec_event *event;
 	unsigned long event_bit = 1 << ec_dev->event_data.event_type;
 	int total_size = sizeof(*event) + ec_dev->event_size;
@@ -156,7 +157,8 @@ static struct ec_event *cros_ec_chardev_fetch_event(struct chardev_priv *priv,
 static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
 {
 	struct miscdevice *mdev = filp->private_data;
-	struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent);
+	struct cros_ec_dev *ec = dev_get_drvdata(mdev->parent);
+	struct cros_ec_device *ec_dev = ec->ec_dev;
 	struct chardev_priv *priv;
 	int ret;
 
@@ -165,13 +167,14 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
 		return -ENOMEM;
 
 	priv->ec_dev = ec_dev;
+	priv->cmd_offset = ec->cmd_offset;
 	filp->private_data = priv;
 	INIT_LIST_HEAD(&priv->events);
 	init_waitqueue_head(&priv->wait_event);
 	nonseekable_open(inode, filp);
 
 	priv->notifier.notifier_call = cros_ec_chardev_mkbp_event;
-	ret = blocking_notifier_chain_register(&ec_dev->ec_dev->event_notifier,
+	ret = blocking_notifier_chain_register(&ec_dev->event_notifier,
 					       &priv->notifier);
 	if (ret) {
 		dev_err(ec_dev->dev, "failed to register event notifier\n");
@@ -199,7 +202,6 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
 	char msg[sizeof(struct ec_response_get_version) +
 		 sizeof(CROS_EC_DEV_VERSION)];
 	struct chardev_priv *priv = filp->private_data;
-	struct cros_ec_dev *ec_dev = priv->ec_dev;
 	size_t count;
 	int ret;
 
@@ -233,7 +235,7 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
 	if (*offset != 0)
 		return 0;
 
-	ret = ec_get_version(ec_dev, msg, sizeof(msg));
+	ret = ec_get_version(priv, msg, sizeof(msg));
 	if (ret)
 		return ret;
 
@@ -249,10 +251,10 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
 static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
 {
 	struct chardev_priv *priv = filp->private_data;
-	struct cros_ec_dev *ec_dev = priv->ec_dev;
+	struct cros_ec_device *ec_dev = priv->ec_dev;
 	struct ec_event *event, *e;
 
-	blocking_notifier_chain_unregister(&ec_dev->ec_dev->event_notifier,
+	blocking_notifier_chain_unregister(&ec_dev->event_notifier,
 					   &priv->notifier);
 
 	list_for_each_entry_safe(event, e, &priv->events, node) {
@@ -267,7 +269,7 @@ static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
 /*
  * Ioctls
  */
-static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
+static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *arg)
 {
 	struct cros_ec_command *s_cmd;
 	struct cros_ec_command u_cmd;
@@ -296,8 +298,8 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
 		goto exit;
 	}
 
-	s_cmd->command += ec->cmd_offset;
-	ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
+	s_cmd->command += priv->cmd_offset;
+	ret = cros_ec_cmd_xfer(priv->ec_dev, s_cmd);
 	/* Only copy data to userland if data was received. */
 	if (ret < 0)
 		goto exit;
@@ -309,10 +311,9 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
 	return ret;
 }
 
-static long cros_ec_chardev_ioctl_readmem(struct cros_ec_dev *ec,
-					   void __user *arg)
+static long cros_ec_chardev_ioctl_readmem(struct chardev_priv *priv, void __user *arg)
 {
-	struct cros_ec_device *ec_dev = ec->ec_dev;
+	struct cros_ec_device *ec_dev = priv->ec_dev;
 	struct cros_ec_readmem s_mem = { };
 	long num;
 
@@ -341,16 +342,15 @@ static long cros_ec_chardev_ioctl(struct file *filp, unsigned int cmd,
 				   unsigned long arg)
 {
 	struct chardev_priv *priv = filp->private_data;
-	struct cros_ec_dev *ec = priv->ec_dev;
 
 	if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
 		return -ENOTTY;
 
 	switch (cmd) {
 	case CROS_EC_DEV_IOCXCMD:
-		return cros_ec_chardev_ioctl_xcmd(ec, (void __user *)arg);
+		return cros_ec_chardev_ioctl_xcmd(priv, (void __user *)arg);
 	case CROS_EC_DEV_IOCRDMEM:
-		return cros_ec_chardev_ioctl_readmem(ec, (void __user *)arg);
+		return cros_ec_chardev_ioctl_readmem(priv, (void __user *)arg);
 	case CROS_EC_DEV_IOCEVENTMASK:
 		priv->event_mask = arg;
 		return 0;
@@ -372,8 +372,8 @@ static const struct file_operations chardev_fops = {
 
 static int cros_ec_chardev_probe(struct platform_device *pdev)
 {
-	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
-	struct cros_ec_platform *ec_platform = dev_get_platdata(ec_dev->dev);
+	struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
+	struct cros_ec_platform *ec_platform = dev_get_platdata(ec->dev);
 	struct miscdevice *misc;
 
 	/* Create a char device: we want to create it anew */
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v3 4/8] platform/chrome: Disallow sending commands through unregistered ec_dev

[Tzung-Bi Shih]

Return earlier if attempting to send commands through an unregistered
struct cros_ec_device.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
Changes from v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-4-tzungbi@kernel.org/):
- Use atomic_t for `registered` so that it doesn't need to be handled with
  locks.
- Add new helper function cros_ec_device_registered().

 drivers/platform/chrome/cros_ec.c           |  5 +++++
 drivers/platform/chrome/cros_ec_proto.c     | 17 +++++++++++++++++
 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 fd58781a2fb7..42512e2b4f65 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/atomic.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
@@ -200,6 +201,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 	if (!ec_dev->dout)
 		return -ENOMEM;
 
+	atomic_set(&ec_dev->registered, 1);
 	lockdep_register_key(&ec_dev->lockdep_key);
 	mutex_init(&ec_dev->lock);
 	lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
@@ -300,6 +302,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 
 	return 0;
 exit:
+	atomic_set(&ec_dev->registered, 0);
 	platform_device_unregister(ec_dev->ec);
 	platform_device_unregister(ec_dev->pd);
 	mutex_destroy(&ec_dev->lock);
@@ -318,6 +321,8 @@ EXPORT_SYMBOL(cros_ec_register);
  */
 void cros_ec_unregister(struct cros_ec_device *ec_dev)
 {
+	atomic_set(&ec_dev->registered, 0);
+
 	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..ee3050ffe226 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/atomic.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/limits.h>
@@ -656,6 +657,11 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
 {
 	int ret;
 
+	if (!cros_ec_device_registered(ec_dev)) {
+		dev_err(ec_dev->dev, "unregistered ec_dev; cannot send command\n");
+		return -ENODEV;
+	}
+
 	mutex_lock(&ec_dev->lock);
 	if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) {
 		ret = cros_ec_query_all(ec_dev);
@@ -1153,5 +1159,16 @@ 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 - Check if the ec_dev is registered.
+ *
+ * @ec_dev: EC device
+ */
+bool cros_ec_device_registered(struct cros_ec_device *ec_dev)
+{
+	return atomic_read(&ec_dev->registered) == 1;
+}
+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 3ec24f445c29..9ab83a8318ce 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -122,6 +122,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: 1 if this device had been registered; otherwise 0.
  * @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
@@ -180,6 +181,7 @@ struct cros_ec_device {
 	int dout_size;
 	bool wake_enabled;
 	bool suspended;
+	atomic_t registered;
 	int (*cmd_xfer)(struct cros_ec_device *ec,
 			struct cros_ec_command *msg);
 	int (*pkt_xfer)(struct cros_ec_device *ec,
@@ -272,6 +274,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.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v3 4/8] platform/chrome: Disallow sending commands through unregistered ec_dev

[Greg KH]

On Mon, Jul 21, 2025 at 04:44:52AM +0000, Tzung-Bi Shih wrote:
> Return earlier if attempting to send commands through an unregistered
> struct cros_ec_device.

Why would it ever be "uregistered"?

> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> Changes from v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-4-tzungbi@kernel.org/):
> - Use atomic_t for `registered` so that it doesn't need to be handled with
>   locks.
> - Add new helper function cros_ec_device_registered().
> 
>  drivers/platform/chrome/cros_ec.c           |  5 +++++
>  drivers/platform/chrome/cros_ec_proto.c     | 17 +++++++++++++++++
>  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 fd58781a2fb7..42512e2b4f65 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/atomic.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
> @@ -200,6 +201,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  	if (!ec_dev->dout)
>  		return -ENOMEM;
>  
> +	atomic_set(&ec_dev->registered, 1);
>  	lockdep_register_key(&ec_dev->lockdep_key);
>  	mutex_init(&ec_dev->lock);
>  	lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
> @@ -300,6 +302,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  
>  	return 0;
>  exit:
> +	atomic_set(&ec_dev->registered, 0);
>  	platform_device_unregister(ec_dev->ec);
>  	platform_device_unregister(ec_dev->pd);
>  	mutex_destroy(&ec_dev->lock);
> @@ -318,6 +321,8 @@ EXPORT_SYMBOL(cros_ec_register);
>   */
>  void cros_ec_unregister(struct cros_ec_device *ec_dev)
>  {
> +	atomic_set(&ec_dev->registered, 0);
> +
>  	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..ee3050ffe226 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/atomic.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/limits.h>
> @@ -656,6 +657,11 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
>  {
>  	int ret;
>  
> +	if (!cros_ec_device_registered(ec_dev)) {
> +		dev_err(ec_dev->dev, "unregistered ec_dev; cannot send command\n");
> +		return -ENODEV;
> +	}
> +
>  	mutex_lock(&ec_dev->lock);
>  	if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) {
>  		ret = cros_ec_query_all(ec_dev);
> @@ -1153,5 +1159,16 @@ 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 - Check if the ec_dev is registered.
> + *
> + * @ec_dev: EC device
> + */
> +bool cros_ec_device_registered(struct cros_ec_device *ec_dev)
> +{
> +	return atomic_read(&ec_dev->registered) == 1;
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_device_registered);

This isn't going to do what you think it does :(

Hint, the state can change right after you call this, making it
pointless.

If you really need this (and huge hint, I do not think you do, as
something must be wrong if you ever need it), use a lock properly, not
just an atomic value, as that doesn't work here at all, and is kind of
pointless.

So step back, what exactly is the problem here that you are trying to
solve with this?

thanks,

greg k-h

Re: [PATCH v3 4/8] platform/chrome: Disallow sending commands through unregistered ec_dev

[Tzung-Bi Shih]

On Mon, Jul 21, 2025 at 07:47:47AM +0200, Greg KH wrote:
> On Mon, Jul 21, 2025 at 04:44:52AM +0000, Tzung-Bi Shih wrote:
> > Return earlier if attempting to send commands through an unregistered
> > struct cros_ec_device.
> 
> Why would it ever be "uregistered"?

If the EC rebooted while a userland program opened a file of the
chardev in cros_ec_chardev, the struct cros_ec_device [1][2] may be
unregistered due to some drivers re-probe the device.

The issue mainly comes from the discussion of cros_ec_usb [3]: using a
`registered_list` vs. calling cros_ec_{un,}register() everytime in
usb_driver's .probe()/.disconnect().

[1] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_chardev.c#L241
[2] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_chardev.c#L71
[3] https://lore.kernel.org/chrome-platform/20250624110028.409318-1-dawidn@google.com/T/#u

> > +/**
> > + * cros_ec_device_registered - Check if the ec_dev is registered.
> > + *
> > + * @ec_dev: EC device
> > + */
> > +bool cros_ec_device_registered(struct cros_ec_device *ec_dev)
> > +{
> > +	return atomic_read(&ec_dev->registered) == 1;
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_device_registered);
> 
> This isn't going to do what you think it does :(
> 
> Hint, the state can change right after you call this, making it
> pointless.

In current use cases, once it turns from "registered" to "unregistered", it
should never back to "registered".  The flag is more or less to let the
kernel know the struct cros_ec_device is stale.  The object is still valid
for accessing because an opening file instance is referencing.

Re: [PATCH v3 4/8] platform/chrome: Disallow sending commands through unregistered ec_dev

[Greg KH]

On Mon, Jul 21, 2025 at 09:31:05AM +0000, Tzung-Bi Shih wrote:
> On Mon, Jul 21, 2025 at 07:47:47AM +0200, Greg KH wrote:
> > On Mon, Jul 21, 2025 at 04:44:52AM +0000, Tzung-Bi Shih wrote:
> > > Return earlier if attempting to send commands through an unregistered
> > > struct cros_ec_device.
> > 
> > Why would it ever be "uregistered"?
> 
> If the EC rebooted while a userland program opened a file of the
> chardev in cros_ec_chardev, the struct cros_ec_device [1][2] may be
> unregistered due to some drivers re-probe the device.

Ah, the fun "disconnect the device while the file is open" issue we have
in many subsystems :(

> The issue mainly comes from the discussion of cros_ec_usb [3]: using a
> `registered_list` vs. calling cros_ec_{un,}register() everytime in
> usb_driver's .probe()/.disconnect().
> 
> [1] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_chardev.c#L241
> [2] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_chardev.c#L71
> [3] https://lore.kernel.org/chrome-platform/20250624110028.409318-1-dawidn@google.com/T/#u
> 
> > > +/**
> > > + * cros_ec_device_registered - Check if the ec_dev is registered.
> > > + *
> > > + * @ec_dev: EC device
> > > + */
> > > +bool cros_ec_device_registered(struct cros_ec_device *ec_dev)
> > > +{
> > > +	return atomic_read(&ec_dev->registered) == 1;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cros_ec_device_registered);
> > 
> > This isn't going to do what you think it does :(
> > 
> > Hint, the state can change right after you call this, making it
> > pointless.
> 
> In current use cases, once it turns from "registered" to "unregistered", it
> should never back to "registered".  The flag is more or less to let the
> kernel know the struct cros_ec_device is stale.  The object is still valid
> for accessing because an opening file instance is referencing.

But again, you don't know if it just went from "registered" ->
"unregistered" after calling this function and getting a return value of
"I am registered!".

Do it right, with a real lock, an atomic value provides nothing here
other than the potential to reduce a race window, not actually prevent
it from happening at all.

thanks,

greg k-h

[PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih]

Prepare to decouple the lifecycle of struct cros_ec_device from specific
device by introducing a kref.

Replace all occurrences of kzalloc for struct cros_ec_device to a new
helper cros_ec_device_alloc() and initialize the kref in the helper.

No functional changes.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
No changes since v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-5-tzungbi@kernel.org/).

 drivers/platform/chrome/cros_ec.c           |  7 +++-
 drivers/platform/chrome/cros_ec_i2c.c       |  4 +-
 drivers/platform/chrome/cros_ec_ishtp.c     |  9 +++--
 drivers/platform/chrome/cros_ec_lpc.c       | 11 +++--
 drivers/platform/chrome/cros_ec_proto.c     | 45 +++++++++++++++++++++
 drivers/platform/chrome/cros_ec_rpmsg.c     | 20 +++++----
 drivers/platform/chrome/cros_ec_spi.c       | 10 +++--
 drivers/platform/chrome/cros_ec_uart.c      | 17 +++++---
 include/linux/platform_data/cros_ec_proto.h |  6 +++
 9 files changed, 101 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 42512e2b4f65..9604424b6e7a 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -173,8 +173,10 @@ static int cros_ec_ready_event(struct notifier_block *nb,
  * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
  * @ec_dev: Device to register.
  *
- * Before calling this, allocate a pointer to a new device and then fill
- * in all the fields up to the --private-- marker.
+ * Before calling this, allocate a pointer to a new device via
+ * cros_ec_device_alloc() and then fill in all the required fields.
+ *
+ * On success, the ownership of refcount of the ec_dev will be taken.
  *
  * Return: 0 on success or negative error code.
  */
@@ -330,6 +332,7 @@ void cros_ec_unregister(struct cros_ec_device *ec_dev)
 	platform_device_unregister(ec_dev->ec);
 	mutex_destroy(&ec_dev->lock);
 	lockdep_unregister_key(&ec_dev->lockdep_key);
+	cros_ec_device_put(ec_dev);
 }
 EXPORT_SYMBOL(cros_ec_unregister);
 
diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index 38af97cdaab2..26c32ceef2bd 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -292,12 +292,11 @@ static int cros_ec_i2c_probe(struct i2c_client *client)
 	struct cros_ec_device *ec_dev = NULL;
 	int err;
 
-	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+	ec_dev = cros_ec_device_alloc(dev);
 	if (!ec_dev)
 		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;
@@ -311,6 +310,7 @@ static int cros_ec_i2c_probe(struct i2c_client *client)
 	err = cros_ec_register(ec_dev);
 	if (err) {
 		dev_err(dev, "cannot register EC\n");
+		cros_ec_device_put(ec_dev);
 		return err;
 	}
 
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index 7e7190b30cbb..fdf61acb6772 100644
--- a/drivers/platform/chrome/cros_ec_ishtp.c
+++ b/drivers/platform/chrome/cros_ec_ishtp.c
@@ -542,15 +542,15 @@ 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);
+	int ret;
 
-	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+	ec_dev = cros_ec_device_alloc(dev);
 	if (!ec_dev)
 		return -ENOMEM;
 
 	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;
@@ -559,7 +559,10 @@ static int cros_ec_dev_init(struct ishtp_cl_data *client_data)
 			   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);
+	ret = cros_ec_register(ec_dev);
+	if (ret)
+		cros_ec_device_put(ec_dev);
+	return ret;
 }
 
 static void reset_handler(struct work_struct *work)
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 7d9a78289c96..f75849cc14c3 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -637,12 +637,11 @@ 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;
 
 	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;
@@ -661,13 +660,14 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 		ec_dev->irq = irq;
 	else if (irq != -ENXIO) {
 		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
-		return irq;
+		ret = irq;
+		goto err;
 	}
 
 	ret = cros_ec_register(ec_dev);
 	if (ret) {
 		dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
-		return ret;
+		goto err;
 	}
 
 	/*
@@ -685,6 +685,9 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	}
 
 	return 0;
+err:
+	cros_ec_device_put(ec_dev);
+	return ret;
 }
 
 static void cros_ec_lpc_remove(struct platform_device *pdev)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index ee3050ffe226..7fac4d7c1b78 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -1159,6 +1159,51 @@ 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_alloc - Allocate memory for struct cros_ec_device.
+ *
+ * @dev: The associated device.
+ */
+struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
+{
+	struct cros_ec_device *ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+
+	if (ec_dev) {
+		ec_dev->dev = dev;
+		kref_init(&ec_dev->kref);
+	}
+
+	return ec_dev;
+}
+EXPORT_SYMBOL_GPL(cros_ec_device_alloc);
+
+/**
+ * cros_ec_device_get - Get a refcount of the ec_dev.
+ *
+ * @ec_dev: EC device
+ */
+void cros_ec_device_get(struct cros_ec_device *ec_dev)
+{
+	kref_get(&ec_dev->kref);
+}
+EXPORT_SYMBOL_GPL(cros_ec_device_get);
+
+static void cros_ec_device_release(struct kref *kref)
+{
+	/* Do not free the cros_ec_device as it is still dev-managed. */
+}
+
+/**
+ * cros_ec_device_put - Put a refcount of the ec_dev.
+ *
+ * @ec_dev: EC device
+ */
+void cros_ec_device_put(struct cros_ec_device *ec_dev)
+{
+	kref_put(&ec_dev->kref, cros_ec_device_release);
+}
+EXPORT_SYMBOL_GPL(cros_ec_device_put);
+
 /**
  * cros_ec_device_registered - Check if the ec_dev is registered.
  *
diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
index bc2666491db1..85afb15cd162 100644
--- a/drivers/platform/chrome/cros_ec_rpmsg.c
+++ b/drivers/platform/chrome/cros_ec_rpmsg.c
@@ -216,15 +216,16 @@ 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;
 
 	ec_rpmsg = devm_kzalloc(dev, sizeof(*ec_rpmsg), GFP_KERNEL);
-	if (!ec_rpmsg)
-		return -ENOMEM;
+	if (!ec_rpmsg) {
+		ret = -ENOMEM;
+		goto err;
+	}
 
-	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;
@@ -240,14 +241,16 @@ static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
 		  cros_ec_rpmsg_host_event_function);
 
 	ec_rpmsg->ept = cros_ec_rpmsg_create_ept(rpdev);
-	if (!ec_rpmsg->ept)
-		return -ENOMEM;
+	if (!ec_rpmsg->ept) {
+		ret = -ENOMEM;
+		goto err;
+	}
 
 	ret = cros_ec_register(ec_dev);
 	if (ret < 0) {
 		rpmsg_destroy_ept(ec_rpmsg->ept);
 		cancel_work_sync(&ec_rpmsg->host_event_work);
-		return ret;
+		goto err;
 	}
 
 	ec_rpmsg->probe_done = true;
@@ -256,6 +259,9 @@ static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
 		schedule_work(&ec_rpmsg->host_event_work);
 
 	return 0;
+err:
+	cros_ec_device_put(ec_dev);
+	return ret;
 }
 
 static void cros_ec_rpmsg_remove(struct rpmsg_device *rpdev)
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8ca0f854e7ac..3f31d9ee4335 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;
 
@@ -757,7 +757,6 @@ 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;
@@ -772,17 +771,20 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 
 	err = cros_ec_spi_devm_high_pri_alloc(dev, ec_spi);
 	if (err)
-		return err;
+		goto fail;
 
 	err = cros_ec_register(ec_dev);
 	if (err) {
 		dev_err(dev, "cannot register EC\n");
-		return err;
+		goto fail;
 	}
 
 	device_init_wakeup(&spi->dev, true);
 
 	return 0;
+fail:
+	cros_ec_device_put(ec_dev);
+	return err;
 }
 
 static void cros_ec_spi_remove(struct spi_device *spi)
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 19c179d49c90..9ee7ad9dcc0e 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;
 
@@ -271,12 +271,11 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
 	ret = cros_ec_uart_acpi_probe(ec_uart);
 	if (ret < 0) {
 		dev_err(dev, "Failed to get ACPI info (%d)", ret);
-		return ret;
+		goto err;
 	}
 
 	/* 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;
@@ -290,18 +289,24 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
 	ret = devm_serdev_device_open(dev, serdev);
 	if (ret) {
 		dev_err(dev, "Unable to open UART device");
-		return ret;
+		goto err;
 	}
 
 	ret = serdev_device_set_baudrate(serdev, ec_uart->baudrate);
 	if (ret < 0) {
 		dev_err(dev, "Failed to set up host baud rate (%d)", ret);
-		return ret;
+		goto err;
 	}
 
 	serdev_device_set_flow_control(serdev, ec_uart->flowcontrol);
 
-	return cros_ec_register(ec_dev);
+	ret = cros_ec_register(ec_dev);
+	if (ret)
+		goto err;
+	return 0;
+err:
+	cros_ec_device_put(ec_dev);
+	return ret;
 }
 
 static void cros_ec_uart_remove(struct serdev_device *serdev)
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 9ab83a8318ce..ecab81ed4f2b 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -159,6 +159,7 @@ struct cros_ec_command {
  * @pd: The platform_device used by the mfd driver to interface with the
  *      PD behind an EC.
  * @panic_notifier: EC panic notifier.
+ * @kref: The refcount.
  */
 struct cros_ec_device {
 	/* These are used by other drivers that want to talk to the EC */
@@ -205,6 +206,8 @@ struct cros_ec_device {
 	struct platform_device *pd;
 
 	struct blocking_notifier_head panic_notifier;
+
+	struct kref kref;
 };
 
 /**
@@ -274,6 +277,9 @@ 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);
 
+struct cros_ec_device *cros_ec_device_alloc(struct device *dev);
+void cros_ec_device_get(struct cros_ec_device *ec_dev);
+void cros_ec_device_put(struct cros_ec_device *ec_dev);
 bool cros_ec_device_registered(struct cros_ec_device *ec_dev);
 
 /**
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Greg KH]

On Mon, Jul 21, 2025 at 04:44:53AM +0000, Tzung-Bi Shih wrote:
> Prepare to decouple the lifecycle of struct cros_ec_device from specific
> device by introducing a kref.

Ick, are you sure?  This is a device, so use struct device for it.
That's what it is there for.

So shouldn't this be its own 'struct device' and let the driver core
handle it correctly instead of trying to have a "child" structure with a
reference count that is not shown in sysfs at all?

thanks,

greg k-h

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih]

On Mon, Jul 21, 2025 at 08:15:07AM +0200, Greg KH wrote:
> On Mon, Jul 21, 2025 at 04:44:53AM +0000, Tzung-Bi Shih wrote:
> > Prepare to decouple the lifecycle of struct cros_ec_device from specific
> > device by introducing a kref.
> 
> Ick, are you sure?  This is a device, so use struct device for it.
> That's what it is there for.
> 
> So shouldn't this be its own 'struct device' and let the driver core
> handle it correctly instead of trying to have a "child" structure with a
> reference count that is not shown in sysfs at all?

Thanks for the review.

To make sure I understand, do the following patches (draft and simplified) make
sense?  Short summary:
- cros_ec_device_alloc() allocates resources and setup the destructor.
- cros_ec_register() adds the device to the device hierarchy.
- cros_ec_unregister() removes the device.
- cros_ec_chardev_open() gets a refcount of the device.
- cros_ec_chardev_release() puts the refcount.

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index f8ffdc1e1b16..4d0f1ffc3387 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -11,0 +12 @@
+#include <linux/idr.h>
@@ -22,0 +24,6 @@
+static DEFINE_IDA(cros_ec_devid_ida);
+
+static struct class cros_ec_class = {
+	.name = "cros_ec",
+};
+
@@ -101,0 +109,44 @@ EXPORT_SYMBOL(cros_ec_irq_thread);
+static void cros_ec_device_release(struct device *dev)
+{
+	struct cros_ec_device *ec_dev = container_of(dev, struct cros_ec_device, this_dev);
+
+	ida_free(&cros_ec_devid_ida, ec_dev->devid);
+	kfree(ec_dev);
+}
+
+/**
+ * cros_ec_device_alloc - Allocate a new struct cros_ec_device.
+ *
+ * @parent: The parent device.
+ */
+struct cros_ec_device *cros_ec_device_alloc(struct device *parent)
+{
+	struct cros_ec_device *ec_dev = NULL;
+	int id = -1;
+
+	ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
+	if (!ec_dev)
+		goto err;
+
+	id = ida_alloc(&cros_ec_devid_ida, GFP_KERNEL);
+	if (id < 0)
+		goto err;
+
+	ec_dev->dev = &ec_dev->this_dev;
+	ec_dev->this_dev.parent = parent;
+	ec_dev->this_dev.release = cros_ec_device_release;
+	ec_dev->this_dev.class = &cros_ec_class;
+	ec_dev->devid = id;
+
+	device_initialize(&ec_dev->this_dev);
+	dev_set_name(&ec_dev->this_dev, "cros_ec_transport.%d", id);
+	dev_set_drvdata(&ec_dev->this_dev, ec_dev);
+	return ec_dev;
+err:
+	if (id >= 0)
+		ida_free(&cros_ec_devid_ida, id);
+	kfree(ec_dev);
+	return NULL;
+}
+EXPORT_SYMBOL(cros_ec_device_alloc);
+
@@ -206,0 +258,4 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
+	err = device_add(dev);
+	if (err)
+		return err;
+
@@ -334,0 +390,2 @@ void cros_ec_unregister(struct cros_ec_device *ec_dev)
+
+	device_unregister(&ec_dev->this_dev);
@@ -500,0 +558,19 @@ EXPORT_SYMBOL(cros_ec_resume);
+static int __init cros_ec_init(void)
+{
+	int ret;
+
+	ret = class_register(&cros_ec_class);
+	if (ret)
+		pr_err("cros_ec: Failed to register device class\n");
+
+	return ret;
+}
+
+static void __exit cros_ec_exit(void)
+{
+	class_unregister(&cros_ec_class);
+}
+
+module_init(cros_ec_init);
+module_exit(cros_ec_exit);
+

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 8ca0f854e7ac..6851e466e1dc 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -752 +752 @@ static int cros_ec_spi_probe(struct spi_device *spi)
-	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+	ec_dev = cros_ec_device_alloc(dev);
@@ -760 +759,0 @@ static int cros_ec_spi_probe(struct spi_device *spi)
-	ec_dev->dev = dev;

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c9d80ad5b57e..13109ef6e452 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -183,0 +184 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
+	get_device(ec_dev->dev);
@@ -258,0 +260 @@ static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
+	put_device(ec_dev->dev);

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Greg KH]

On Thu, Jul 24, 2025 at 09:58:39AM +0000, Tzung-Bi Shih wrote:
> On Mon, Jul 21, 2025 at 08:15:07AM +0200, Greg KH wrote:
> > On Mon, Jul 21, 2025 at 04:44:53AM +0000, Tzung-Bi Shih wrote:
> > > Prepare to decouple the lifecycle of struct cros_ec_device from specific
> > > device by introducing a kref.
> > 
> > Ick, are you sure?  This is a device, so use struct device for it.
> > That's what it is there for.
> > 
> > So shouldn't this be its own 'struct device' and let the driver core
> > handle it correctly instead of trying to have a "child" structure with a
> > reference count that is not shown in sysfs at all?
> 
> Thanks for the review.
> 
> To make sure I understand, do the following patches (draft and simplified) make
> sense?  Short summary:
> - cros_ec_device_alloc() allocates resources and setup the destructor.
> - cros_ec_register() adds the device to the device hierarchy.
> - cros_ec_unregister() removes the device.
> - cros_ec_chardev_open() gets a refcount of the device.
> - cros_ec_chardev_release() puts the refcount.

NEVER attempt to increment/decrement refcounts on open/release.  That
way lies madness and should not be needed at all, the underlying
infrastructure should keep things working properly here, right?

thanks,

greg k-h

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih]

On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> On Thu, Jul 24, 2025 at 09:58:39AM +0000, Tzung-Bi Shih wrote:
> > On Mon, Jul 21, 2025 at 08:15:07AM +0200, Greg KH wrote:
> > > On Mon, Jul 21, 2025 at 04:44:53AM +0000, Tzung-Bi Shih wrote:
> > > > Prepare to decouple the lifecycle of struct cros_ec_device from specific
> > > > device by introducing a kref.
> > > 
> > > Ick, are you sure?  This is a device, so use struct device for it.
> > > That's what it is there for.
> > > 
> > > So shouldn't this be its own 'struct device' and let the driver core
> > > handle it correctly instead of trying to have a "child" structure with a
> > > reference count that is not shown in sysfs at all?
> > 
> > Thanks for the review.
> > 
> > To make sure I understand, do the following patches (draft and simplified) make
> > sense?  Short summary:
> > - cros_ec_device_alloc() allocates resources and setup the destructor.
> > - cros_ec_register() adds the device to the device hierarchy.
> > - cros_ec_unregister() removes the device.
> > - cros_ec_chardev_open() gets a refcount of the device.
> > - cros_ec_chardev_release() puts the refcount.
> 
> NEVER attempt to increment/decrement refcounts on open/release.  That
> way lies madness and should not be needed at all, the underlying
> infrastructure should keep things working properly here, right?

Pardon me, but I don't really understand.  If it doesn't hold a refcount when
the file opening, how could it make sure the userland program accesses valid
memory after the underlying device has been removed?

Imagining the scenario (without holding a refcount):

Refcount    Event
--------------------------------------------------------------------
N/A         An USB device connected.

            The usb_driver's probe() called:
            - cros_ec_device_alloc()
                -> kzalloc() the struct cros_ec_device
1               -> device_initialize()
            - cros_ec_register()
1+N             -> device_add()

            An userland program opened a FD.

            The USB device disconnected.

            The usb_driver's disconnect() called:
            - cros_ec_unregister()
                  -> device_unregister()
1                     -> device_del()
0                     -> put_device().  The final reference count.
                          -> release()
                          -> kfree() the struct cros_ec_device

            The userland program accesses the struct cros_ec_device.

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Greg KH]

On Thu, Jul 24, 2025 at 01:32:01PM +0000, Tzung-Bi Shih wrote:
> On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> > On Thu, Jul 24, 2025 at 09:58:39AM +0000, Tzung-Bi Shih wrote:
> > > On Mon, Jul 21, 2025 at 08:15:07AM +0200, Greg KH wrote:
> > > > On Mon, Jul 21, 2025 at 04:44:53AM +0000, Tzung-Bi Shih wrote:
> > > > > Prepare to decouple the lifecycle of struct cros_ec_device from specific
> > > > > device by introducing a kref.
> > > > 
> > > > Ick, are you sure?  This is a device, so use struct device for it.
> > > > That's what it is there for.
> > > > 
> > > > So shouldn't this be its own 'struct device' and let the driver core
> > > > handle it correctly instead of trying to have a "child" structure with a
> > > > reference count that is not shown in sysfs at all?
> > > 
> > > Thanks for the review.
> > > 
> > > To make sure I understand, do the following patches (draft and simplified) make
> > > sense?  Short summary:
> > > - cros_ec_device_alloc() allocates resources and setup the destructor.
> > > - cros_ec_register() adds the device to the device hierarchy.
> > > - cros_ec_unregister() removes the device.
> > > - cros_ec_chardev_open() gets a refcount of the device.
> > > - cros_ec_chardev_release() puts the refcount.
> > 
> > NEVER attempt to increment/decrement refcounts on open/release.  That
> > way lies madness and should not be needed at all, the underlying
> > infrastructure should keep things working properly here, right?
> 
> Pardon me, but I don't really understand.  If it doesn't hold a refcount when
> the file opening, how could it make sure the userland program accesses valid
> memory after the underlying device has been removed?
> 
> Imagining the scenario (without holding a refcount):
> 
> Refcount    Event
> --------------------------------------------------------------------
> N/A         An USB device connected.
> 
>             The usb_driver's probe() called:
>             - cros_ec_device_alloc()
>                 -> kzalloc() the struct cros_ec_device
> 1               -> device_initialize()
>             - cros_ec_register()
> 1+N             -> device_add()
> 
>             An userland program opened a FD.
> 
>             The USB device disconnected.
> 
>             The usb_driver's disconnect() called:
>             - cros_ec_unregister()
>                   -> device_unregister()
> 1                     -> device_del()
> 0                     -> put_device().  The final reference count.
>                           -> release()
>                           -> kfree() the struct cros_ec_device
> 
>             The userland program accesses the struct cros_ec_device.

You are attempting to have one reference count for one object that has
different lifecycles.  That doesn't work well, if at all.  See the
numerous talks at the Linux Plumbers conference over the past few years
about why this is a "bad idea" and for ideas on how to redesign it to
fix the issue.

Hint, 2 objects, different reference counts :)

thanks,

greg k-h

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih]

On Fri, Jul 25, 2025 at 06:58:23AM +0200, Greg KH wrote:
> On Thu, Jul 24, 2025 at 01:32:01PM +0000, Tzung-Bi Shih wrote:
> > On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> > > NEVER attempt to increment/decrement refcounts on open/release.  That
> > > way lies madness and should not be needed at all, the underlying
> > > infrastructure should keep things working properly here, right?
> > [...]
> You are attempting to have one reference count for one object that has
> different lifecycles.  That doesn't work well, if at all.  See the
> numerous talks at the Linux Plumbers conference over the past few years
> about why this is a "bad idea" and for ideas on how to redesign it to
> fix the issue.
> 
> Hint, 2 objects, different reference counts :)

Referenced the talk [1] and its related works and hope I understand it
correctly.

[1] https://lpc.events/event/17/contributions/1627/

Summarize the proposed fix and I hope it makes sense:

Introduce a new helper: ref_proxy for managing "weak" references to the
object that may be gone at any time.  `ref` is the target pointer (i.e.
the resource) which could be released asynchronously (from the resource
consumers' point of view).  The struct ref_proxy_struct is for resource
providers and is only freed on dropping the final reference.

  +struct ref_proxy_struct {
  +       struct srcu_struct srcu;
  +       void __rcu *ref;
  +       struct kref kref;
  +};
  +
  +struct ref_proxy_struct *ref_proxy_struct_alloc(void *ref)
  +{
  +       struct ref_proxy_struct *rp;
  +
  +       rp = kzalloc(sizeof(*rp), GFP_KERNEL);
  +       if (!rp)
  +               return NULL;
  +
  +       init_srcu_struct(&rp->srcu);
  +       rcu_assign_pointer(rp->ref, ref);
  +       synchronize_srcu(&rp->srcu);
  +       kref_init(&rp->kref);
  +
  +       return rp;
  +}
  +EXPORT_SYMBOL(ref_proxy_struct_alloc);
  +
  +static void ref_proxy_struct_release(struct kref *kref)
  +{
  +       struct ref_proxy_struct *rp = container_of(kref, struct ref_proxy_struct, kref);
  +
  +       cleanup_srcu_struct(&rp->srcu);
  +       kfree(rp);
  +}
  +
  +void ref_proxy_struct_free(struct ref_proxy_struct *rp)
  +{
  +       rcu_assign_pointer(rp->ref, NULL);
  +       synchronize_srcu(&rp->srcu);
  +       kref_put(&rp->kref, ref_proxy_struct_release);
  +}
  +EXPORT_SYMBOL(ref_proxy_struct_free);

The struct ref_proxy is for resource consumers.  It holds a reference to
the ref_proxy_struct during its lifetime.

  +struct ref_proxy {
  +       struct ref_proxy_struct *rp;
  +       int idx;
  +};
  +
  +struct ref_proxy *ref_proxy_alloc(struct ref_proxy_struct *rp)
  +{
  +       struct ref_proxy *proxy;
  +
  +       proxy = kzalloc(sizeof(*proxy), GFP_KERNEL);
  +       if (!proxy)
  +               return NULL;
  +
  +       proxy->rp = rp;
  +       kref_get(&rp->kref);
  +
  +       return proxy;
  +}
  +EXPORT_SYMBOL(ref_proxy_alloc);
  +
  +void ref_proxy_free(struct ref_proxy *proxy)
  +{
  +       struct ref_proxy_struct *rp = proxy->rp;
  +
  +       kref_put(&rp->kref, ref_proxy_struct_release);
  +       kfree(proxy);
  +}
  +EXPORT_SYMBOL(ref_proxy_free);

When a resource consumer needs to reference the resource, it gets the
pointer via ref_proxy_get() which entering a RCU critical section and
puts the pointer via ref_proxy_put() which leaving the critical section
afterward.

  +void *ref_proxy_get(struct ref_proxy *proxy)
  +{
  +       struct ref_proxy_struct *rp = proxy->rp;
  +       void *ref;
  +
  +       proxy->idx = srcu_read_lock(&rp->srcu);
  +
  +       ref = rcu_dereference(rp->ref);
  +       if (!ref)
  +               srcu_read_unlock(&rp->srcu, proxy->idx);
  +
  +       return ref;
  +}
  +EXPORT_SYMBOL(ref_proxy_get);
  +
  +void ref_proxy_put(struct ref_proxy *proxy)
  +{
  +       struct ref_proxy_struct *rp = proxy->rp;
  +
  +       srcu_read_unlock(&rp->srcu, proxy->idx);
  +}
  +EXPORT_SYMBOL(ref_proxy_put);

The resource provider (e.g. cros_ec_spi) should provide a ref_proxy on
probing and free the ref_proxy on removing.

  @@ -770,0 +772,4 @@ static int cros_ec_spi_probe(struct spi_device *spi)
  +       ec_dev->ref_proxy = ref_proxy_struct_alloc(ec_dev);
  +       if (!ec_dev->ref_proxy)
  +               return -ENOMEM;
  +

   static void cros_ec_spi_remove(struct spi_device *spi)
   {
          struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
  
  +       ref_proxy_struct_free(ec_dev->ref_proxy);
          cros_ec_unregister(ec_dev);
   }

The resource consumer (e.g. cros_ec_chardev) should create an instance of
ref_proxy and tie to its local lifecycle.  It should get the pointer before
accessing.  It indicates the resource is no longer available if the
returning pointer is NULL.

  @@ -166,7 +187,10 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
          if (!priv)
                  return -ENOMEM;
  
  -       priv->ec_dev = ec_dev;
  +       priv->ec_dev_proxy = ref_proxy_alloc(ec_dev->ref_proxy);
  +       if (!priv->ec_dev_proxy)
  +               return -ENOMEM;
  +
          priv->cmd_offset = ec->cmd_offset;
          filp->private_data = priv;
          INIT_LIST_HEAD(&priv->events);

  @@ -251,11 +276,16 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
   static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
   {
          struct chardev_priv *priv = filp->private_data;
  -       struct cros_ec_device *ec_dev = priv->ec_dev;
  +       struct cros_ec_device *ec_dev;
          struct ec_event *event, *e;
  
  -       blocking_notifier_chain_unregister(&ec_dev->event_notifier,
  -                                          &priv->notifier);
  +       ec_dev = ref_proxy_get(priv->ec_dev_proxy);
  +       if (ec_dev) {
  +               blocking_notifier_chain_unregister(&ec_dev->event_notifier,
  +                                                  &priv->notifier);
  +               ref_proxy_put(priv->ec_dev_proxy);
  +       }
  +       ref_proxy_free(priv->ec_dev_proxy);

  @@ -64,7 +66,11 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen)
          msg->command = EC_CMD_GET_VERSION + priv->cmd_offset;
          msg->insize = sizeof(*resp);

  -       ret = cros_ec_cmd_xfer_status(priv->ec_dev, msg);
  +       ec_dev = ref_proxy_get(priv->ec_dev_proxy);
  +       if (!ec_dev)
  +               return -ENODEV;
  +       ret = cros_ec_cmd_xfer_status(ec_dev, msg);
  +       ref_proxy_put(priv->ec_dev_proxy);
          if (ret < 0) {
                  snprintf(str, maxlen,
                           "Unknown EC version, returned error: %d\n",

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Greg KH]

On Fri, Aug 01, 2025 at 07:25:55AM +0000, Tzung-Bi Shih wrote:
> On Fri, Jul 25, 2025 at 06:58:23AM +0200, Greg KH wrote:
> > On Thu, Jul 24, 2025 at 01:32:01PM +0000, Tzung-Bi Shih wrote:
> > > On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> > > > NEVER attempt to increment/decrement refcounts on open/release.  That
> > > > way lies madness and should not be needed at all, the underlying
> > > > infrastructure should keep things working properly here, right?
> > > [...]
> > You are attempting to have one reference count for one object that has
> > different lifecycles.  That doesn't work well, if at all.  See the
> > numerous talks at the Linux Plumbers conference over the past few years
> > about why this is a "bad idea" and for ideas on how to redesign it to
> > fix the issue.
> > 
> > Hint, 2 objects, different reference counts :)
> 
> Referenced the talk [1] and its related works and hope I understand it
> correctly.
> 
> [1] https://lpc.events/event/17/contributions/1627/
> 
> Summarize the proposed fix and I hope it makes sense:
> 
> Introduce a new helper: ref_proxy for managing "weak" references to the
> object that may be gone at any time.  `ref` is the target pointer (i.e.
> the resource) which could be released asynchronously (from the resource
> consumers' point of view).  The struct ref_proxy_struct is for resource
> providers and is only freed on dropping the final reference.

<snip>

Nice!  At a quick glance, this does look like what we need (and if it
helps, matches what was just done in the Rust kernel apis, right?)

Want to turn this into something real and submit it for review after you
test it out with your driver to see if it does work?

thanks,

greg k-h

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih]

On Fri, Aug 01, 2025 at 10:22:18AM +0200, Greg KH wrote:
> On Fri, Aug 01, 2025 at 07:25:55AM +0000, Tzung-Bi Shih wrote:
> > On Fri, Jul 25, 2025 at 06:58:23AM +0200, Greg KH wrote:
> > > On Thu, Jul 24, 2025 at 01:32:01PM +0000, Tzung-Bi Shih wrote:
> > > > On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> > > > > NEVER attempt to increment/decrement refcounts on open/release.  That
> > > > > way lies madness and should not be needed at all, the underlying
> > > > > infrastructure should keep things working properly here, right?
> > > > [...]
> > > You are attempting to have one reference count for one object that has
> > > different lifecycles.  That doesn't work well, if at all.  See the
> > > numerous talks at the Linux Plumbers conference over the past few years
> > > about why this is a "bad idea" and for ideas on how to redesign it to
> > > fix the issue.
> > > 
> > > Hint, 2 objects, different reference counts :)
> > 
> > Referenced the talk [1] and its related works and hope I understand it
> > correctly.
> > 
> > [1] https://lpc.events/event/17/contributions/1627/
> > 
> > Summarize the proposed fix and I hope it makes sense:
> > 
> > Introduce a new helper: ref_proxy for managing "weak" references to the
> > object that may be gone at any time.  `ref` is the target pointer (i.e.
> > the resource) which could be released asynchronously (from the resource
> > consumers' point of view).  The struct ref_proxy_struct is for resource
> > providers and is only freed on dropping the final reference.
> 
> <snip>
> 
> Nice!  At a quick glance, this does look like what we need (and if it
> helps, matches what was just done in the Rust kernel apis, right?)

Thanks for the review.  I'd need to check.  I wasn't aware of what was just
done in the Rust kernel APIs.

> Want to turn this into something real and submit it for review after you
> test it out with your driver to see if it does work?

Have tested on the draft version and it works.  Will consolidate the patches
and send them for further reviews.

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Greg KH]

On Fri, Aug 01, 2025 at 08:41:32AM +0000, Tzung-Bi Shih wrote:
> On Fri, Aug 01, 2025 at 10:22:18AM +0200, Greg KH wrote:
> > On Fri, Aug 01, 2025 at 07:25:55AM +0000, Tzung-Bi Shih wrote:
> > > On Fri, Jul 25, 2025 at 06:58:23AM +0200, Greg KH wrote:
> > > > On Thu, Jul 24, 2025 at 01:32:01PM +0000, Tzung-Bi Shih wrote:
> > > > > On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> > > > > > NEVER attempt to increment/decrement refcounts on open/release.  That
> > > > > > way lies madness and should not be needed at all, the underlying
> > > > > > infrastructure should keep things working properly here, right?
> > > > > [...]
> > > > You are attempting to have one reference count for one object that has
> > > > different lifecycles.  That doesn't work well, if at all.  See the
> > > > numerous talks at the Linux Plumbers conference over the past few years
> > > > about why this is a "bad idea" and for ideas on how to redesign it to
> > > > fix the issue.
> > > > 
> > > > Hint, 2 objects, different reference counts :)
> > > 
> > > Referenced the talk [1] and its related works and hope I understand it
> > > correctly.
> > > 
> > > [1] https://lpc.events/event/17/contributions/1627/
> > > 
> > > Summarize the proposed fix and I hope it makes sense:
> > > 
> > > Introduce a new helper: ref_proxy for managing "weak" references to the
> > > object that may be gone at any time.  `ref` is the target pointer (i.e.
> > > the resource) which could be released asynchronously (from the resource
> > > consumers' point of view).  The struct ref_proxy_struct is for resource
> > > providers and is only freed on dropping the final reference.
> > 
> > <snip>
> > 
> > Nice!  At a quick glance, this does look like what we need (and if it
> > helps, matches what was just done in the Rust kernel apis, right?)
> 
> Thanks for the review.  I'd need to check.  I wasn't aware of what was just
> done in the Rust kernel APIs.

Take a look at that, if there's any way it can align up with what the
rust code does, if possible, that would be good as we all are going to
be crossing between the two different languages a lot :)

> > Want to turn this into something real and submit it for review after you
> > test it out with your driver to see if it does work?
> 
> Have tested on the draft version and it works.  Will consolidate the patches
> and send them for further reviews.

Great!  No rush, we can't do anything until after -rc1 is out.

thanks,

greg k-h

Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

[Tzung-Bi Shih]

On Fri, Aug 01, 2025 at 09:50:01AM +0100, Greg KH wrote:
> On Fri, Aug 01, 2025 at 08:41:32AM +0000, Tzung-Bi Shih wrote:
> > On Fri, Aug 01, 2025 at 10:22:18AM +0200, Greg KH wrote:
> > > On Fri, Aug 01, 2025 at 07:25:55AM +0000, Tzung-Bi Shih wrote:
> > > > On Fri, Jul 25, 2025 at 06:58:23AM +0200, Greg KH wrote:
<snip>
> > > 
> > > Nice!  At a quick glance, this does look like what we need (and if it
> > > helps, matches what was just done in the Rust kernel apis, right?)
> > 
> > Thanks for the review.  I'd need to check.  I wasn't aware of what was just
> > done in the Rust kernel APIs.
> 
> Take a look at that, if there's any way it can align up with what the
> rust code does, if possible, that would be good as we all are going to
> be crossing between the two different languages a lot :)

I'd still need some time to find and look at the relevant Rust code (language
and overall status of Rust kernel API-wise).  I guess they should spread in
rust/kernel/device.rs and rust/kernel/devres.rs?

> 
> > > Want to turn this into something real and submit it for review after you
> > > test it out with your driver to see if it does work?
> > 
> > Have tested on the draft version and it works.  Will consolidate the patches
> > and send them for further reviews.
> 
> Great!  No rush, we can't do anything until after -rc1 is out.

While I'm still finding and looking at the Rust part, sent a version [1] for
review first.

[1] https://lore.kernel.org/chrome-platform/20250814091020.1302888-1-tzungbi@kernel.org/

[PATCH v3 6/8] platform/chrome: Don't initialize common utilities when registering

[Tzung-Bi Shih]

Lifecycle of common utilities in struct cros_ec_device should be the
same as struct cros_ec_device.  It is possible that a struct
cros_ec_device is unregistered however the utilities are still using.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
New patch in the v3 series.

 drivers/platform/chrome/cros_ec.c       | 10 ----------
 drivers/platform/chrome/cros_ec_proto.c | 16 ++++++++++++----
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 9604424b6e7a..7e7930519947 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -185,9 +185,6 @@ 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;
@@ -204,9 +201,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 		return -ENOMEM;
 
 	atomic_set(&ec_dev->registered, 1);
-	lockdep_register_key(&ec_dev->lockdep_key);
-	mutex_init(&ec_dev->lock);
-	lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
 
 	/* Send RWSIG continue to jump to RW for devices using RWSIG. */
 	err = cros_ec_rwsig_continue(ec_dev);
@@ -307,8 +301,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 	atomic_set(&ec_dev->registered, 0);
 	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);
@@ -330,8 +322,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);
 	cros_ec_device_put(ec_dev);
 }
 EXPORT_SYMBOL(cros_ec_unregister);
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 7fac4d7c1b78..f747068a36cd 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -1168,10 +1168,16 @@ struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
 {
 	struct cros_ec_device *ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
 
-	if (ec_dev) {
-		ec_dev->dev = dev;
-		kref_init(&ec_dev->kref);
-	}
+	if (!ec_dev)
+		return NULL;
+
+	ec_dev->dev = dev;
+	kref_init(&ec_dev->kref);
+	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);
 
 	return ec_dev;
 }
@@ -1191,6 +1197,8 @@ EXPORT_SYMBOL_GPL(cros_ec_device_get);
 static void cros_ec_device_release(struct kref *kref)
 {
 	/* Do not free the cros_ec_device as it is still dev-managed. */
+	mutex_destroy(&ec_dev->lock);
+	lockdep_unregister_key(&ec_dev->lockdep_key);
 }
 
 /**
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v3 7/8] platform/chrome: cros_ec_chardev: Hold refcount of struct cros_ec_device

[Tzung-Bi Shih]

Hold refcount of struct cros_ec_device to prevent the memory from
releasing when the fops still depend on it.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
No changes since v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-6-tzungbi@kernel.org/).

 drivers/platform/chrome/cros_ec_chardev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c9d80ad5b57e..1cb09ca3b850 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -181,6 +181,7 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
 		kfree(priv);
 	}
 
+	cros_ec_device_get(ec_dev);
 	return ret;
 }
 
@@ -256,6 +257,7 @@ static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
 
 	blocking_notifier_chain_unregister(&ec_dev->event_notifier,
 					   &priv->notifier);
+	cros_ec_device_put(ec_dev);
 
 	list_for_each_entry_safe(event, e, &priv->events, node) {
 		list_del(&event->node);
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v3 8/8] platform/chrome: Manage struct cros_ec_device lifecycle by its refcount

[Tzung-Bi Shih]

Stop delegating the memory to dev-managed.  Use its own refcount for
managing the lifecycle.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
No changes since v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-7-tzungbi@kernel.org/).

 drivers/platform/chrome/cros_ec_proto.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index f747068a36cd..06ccb07fad5f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -1166,7 +1166,7 @@ EXPORT_SYMBOL_GPL(cros_ec_get_cmd_versions);
  */
 struct cros_ec_device *cros_ec_device_alloc(struct device *dev)
 {
-	struct cros_ec_device *ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+	struct cros_ec_device *ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
 
 	if (!ec_dev)
 		return NULL;
@@ -1196,9 +1196,11 @@ EXPORT_SYMBOL_GPL(cros_ec_device_get);
 
 static void cros_ec_device_release(struct kref *kref)
 {
-	/* Do not free the cros_ec_device as it is still dev-managed. */
+	struct cros_ec_device *ec_dev = container_of(kref, struct cros_ec_device, kref);
+
 	mutex_destroy(&ec_dev->lock);
 	lockdep_unregister_key(&ec_dev->lockdep_key);
+	kfree(ec_dev);
 }
 
 /**
-- 
2.50.0.727.gbf7dc18ff4-goog