Thread (70 messages) 70 messages, 12 authors, 2026-02-04

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 posted
quoted
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
quoted
[1] https://lore.kernel.org/all/aXT45B6vLf9R3Pbf@hovoldconsulting.com/ (local)
-- 
Regards,

Laurent Pinchart
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help