Laurent Pinchart wrote:

On Fri, Sep 12, 2025 at 06:22:48PM +0200, Danilo Krummrich wrote:

quoted

On 9/12/25 4:54 PM, Laurent Pinchart wrote:

quoted

On Fri, Sep 12, 2025 at 04:44:56PM +0200, Bartosz Golaszewski wrote:

quoted

On Fri, Sep 12, 2025 at 4:40 PM Greg Kroah-Hartman wrote:

quoted

Either way, I think this patch series stands on its own, it doesn't
require cdev to implement it, drivers can use it to wrap a cdev if they
want to.  We have other structures that want to do this type of thing
today as is proof with the rust implementation for the devm api.

Yeah, I'm not against this going upstream. If more development is
needed for this to be usable in other parts of the kernel, that can be
done gradually. Literally no subsystem ever was perfect on day 1.

To be clear, I'm not against the API being merged for the use cases that
would benefit from it, but I don't want to see drivers using it to
protect from the cdev/unregistration race.

I mean, revocable is really a synchronization primitive in the end that
"revokes" access to some resource in a race free way.

So, technically, it probably belongs into lib/.

I think the reason it ended up in drivers/base/ is that one common use case is
to revoke a device resource from a driver when the device is unbound from this
driver; or in other words devres is an obvious user.

So, I think that any other API (cdev, devres, etc.) should  be built on top of it.

No issue with that. I'm sure there are people who have better knowledge
than me when it comes to implementing the low-level primitive in the
most efficient way. What I have lots of experience with is the impact of
API design on drivers, and the API misuse (including through cargo-cult
programming) this can generate. Let's design the API towards drivers
correctly.

Note that I dropped the "managed_fops" [1] effort, targeted for use for
CXL, in favor of simply this in the CXL ioctl device shutdown path:

        cdev_device_del(&cxlmd->cdev, &cxlmd->dev);
        scoped_guard(rwsem_write, &cxl_memdev_rwsem)
		cxlmd->cxlds = NULL;
        put_device(&cxlmd->dev);

Pair that with:

        guard(rwsem_read)(&cxl_memdev_rwsem);
        cxlds = cxlmd->cxlds;
        if (cxlds)
                return __cxl_memdev_ioctl(cxlmd, cmd, arg);
	return -ENXIO;

...on the ioctl invocation side.

This "revocable" mechanism looks useful for other inter-driver resource
sharing, but not for the well known issues with cdev. For cdev and the
design pattern of "shutdown the ioctl path on a core-subsystem device
object that is also a chardev", just use cdev_device_add() with an
rwsem.

[1]: http://lore.kernel.org/all/CAPcyv4h74NjqcuUjv4zFKHAxio_bV0bngLoxP=ACw=JvMfq-UA@mail.gmail.com (local)