Message-ID: <20260128232854.GB3275574@killaraus>
Date: Thu, 29 Jan 2026 01:28:54 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Johan Hovold <johan@...nel.org>
Cc: Tzung-Bi Shih <tzungbi@...nel.org>,
	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>,
	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 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:
> > 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
> 
> > 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.

> > 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. ]
> 
> > > 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.
> 
> 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. ]
> 
> > > 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.

> > > 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.

> > > [1] https://lore.kernel.org/all/aXT45B6vLf9R3Pbf@hovoldconsulting.com/

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists