[Nouveau] [PATCH] drm/nouveau: don't hold spin lock while calling kzalloc with GFP_KERNEL

[Pekka Paalanen]

On Sun,  7 Feb 2010 02:11:20 +0100
Maarten Maathuis <madman2003 at gmail.com> wrote:

> - 'joi' on irc pointed out that this triggers a BUG_ON, because
> kzalloc could sleep.
> - The irq handler should restore the value NV03_PFIFO_CACHES, but
> still it's better if this stuff doesn't happen in the middle of
> fifo create context. I see no reason in spin locking pgraph
> create context, it isn't activated at that stage.
> - Move and rename the lock after some discussion with 'joi', even
> better naming suggestions are appreciated.
> 
> Signed-off-by: Maarten Maathuis <madman2003 at gmail.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_channel.c |    9 ++-------
>  drivers/gpu/drm/nouveau/nouveau_drv.h     |    4 +++-
>  drivers/gpu/drm/nouveau/nouveau_irq.c     |    4 ++--
>  drivers/gpu/drm/nouveau/nouveau_state.c   |    2 +-
>  drivers/gpu/drm/nouveau/nv04_fifo.c       |    5 +++++
>  drivers/gpu/drm/nouveau/nv40_fifo.c       |    5 +++++
>  drivers/gpu/drm/nouveau/nv50_fifo.c       |    4 ++++
>  7 files changed, 22 insertions(+), 11 deletions(-)

<...>

> diff --git a/drivers/gpu/drm/nouveau/nv50_fifo.c
> b/drivers/gpu/drm/nouveau/nv50_fifo.c index 204a79f..983e43b
> 100644 --- a/drivers/gpu/drm/nouveau/nv50_fifo.c
> +++ b/drivers/gpu/drm/nouveau/nv50_fifo.c
> @@ -243,6 +243,7 @@ nv50_fifo_create_context(struct
> nouveau_channel *chan) struct drm_device *dev = chan->dev;
>  	struct drm_nouveau_private *dev_priv = dev->dev_private;
>  	struct nouveau_gpuobj *ramfc = NULL;
> +	unsigned long flags;
>  	int ret;
>  
>  	NV_DEBUG(dev, "ch%d\n", chan->id);
> @@ -278,6 +279,8 @@ nv50_fifo_create_context(struct
> nouveau_channel *chan) return ret;
>  	}
>  
> +	spin_lock_irqsave(&dev_priv->context_switch_lock, flags);
> +
>  	dev_priv->engine.instmem.prepare_access(dev, true);
>  
>  	nv_wo32(dev, ramfc, 0x08/4, chan->pushbuf_base);
> @@ -310,6 +313,7 @@ nv50_fifo_create_context(struct
> nouveau_channel *chan) return ret;
>  	}
>  
> +	spin_unlock_irqrestore(&dev_priv->context_switch_lock,
> flags); return 0;
>  }

After this patch, sparse complains:
drivers/gpu/drm/nouveau/nv50_fifo.c:241:1: warning: context imbalance in 'nv50_fifo_create_context' - different lock contexts for basic block

Near the end of the function, the failure exit path does not unlock the
spinlock.

Btw. before this patch nouveau_channel_alloc() contains two cases of
failure path not releasing the spinlock, plus, under the spinlock,
a call to a function that re-locks the spinlock, hence hangs.
Sparse does warn about exiting a function without releasing a
spinlock in every path, i.e., inconsistent lock behaviour.

Thanks.

-- 
Pekka Paalanen
http://www.iki.fi/pq/