Re: Is devm_* broken ?

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

On Wednesday 15 July 2015 18:20:02 Takashi Iwai wrote:
> On Wed, 15 Jul 2015 18:08:34 +0200, Laurent Pinchart wrote:
> > On Wednesday 15 July 2015 17:51:28 Takashi Iwai wrote:
> > > On Wed, 15 Jul 2015 00:34:53 +0200, Laurent Pinchart wrote:
> > > > Hello,
> > > > 
> > > > I came to realize not too long ago that the following sequence of
> > > > events will lead to a crash with any platform driver that uses devm_*
> > > > and creates device nodes.
> > > > 
> > > > 1. Get a platform device bound it its driver
> > > > 2. Open the corresponding device node in userspace and keep it open
> > > > 3. Unbind the platform device from its driver through sysfs
> > > > 
> > > > echo <device-name> > /sys/bus/platform/drivers/<driver-name>/unbind
> > > > 
> > > > (or for hotpluggable devices just unplug the device)
> > > > 
> > > > 4. Close the device node
> > > > 5. Enjoy the fireworks
> > > > 
> > > > While having a device node open prevents modules from being unloaded,
> > > > it doesn't prevent devices from being unbound from drivers. If the
> > > > driver uses devm_* helpers to allocate memory the memory will be freed
> > > > when the device is unbound from the driver, but that memory will still
> > > > be used by any operation touching an open device node.
> > > > 
> > > > Is devm_* inherently broken ? It's so widely used, tell me I'm missing
> > > > something obvious.
> > > 
> > > I don't think this is specific to devm_*() but it's about the resource
> > > management in general.  After bus or driver's remove callback, all
> > > device resources that have been assigned by the driver are supposed to
> > > be freed, or ready to be freed.
> > 
> > The remove callback notifies drivers that the device has been removed and
> > that it's time to clean up. However, drivers have no control over
> > userspace, so they can't force applications to close all open file
> > handles, unmap memory and otherwise free all device-related resources
> > immediately and synchronously. The best a driver can do is prevent any
> > new reference to a resource from being taken by userspace (returning an
> > error from open() for instance) and wait until all existing references
> > get released before finally freeing resources. This is where devm_* hurts
> > as a driver can't delay freeing resources until after all references held
> > by userspace are released.
> 
> Right, and this is what ALSA drivers does in general.

Does that mean that an ALSA driver that uses devm_* will crash if the device 
is unbound from the driver (possibly because it gets disconnected) while 
userspace uses the ALSA device ? Isn't that considered as an issue ?

> > If I were to switch the uvcvideo driver from kzalloc to devm_kzalloc it
> > would crash if the webcam gets disconnected while userspace has the V4L2
> > device node open.
> 
> The disconnection is a bit different story, but I see your concern.

>From a resources release point of view disconnection and unbind are similar.

-- 
Regards,

Laurent Pinchart