Re: cdev/devm_* issues (was Re: [TECH TOPIC] Rust for Linux)

[Dan Williams <dan.j.williams@intel.com>]

On Thu, Jul 8, 2021 at 4:07 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Thanks Laurent, Greg, this is important.
>
> Replying here to get this in the right thread.
>
> > On Wed, Jul 07, 2021 at 08:17:08PM +0300, Laurent Pinchart wrote:
>
> > >  Here's what
> > > I think should be done for drivers that expose character devices.
> > >
> > > - Drivers must stop allocating their internal data structure (the one
> > >   they set as device data with dev_set_drvdata()) with devm_kzalloc().
> > >   The data structure must instead be allocated with a plain kzalloc()
> > >   and reference-counted.
> > >
> > >   Most drivers will register a single character device using a
> > >   subsystem-specific API (e.g. video_register_device() in V4L2). The
> > >   subsystem needs to provide a .release() callback, called when the
> > >   last reference to the character device is released. Drivers must
> > >   implement this, and can simply free their internal data structure at
> > >   this point.
>
> What we did in GPIO to get around the whole issue is to split
> our device in two abstractions. The device that spawns a GPIO
> driver can be a GPIO on any bus (platform, PCI, SPI, I2C, ...)
> then that populates and registers one (or more) gpio_chips.
> That is a struct that deal with handling the hardware but does
> not contain any struct device.
>
> When registering gpio_chip we create a struct gpio_device which
> contains a struct device and a struct cdev (exposed to userspace)
> and all in-kernel consumer handles (called struct gpio_desc). This
> is reference counted on the struct device and uses
> ->release() to eventually clean itself up.
>
> The crucial part is what happens when a device with GPIOs
> disappears, if e.g. the USB device is unplugged or the driver
> is rmmod:ed by force from the command line. We then unregister
> the struct gpio_chip and drop all devm_*  resources taken by the
> driver (referencing the struct dev in the USB device or so) so these
> go away, but the struct gpio_device stays around
> until the last reference from userspace is dropped.
>
> In order to not crash calls from the character device the device is
> "numbed", so any calls will just return "OK" but nothing happens.
> We then hope userspace will be so nice to terminate once it realizes
> that it is no longer needed, closing the chardev and releasing the
> resources held.
>
> This works for us but I admit it is a bit kludgy. I guess it is not a
> generally useful practice, we just had to come up with something.

We do something similar in the CXL driver. The driver data is
allocated with devm. When ->remove() is triggered the driver data is
disconnected from the cdev (because it's about to be freed by devres)
and all future ioctls attempts fail until userspace finally gives up
and closes the device-file to release the cdev.

I clumsily attempted to solve this problem in a generic way here:

https://lore.kernel.org/r/CAPcyv4hEpdh_aGcs_73w5KmYWdvR29KB2M2-NNXsaXwxf35Hwg@mail.gmail.com

...Christoph rightly pointed out that this is something debugfs
already handles with its file_operations proxy implementation. I'm
thinking that concept can be extended for drivers to deploy their
ioctl implementation behind a proxy that handles the ioctl shutdown
synchronization in the core and not have every driver open code it.
See fs/debugfs/file.c::FULL_PROXY_FUNC().