public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Is devm_* broken ?
@ 2015-07-14 22:34 Laurent Pinchart
  2015-07-15 15:51 ` Takashi Iwai
  2015-07-15 17:00 ` Dan Williams
  0 siblings, 2 replies; 14+ messages in thread
From: Laurent Pinchart @ 2015-07-14 22:34 UTC (permalink / raw)
  To: linux-kernel

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.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-14 22:34 Is devm_* broken ? Laurent Pinchart
@ 2015-07-15 15:51 ` Takashi Iwai
  2015-07-15 16:08   ` Laurent Pinchart
  2015-07-15 17:00 ` Dan Williams
  1 sibling, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2015-07-15 15:51 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-kernel

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.


Takashi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 15:51 ` Takashi Iwai
@ 2015-07-15 16:08   ` Laurent Pinchart
  2015-07-15 16:20     ` Takashi Iwai
  0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-07-15 16:08 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-kernel

Hello Takashi,

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.

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.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 16:08   ` Laurent Pinchart
@ 2015-07-15 16:20     ` Takashi Iwai
  2015-07-15 16:27       ` Laurent Pinchart
  0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2015-07-15 16:20 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-kernel

On Wed, 15 Jul 2015 18:08:34 +0200,
Laurent Pinchart wrote:
> 
> Hello Takashi,
> 
> 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.

> 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.


Takashi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 16:20     ` Takashi Iwai
@ 2015-07-15 16:27       ` Laurent Pinchart
  2015-07-15 16:34         ` Takashi Iwai
  0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-07-15 16:27 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-kernel

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 16:27       ` Laurent Pinchart
@ 2015-07-15 16:34         ` Takashi Iwai
  2015-07-28 14:10           ` Laurent Pinchart
  0 siblings, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2015-07-15 16:34 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-kernel

On Wed, 15 Jul 2015 18:27:42 +0200,
Laurent Pinchart wrote:
> 
> 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 ?

No, usually the driver calls snd_card_remove() and it blocks until all
resources are closed/freed, thus devm_*() (that will be released after
that) won't crash.  For the disconnection, there is an asynchronous
version, and the rest is managed at the last close.

> > > 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.

Similar but slightly different, IIRC.  The disconnect (in USB) is a
part of unbind, but not vice versa.


Takashi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-14 22:34 Is devm_* broken ? Laurent Pinchart
  2015-07-15 15:51 ` Takashi Iwai
@ 2015-07-15 17:00 ` Dan Williams
  2015-07-15 18:03   ` Tejun Heo
  1 sibling, 1 reply; 14+ messages in thread
From: Dan Williams @ 2015-07-15 17:00 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Linux Kernel Mailing List, Tejun Heo

[ adding Tejun ]

On Tue, Jul 14, 2015 at 3:34 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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.

Sounds like a real problem.  The drivers I've used devm with have an
upper layer that prevents this crash, but that's not much consolation.
I think adding lifetime to devm allocations would be useful that way
->probe() and open() can do a devres_get() while ->remove() and
close() can do a devres_put().  Perhaps I'm also missing something
obvious though...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 17:00 ` Dan Williams
@ 2015-07-15 18:03   ` Tejun Heo
  2015-07-28 14:16     ` Laurent Pinchart
  0 siblings, 1 reply; 14+ messages in thread
From: Tejun Heo @ 2015-07-15 18:03 UTC (permalink / raw)
  To: Dan Williams; +Cc: Laurent Pinchart, Linux Kernel Mailing List

Hello,

On Wed, Jul 15, 2015 at 10:00:54AM -0700, Dan Williams wrote:
> Sounds like a real problem.  The drivers I've used devm with have an
> upper layer that prevents this crash, but that's not much consolation.
> I think adding lifetime to devm allocations would be useful that way
> ->probe() and open() can do a devres_get() while ->remove() and
> close() can do a devres_put().  Perhaps I'm also missing something
> obvious though...

Hmmm... so this really is a general lifetime management problem and
also why sysfs implements revoke semantics.  As memory allocated by
devm_kmalloc() isn't tied to any specific hardware, it seems a bit
murky here but if you consider any other resources, this is clear - a
driver must not access any resources once detach is complete.  These
aren't resources which can be detached and then held while draining
existing userland references.  They immediately conflict with the next
driver which is gonna attach to the device.

A driver should isolate and drain on-going accesses from userland
before finishing detaching one way or another.  No resources attached
to the hardware side can't be held once detaching is complete.  If a
piece of memory isn't attached to the harware side but the userland
interface side which gets isolated and drained after detachment, that
shouldn't be allocated via devm - it has "dev" in its name for a
reason.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 16:34         ` Takashi Iwai
@ 2015-07-28 14:10           ` Laurent Pinchart
  0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2015-07-28 14:10 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-kernel

Hello Takashi-san,

On Wednesday 15 July 2015 18:34:13 Takashi Iwai wrote:
> On Wed, 15 Jul 2015 18:27:42 +0200, Laurent Pinchart wrote:
> > 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
> > ?
>
> No, usually the driver calls snd_card_remove() and it blocks until all
> resources are closed/freed, thus devm_*() (that will be released after
> that) won't crash.  For the disconnection, there is an asynchronous
> version, and the rest is managed at the last close.

Most frameworks clean up at last close time as well, at least when the drivers 
cooperate. My point was that once the .disconnect() handler returns the devm_* 
allocate resources will be freed automatically, which can happen long before 
last close.

> >>> 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.
>
> Similar but slightly different, IIRC.  The disconnect (in USB) is a
> part of unbind, but not vice versa.

The USB interface disconnect operation is called from usb_unbind_interface() 
and the USB device disconnect operation from usb_unbind_device(), respectively 
handling the USB interface driver or the USB device driver .remove() operation 
(usb_unbind_interface() is also called from usb_driver_release_interface()). 
Both .remove() operations are called by the driver core, either from an unbind 
request through sysfs or from a device disconnection, going through 
usb_disconnect() in that case.

usb_disconnect() will not be called in case of a sysfs unbind, but the 
driver's .disconnect() will be called.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-15 18:03   ` Tejun Heo
@ 2015-07-28 14:16     ` Laurent Pinchart
  2015-07-28 15:22       ` Tejun Heo
  0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-07-28 14:16 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Dan Williams, Linux Kernel Mailing List

