Re: [PATCH] drm: do not expose vblank data before drm_vblank_init completes

[Marcin Slusarz <marcin.slusarz@gmail.com>]

On Sun, May 27, 2012 at 10:25:21PM +0200, Marcin Slusarz wrote:
> On Thu, May 24, 2012 at 08:09:41PM +0200, Bruno Prémont wrote:
> > I can easily trigger a crash in nouveau interrupt handler by unbinding
> > and rebinding the GPU.
> > 
> > The command used:
> >   echo $pci_device > nouveau/unbind && \
> > 	sleep 5 && \
> > 	echo $pci_device > nouveau/bind
> > 
> > 
> > Kernel is 3.4.0 with modular drm/nouveau.
> > GPU is NVidia nForce IGP (NV11)
> > 
> > 
> > Unbinding seems to work fine, display switching back to VGA text mode.
> > Rebinding the GPU slightly later causes the below trace:
> > 
> > (...)
> 
> It crashed because Nouveau failed to disable vblank interrupt on unbind
> and this interrupt triggered while drm was initializing vblank data.
> 
> Nouveau side was fixed by "drm/nv04/disp: disable vblank interrupts when
> disabling display" by Ben Skeggs (3.5 merge window), drm side can be fixed
> by this:
> 
> ---
> From: Marcin Slusarz <marcin.slusarz@gmail.com>
> Subject: [PATCH] drm: do not expose vblank data before drm_vblank_init completes
> 
> It fixes oops in drm_handle_vblank when vblank interrupt triggers before
> drm_vblank_init completes. Driver side should not let this happen, but let's
> be on the safe side and handle this case.
> 
> Reported-by: Bruno Prémont <bonbons@linux-vserver.org>
> Tested-by: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Marcin Slusarz <marcin.slusarz@gmail.com>
> ---
>  drivers/gpu/drm/drm_irq.c |   21 +++++++++++++--------
>  1 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c869436..7dda18c 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -183,12 +183,8 @@ static void vblank_disable_fn(unsigned long arg)
>  	}
>  }
>  
> -void drm_vblank_cleanup(struct drm_device *dev)
> +static void __drm_vblank_cleanup(struct drm_device *dev)
>  {
> -	/* Bail if the driver didn't call drm_vblank_init() */
> -	if (dev->num_crtcs == 0)
> -		return;
> -
>  	del_timer(&dev->vblank_disable_timer);
>  
>  	vblank_disable_fn((unsigned long)dev);
> @@ -201,6 +197,15 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  	kfree(dev->last_vblank_wait);
>  	kfree(dev->vblank_inmodeset);
>  	kfree(dev->_vblank_time);
> +}
> +
> +void drm_vblank_cleanup(struct drm_device *dev)
> +{
> +	/* Bail if the driver didn't call drm_vblank_init() */
> +	if (dev->num_crtcs == 0)
> +		return;
> +
> +	__drm_vblank_cleanup(dev);
>  
>  	dev->num_crtcs = 0;
>  }
> @@ -215,8 +220,6 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>  	spin_lock_init(&dev->vbl_lock);
>  	spin_lock_init(&dev->vblank_time_lock);
>  
> -	dev->num_crtcs = num_crtcs;
> -
>  	dev->vbl_queue = kmalloc(sizeof(wait_queue_head_t) * num_crtcs,
>  				 GFP_KERNEL);
>  	if (!dev->vbl_queue)
> @@ -268,10 +271,12 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>  	}
>  
>  	dev->vblank_disable_allowed = 0;
> +	dev->num_crtcs = num_crtcs;
> +
>  	return 0;
>  
>  err:
> -	drm_vblank_cleanup(dev);
> +	__drm_vblank_cleanup(dev);
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_vblank_init);
> -- 

Dave?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel