[PATCH RFC 1/3] DRM: Armada: Add Armada DRM driver

[Russell King - ARM Linux]

On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote:
> On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote:
> > On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote:
> > > +		mutex_lock(&dev->struct_mutex);
> > > +		free = drm_mm_search_free(&priv->linear, size, align, 0);
> > > +		if (free)
> > > +			obj->linear = drm_mm_get_block(free, size, align);
> > > +		mutex_unlock(&dev->struct_mutex);
> > 
> > The two-stage search_free+get_block drm_mm interface is something I'd like
> > to remove since it' complicates the interface for no gain. And the
> > preallocation trick for atomic contexts is pretty racy as-is. Can you
> > please convert this over to the insert_node interfaces which take a
> > preallocate drm_mm_node?
> 
> Yes and no.  This is only racy if it is callable from atomic contexts,
> which the above isn't, because it takes a mutex, and the above is the
> only place which allocations against that pool are done.  Mutexes can't
> be taken in atomic contexts.  Plus it's using the non-atomic drm_mm_*
> interfaces.
> 
> However, the insert_node interfaces appear not to solve any kind of race.
> All they do is allow the kmalloc to be moved out of this region into
> the user of this code sequence, while offering no additional locking or
> safety.  So the mutex lock around a call to drm_mm_insert_node*() is
> still required.
> 
> As the kmalloc() happens beneath the mutex lock, another thread can't race
> with an allocation in the above code anyway.  The *only* reason I'll change
> this is that it is nicer to the system not to hold locks while allocating
> memory.

Err.  There's bugs here in the other drivers such as i915 which follow
your suggestion.  The problem is this:

Every time they allocate using drm_mm_insert_node(), they kmalloc a new
struct drm_mm_node for this function to fill in and attach.

When they've done with the allocation, they call drm_mm_put_block().
This places the node onto the mm's unused_nodes list provided we don't
already have MM_UNUSED_TARGET nodes on that list.

So, we end up filling up the unused nodes list to MM_UNUSED_TARGET,
at which point we then start freeing any further nodes - and with no
way to pull these off that list, they all just sit there not being
used.

The only function which takes nodes off that list is drm_mm_kmalloc(),
which is declared statically, and so isn't available to drivers.

Are drivers which use the drm_mm_insert_node() function expected to
also use drm_mm_remove_node(), kfreeing the node after that call,
rather than using drm_mm_put_block() ?

Either way, I think someone needs to fix the other drivers and Cc those
patches to Stable...