Hi Teejun,

On Wednesday 15 July 2015 14:03:55 Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 15, 2015 at 10:00:54AM -0700, Dan Williams wrote:
> > Sounds like a real problem.  The drivers I've used devm with have an
> > upper layer that prevents this crash, but that's not much consolation.
> > I think adding lifetime to devm allocations would be useful that way
> > ->probe() and open() can do a devres_get() while ->remove() and
> > close() can do a devres_put().  Perhaps I'm also missing something
> > obvious though...
> 
> Hmmm... so this really is a general lifetime management problem and
> also why sysfs implements revoke semantics.  As memory allocated by
> devm_kmalloc() isn't tied to any specific hardware, it seems a bit
> murky here but if you consider any other resources, this is clear - a
> driver must not access any resources once detach is complete.  These
> aren't resources which can be detached and then held while draining
> existing userland references.  They immediately conflict with the next
> driver which is gonna attach to the device.
> 
> A driver should isolate and drain on-going accesses from userland
> before finishing detaching one way or another.  No resources attached
> to the hardware side can't be held once detaching is complete.  If a
> piece of memory isn't attached to the harware side but the userland
> interface side which gets isolated and drained after detachment, that
> shouldn't be allocated via devm - it has "dev" in its name for a
> reason.

Then that's a message we should start hammering in. There's plenty of drivers 
that have happily switched to devm_kzalloc() to allocate the driver private 
data structure, and that structure can't be freed before the last reference 
from userspace gets dropped. I'd even argue that this is the main use case of 
devm_kzalloc() in drivers.

Using devm_kzalloc() in such a way has value though, and reverting drivers to 
the pre-devm memory allocation code would make error handling and cleanup code 
paths more complex again. Should we introduce a managed allocator for that 
purpose that would have a lifespan explicitly handled by drivers ?

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-28 14:16     ` Laurent Pinchart
@ 2015-07-28 15:22       ` Tejun Heo
  2015-07-28 17:05         ` Laurent Pinchart
  0 siblings, 1 reply; 14+ messages in thread
From: Tejun Heo @ 2015-07-28 15:22 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Dan Williams, Linux Kernel Mailing List

Hello, Laurent.

On Tue, Jul 28, 2015 at 05:16:16PM +0300, Laurent Pinchart wrote:
> Using devm_kzalloc() in such a way has value though, and reverting drivers to 
> the pre-devm memory allocation code would make error handling and cleanup code 
> paths more complex again. Should we introduce a managed allocator for that 
> purpose that would have a lifespan explicitly handled by drivers ?

I don't know.  Sure, we can have memory allocations which are tied to
open file; however, the distinction between that and regular devm
resources, which can't linger on no matter what, would be subtle and
confusing.  IMHO, a better approach would be implmenting generic
revoke feature and sever open files on driver detach so that
everything can be shutdown then.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-28 15:22       ` Tejun Heo
@ 2015-07-28 17:05         ` Laurent Pinchart
  2015-08-04 19:56           ` Pavel Machek
  0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-07-28 17:05 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Dan Williams, Linux Kernel Mailing List

