On Thu, Jul 03, 2025 at 01:52:02PM +0200, Greg KH wrote:
On Thu, Jul 03, 2025 at 11:35:09AM +0000, Tzung-Bi Shih wrote: But yes, one that people have been talking about and discussing generic ways of solving for years now, you have seen the plumbers talks about it, right?
Will check them.
@@ -31,7 +34,14 @@ /* Arbitrary bounded size for the event queue */ #define CROS_MAX_EVENT_LEN PAGE_SIZE +/* This protects 'chardev_list' */ +static DEFINE_MUTEX(chardev_lock); +static LIST_HEAD(chardev_list);
Having a static list of chardevices feels very odd and driver-specific, right
The `chardev_list` is for recording all opened instances. Adding/removing entries in the .open()/.release() fops. The `chardev_lock` is for protecting from accessing the list simultaneously.
They are statically allocated because they can't follow the lifecycle of neither the platform_device (e.g. can be gone after unbinding the driver) nor the chardev (e.g. can be gone after closing the file).
Side note: realized an issue in current version: in the .remove() of platform_driver, it unconditionally resets `ec_dev` for all opened instances.
struct chardev_priv {
- struct list_head list;
- /* This protects 'ec_dev' */
- struct mutex lock;
Protects it from what?
You now have two locks in play, one for the structure itself, and one for the list. Yet how do they interact as the list is a list of the objects which have their own lock?
Are you _SURE_ you need two locks here? If so, you need to document these really well.
`struct chardev_priv` is bound to chardev's lifecycle. It is allocated in the .open() fops; and freed in the .release() fops. The `lock` is for protecting from accessing the `ec_dev` simultaneously (one is chardev itself, another one is the .remove() of platform_driver).
@@ -341,16 +364,20 @@ 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;
- scoped_guard(mutex, &priv->lock) {
if (!priv->ec_dev)return -ENODEV;- }
What prevents ec_dev from changing now, after you have just checked it? This feels very wrong as:
Ah, yes. I did it wrong. The `priv->lock` should be held at least for the following cros_ec_chardev_ioctl_xcmd() and cros_ec_chardev_ioctl_readmem().
- switch (cmd) { case CROS_EC_DEV_IOCXCMD:
return cros_ec_chardev_ioctl_xcmd(ec, (void __user *)arg);
return cros_ec_chardev_ioctl_xcmd(priv->ec_dev, (void __user *)arg);Look, it could have gone away here, right? If not, how?
Yes, I did it wrong. The lock should be still held for them.
@@ -394,8 +421,28 @@ static int cros_ec_chardev_probe(struct platform_device *pdev) static void cros_ec_chardev_remove(struct platform_device *pdev) { struct miscdevice *misc = dev_get_drvdata(&pdev->dev);
- struct chardev_priv *priv;
- /*
* Must deregister the misc device first so that the following* open fops get handled correctly.** misc device is serialized by `misc_mtx`.* 1) If misc_deregister() gets the lock earlier than misc_open(),* the open fops won't be called as the corresponding misc* device is already destroyed.* 2) If misc_open() gets the lock earlier than misc_deregister(),* the following code block resets the `ec_dev` to prevent* the rest of fops from accessing the obsolete `ec_dev`.What "following code block"? What will reset the structure?
+ scoped_guard(mutex, &chardev_lock) { + list_for_each_entry(priv, &chardev_list, list) { + scoped_guard(mutex, &priv->lock) + priv->ec_dev = NULL; + } + }