Re: [PATCH 0/3] Revert "revocable: Revocable resource management"
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: 2026-01-28 23:28:58
Also in:
linux-kselftest, lkml
On Wed, Jan 28, 2026 at 03:23:10PM +0100, Johan Hovold wrote:
On Tue, Jan 27, 2026 at 03:57:44PM +0000, Tzung-Bi Shih wrote:quoted
On Sat, Jan 24, 2026 at 06:05:32PM +0100, Johan Hovold wrote:quoted
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 postedquoted
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.Yeah, fixing it at the chardev (or miscdevice) level would still require some changes at the driver level (e.g. to wakeup sleeping tasks).
Waking up sleeping tasks is a core part of the fix. Drivers will need to do so explicitly in their .remove() operation, through a subsystem-specific wrapper around a cdev helper function.
quoted
Given the drawbacks of [2], I believe cros_ec_chardev remains a valid use case for revocable.Yes, possibly. Miscdevice is a bit of a special case however since you cannot tie the lifetime of your driver data to that of the miscdev, but there are ways to address that. And some drivers already handle this (i.e. without any changes to miscdevice). But notably the revocable design seems to derive from this quirk of miscdevice. [ Side note for completeness: Looking at the cros_ec driver it doesn't seem to involve any hotpluggable buses so this looks like a mostly theoretical issue of a developer with root access actively unbinding an ec-driver while in use. ]quoted
quoted
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/ (local)quoted
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.Yes, that's exactly why we always require a user *before* merging something like this. To be able to determine that the interface is sane. Now the user showed up on the same day as the merge, which is obviously not enough time for proper review and discussion. [ And by short-circuiting the normal process you probably reduce the motivation for people to spend time on the example conversion too. ]quoted
quoted
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.But we need to look at that before throwing up our hands and saying that it's not possible and that each driver should take care of this itself.
There has been previous attempts, including a functional patch series, that didn't get merge due to the sole reason that it duplicated a small amount of logic also present in debugfs. I'll reply to Danilo somewhere else in this mail thread with more information.
quoted
quoted
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.To the contrary, now is right time to do the revert as there are fundamental problems with the current interface that will require a redesign. That's not the kind of work that should be done during an rc stabilisation cycle. Give people a chance to see for themselves what the gpiolib conversion looks like, determine whether this abstraction makes the code more or less readable, and think about possible further uses of a mechanism like this.
fully agreed. Follow-up fixes is not the right way forward. As Johan said, the quick merge despite the open issues, circumventing the normal review process, damages trust, and reduces my motivation to review your work and help shaping a good API to solve this problem.
quoted
-- Regards, Laurent Pinchart