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