On Tuesday 28 July 2015 11:22:25 Tejun Heo wrote:
> On Tue, Jul 28, 2015 at 05:16:16PM +0300, Laurent Pinchart wrote:
> > Using devm_kzalloc() in such a way has value though, and reverting drivers
> > to the pre-devm memory allocation code would make error handling and
> > cleanup code paths more complex again. Should we introduce a managed
> > allocator for that purpose that would have a lifespan explicitly handled
> > by drivers ?
>
> I don't know.  Sure, we can have memory allocations which are tied to
> open file; however, the distinction between that and regular devm
> resources, which can't linger on no matter what, would be subtle and
> confusing.  IMHO, a better approach would be implmenting generic
> revoke feature and sever open files on driver detach so that
> everything can be shutdown then.

Sounds like a topic for the kernel summit :-) I'll send a proposal.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-07-28 17:05         ` Laurent Pinchart
@ 2015-08-04 19:56           ` Pavel Machek
  2015-08-04 21:26             ` Dmitry Torokhov
  0 siblings, 1 reply; 14+ messages in thread
From: Pavel Machek @ 2015-08-04 19:56 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Tejun Heo, Dan Williams, Linux Kernel Mailing List

On Tue 2015-07-28 20:05:49, Laurent Pinchart wrote:
> On Tuesday 28 July 2015 11:22:25 Tejun Heo wrote:
> > On Tue, Jul 28, 2015 at 05:16:16PM +0300, Laurent Pinchart wrote:
> > > Using devm_kzalloc() in such a way has value though, and reverting drivers
> > > to the pre-devm memory allocation code would make error handling and
> > > cleanup code paths more complex again. Should we introduce a managed
> > > allocator for that purpose that would have a lifespan explicitly handled
> > > by drivers ?
> >
> > I don't know.  Sure, we can have memory allocations which are tied to
> > open file; however, the distinction between that and regular devm
> > resources, which can't linger on no matter what, would be subtle and
> > confusing.  IMHO, a better approach would be implmenting generic
> > revoke feature and sever open files on driver detach so that
> > everything can be shutdown then.
> 
> Sounds like a topic for the kernel summit :-) I'll send a proposal.

Hmm. But that means that devm_ everything is broken for 6 months or
so, right?

Does it mean we should stop taking new devm_ conversions at the very
least?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Is devm_* broken ?
  2015-08-04 19:56           ` Pavel Machek
@ 2015-08-04 21:26             ` Dmitry Torokhov
  0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2015-08-04 21:26 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Laurent Pinchart, Tejun Heo, Dan Williams,
	Linux Kernel Mailing List

On Tue, Aug 4, 2015 at 12:56 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Tue 2015-07-28 20:05:49, Laurent Pinchart wrote:
>> On Tuesday 28 July 2015 11:22:25 Tejun Heo wrote:
>> > On Tue, Jul 28, 2015 at 05:16:16PM +0300, Laurent Pinchart wrote:
>> > > Using devm_kzalloc() in such a way has value though, and reverting drivers
>> > > to the pre-devm memory allocation code would make error handling and
>> > > cleanup code paths more complex again. Should we introduce a managed
>> > > allocator for that purpose that would have a lifespan explicitly handled
>> > > by drivers ?
>> >
>> > I don't know.  Sure, we can have memory allocations which are tied to
>> > open file; however, the distinction between that and regular devm
>> > resources, which can't linger on no matter what, would be subtle and
>> > confusing.  IMHO, a better approach would be implmenting generic
>> > revoke feature and sever open files on driver detach so that
>> > everything can be shutdown then.
>>
>> Sounds like a topic for the kernel summit :-) I'll send a proposal.
>
> Hmm. But that means that devm_ everything is broken for 6 months or
> so, right?
>
> Does it mean we should stop taking new devm_ conversions at the very
> least?

No, we should continue carefully review them and take the ones that make sense.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-08-04 21:26 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-14 22:34 Is devm_* broken ? Laurent Pinchart
2015-07-15 15:51 ` Takashi Iwai
2015-07-15 16:08   ` Laurent Pinchart
2015-07-15 16:20     ` Takashi Iwai
2015-07-15 16:27       ` Laurent Pinchart
2015-07-15 16:34         ` Takashi Iwai
2015-07-28 14:10           ` Laurent Pinchart
2015-07-15 17:00 ` Dan Williams
2015-07-15 18:03   ` Tejun Heo
2015-07-28 14:16     ` Laurent Pinchart
2015-07-28 15:22       ` Tejun Heo
2015-07-28 17:05         ` Laurent Pinchart
2015-08-04 19:56           ` Pavel Machek
2015-08-04 21:26             ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox