On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
I fully agree with that, in C there is indeed no value of a revocable type when subsystems can guarantee "sane unregistration semantics".
Indeed, I agree with your message. If I look at the ec_cros presented here, there is no reason for any revocable. In fact it already seems like an abuse of the idea.
The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev" that is alreay properly lifetime controlled - the platform is already removed during the remove of the cros_ec.c.
So, there is no need for a revocable that spans two drivers like this!
The bug is that cros_ec_chardev.c doesn't implement itself correctly and doesn't have a well designed remove() for something that creates a char dev. This issue should be entirely handled within cros_ec_chardev.c and not leak out to other files.
1) Using a miscdevice means loosing out on any refcount managed memory. When using a file you need some per-device memory that lives for as long as all files are open. So don't use miscdev, the better pattern is:
struct chardev_data { struct device dev; struct cdev cdev;
Now you get to have a struct device linked refcount and a free function. The file can savely hold onto a chardev_data for its lifetime.
2) Add locking so the file can exist after the driver is removed:
struct chardev_data { [..] struct rw_semaphore sem; struct cros_ec_dev *ec_dev; };
Refcount the chardev_data::dev in the file operations open/release, refer to it via the private_data.
3) At the start of every fop take the sem and check the ec_dev:
ACQUIRE(rwsem_read, ecdev_sem)(&data->sem); if (ACQUIRE_ERR(ecdev_sem) || !data->ec_dev) return -ENODEV;
4) After unregistering the cdev, but before destroying the device take the write side of the rwsem and NULL ec_dev.
5) Purge all the devm usage from cros_ec_chardev, the only allocation is refcounted instead.
Simple. No need for any synchronize_srcu() for such a simple non-performance oriented driver.
Yes, this can be made better, there is a bit too much boilerplate, but revocable is not the way for cros_ec.
Jason