On Sat, Jan 24, 2026 at 06:05:32PM +0100, Johan Hovold wrote:
> I was surprised to learn that the revocable functionality was merged last week
> given the community feedback on list and at LPC, but not least since there are
> no users of it, which we are supposed to require to be able to evaluate it
> properly.
>
> The chromeos ec driver issue which motivated this work turned out not to need
> it as was found during review. And the example gpiolib conversion was posted
Thanks for sharing your thoughts on revocable.
Regarding cros_ec_chardev, my last attempt [2] to solve its hot-plug issue by
synchronizing misc_{de,}register() with file operations using a global lock
highlighted the difficulty of alternatives: that approach serialized all file
operations and could easily lead to hung tasks if any file operation slept.
Given the drawbacks of [2], I believe cros_ec_chardev remains a valid use
case for revocable.
> the very same morning that this was merged which hardly provides enough time
> for evaluation (even if Bartosz quickly reported a performance regression).
The gpiolib conversion was provided as the first concrete user to enable
this evaluation process. The performance regression Bartosz identified is
valuable feedback, and I believe it is addressed by [3]. I plan to send the
next version of the series after v7.0-rc1 and revisit the issue.
[3] https://lore.kernel.org/all/[email protected]/
> Turns out there are correctness issues with both the gpiolib conversion and
> the revocable design itself that can lead to use-after-free and hung tasks
> (see
> [1] and patch 3/3).
I appreciate you identifying the issues where multiple threads share a file
descriptor; this is a case I overlooked. I see these kinds of findings as a
positive outcome of having wider review and a concrete user, allowing us to
identify and fix issues in the design.
> And as was pointed out repeatedly during review, and again at the day of the
> merge, this does not look like the right interface for the chardev unplug
> issue.
My focus has been on miscdevice [2], but I suspect cdev solutions for device
hot-plug would face similar synchronization challenges between device removal
and in-flight file operations.
> Revert the revocable implementation until a redesign has been proposed and
> evaluated properly.
I'll work on addressing the discovered issues and send follow-up fixes. I
believe keeping the current series in linux-next would be beneficial, as it
allows for easier testing and wider evaluation by others, rather than
reverting at this stage.
>
> Johan
>
>
> [1] https://lore.kernel.org/all/[email protected]/
---
[2]:
diff --git a/drivers/char/misc.c b/drivers/char/misc.c
...
+static struct miscdevice *find_miscdevice(int minor)
+{
+ struct miscdevice *c;
+
+ list_for_each_entry(c, &misc_list, list)
+ if (c->minor == minor)
+ return c;
+ return NULL;
+}
+
+static __poll_t misc_sync_poll(struct file *filp, poll_table *wait)
+{
+ struct miscdevice *c;
+
+ guard(mutex)(&misc_mtx);
+ c = find_miscdevice(iminor(filp->f_inode));
+ if (!c)
+ return -ENODEV;
+ if (!c->fops->poll)
+ return 0;
+
+ return c->fops->poll(filp, wait);
+}
...
+static const struct file_operations misc_sync_fops = {
+ .poll = misc_sync_poll,
+ .read = misc_sync_read,
+ .unlocked_ioctl = misc_sync_ioctl,
+ .release = misc_sync_release,
+};
+
static int misc_open(struct inode *inode, struct file *file)
{
int minor = iminor(inode);
@@ -161,6 +237,7 @@ static int misc_open(struct inode *inode, struct file *file)
replace_fops(file, new_fops);
if (file->f_op->open)
err = file->f_op->open(inode, file);
+ file->f_op = &misc_sync_fops;
