Re: [TECH TOPIC] Rust for Linux

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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