Re: [TECH TOPIC] Rust for Linux

Laurent Pinchart
2021-07-10T16:43:08Z

On Thu, Jul 08, 2021 at 11:08:52AM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 7 Jul 2021 20:17:08 +0300 Laurent Pinchart escreveu:
> > On Wed, Jul 07, 2021 at 02:45:04PM +0200, Greg KH wrote:
> > > On Wed, Jul 07, 2021 at 01:38:44PM +0100, James Bottomley wrote:
> > > > On Wed, 2021-07-07 at 14:20 +0200, Greg KH wrote:
> > > > > On Wed, Jul 07, 2021 at 12:34:31PM +0100, James Bottomley wrote:
> > > > > > On Wed, 2021-07-07 at 11:50 +0100, Mark Brown wrote:
> > > > > > > On Tue, Jul 06, 2021 at 10:36:14PM +0200, Linus Walleij wrote:
> > > > > > > > On Tue, Jul 6, 2021 at 10:00 PM Roland Dreier wrote:
> > > > > > > > > "devres" / devm_xxx was an attempt to deal with this in C,
> > > > > > > > > but it only solves some cases of this and has not received a
> > > > > > > > > lot of adoption (we can argue about the reasons).
> > > > > > > >
> > > > > > > > Really? From my point of view that is adopted all over the map.
> > > > > > > > I add new users all the time and use it as much as I can when
> > > > > > > > writing new drivers.
> > > > > > >
> > > > > > > Yes, it's *super* widely used in most of the kernel. Perhaps
> > > > > > > there's some subsystems that reject it for some reason.
> > > > > > >
> > > > > > > > I think it's a formidable success, people just need to learn to
> > > > > > > > do it more.
> > > > > > >
> > > > > > > There *are* issues with people adopting it too enthusiastically -
> > > > > > > as well as the memory lifetime issues that Laurent mentioned it's
> > > > > > > easy for it to cause problems with interrupt handlers that are
> > > > > > > left live longer than they should be and try to use things that
> > > > > > > were already deallocated.
> >
> > (CC'ing Daniel Vetter as the author of the DRM devres-based resource
> > management)
> >
> > I've given this lots of thoughts lately, in the context of V4L2, but it
> > should be roughly the same for character devices in general. 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.
> >
> > For drivers that register multiple character devices, or in general
> > expose multiple interfaces to userspace or other parts of the kernel,
> > the internal data structure must be properly reference-counted, with a
> > reference released in each .release() callback. There may be ways to
> > simplify this.
>
> Good point. Yeah, indeed some work seems to be required on that area.
>
> Yet, V4L2 is somewhat different here, as most (if not all) devices
> expose multiple cdevs.
>
> Also, in the case of V4L2 USB and PCI devices, their "dev->parent" is
> usually a PCI bridge, or an USB device with multiple functions on it,
> as those hardware contain both audio and video and sometimes input
> (either buttons or remote controllers), typically using different
> drivers. So, when the hardware is hot-unplugged or unbind, several
> drivers and multiple cdevs will be released. Ensuring that those will
> happen at the right time can be a challenge, specially if there are
> pending syscalls and/or threads by the time the device is unbound.
>
> The DRM subsystem likely fits on the same case, as drivers also
> usually create multiple cdevs, and there are DMABUF objects shared
> between different struct devices.

The main difference is that DRM/KMS handles this inside the subsystem
core, instead of leaving it up to the drivers as in V4L2. We have to
live with historical mistakes, but I wish they could retire.

> So, yeah, using devm_* for V4L2 and DRM can indeed bring troubles.
> I can't see a solution there currently but to avoid using devm*,
> handling it using an approach similar to the one you described.

I want to emphasize that it's not all devm_* calls that are problematic.
The ioremap family of devm functions are perfectly fine, as memory
mapped I/O must not be accessed after a driver's .remove() function
returns.

> -
>
> I'm not so sure if using devm_* is a problem on several cases, though.
> I mean, when the hardware is not hot-pluggable, the data lifetime
> is a lot simpler.

You can unbind a device from its driver through sysfs, and that brings
the exact same issues as hardware unplug.

> So, for instance, a regulator driver probably can use devm_* without
> any issues, as it doesn't seem to make much sense to unbind a regulator
> once the device was probed. On drivers like that, not using devm_*
> would just make the probing part of the driver more complex, without
> bringing any real benefit.

Don't forget that regulators, GPIO controllers and other such core
resources can be part of unpluggable devices.

--
Regards,

Laurent Pinchart