Message-ID: <aXjgeNY-jf9rIw09@google.com>
Date: Tue, 27 Jan 2026 15:57:44 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Johan Hovold <johan@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>,
	Linus Walleij <linusw@...nel.org>, Jonathan Corbet <corbet@....net>,
	Shuah Khan <shuah@...nel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Simona Vetter <simona.vetter@...ll.ch>,
	Dan Williams <dan.j.williams@...el.com>,
	Jason Gunthorpe <jgg@...dia.com>, linux-doc@...r.kernel.org,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] Revert "revocable: Revocable resource management"

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/20260121040204.2699886-1-tzungbi@kernel.org/

> 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/aXT45B6vLf9R3Pbf@hovoldconsulting.com/

---
[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;

Powered by blists - more mailing lists