[Acked] ACK: [SRU][Vivid][Wily]drm/fbdev: Return -EBUSY when oopsing

[Andy Whitcroft]

On Mon, Nov 30, 2015 at 07:04:21AM -0700, Tim Gardner wrote:
> Patch included for completeness
> 
> -- 
> Tim Gardner tim.gardner at canonical.com

> From c50bfd08d60cefbe1714c4a53b1c325982858549 Mon Sep 17 00:00:00 2001
> From: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date: Tue, 28 Jul 2015 13:18:40 +0200
> Subject: [PATCH] drm/fbdev: Return -EBUSY when oopsing
> 
> Trying to do anything with kms drivers when oopsing has become a
> failing proposition. But since we can end up in the fbdev code simply
> due to the console unblanking that's done unconditionally just
> removing our panic handler isn't enough. We need to block all fbdev
> callbacks when oopsing.
> 
> There was already one in the blank handler, but it failed silently.
> That makes it impossible for drivers (like i915) who subclass these
> functions to figure this out.
> 
> Instead consistently return -EBUSY so that everyone knows that we
> really don't want to be bothered right now. This also allows us to
> remove a pile of FIXMEs from the i915 fbdev code (since due to the
> failure code they now won't attempt to grab dangerous locks any more).
> 
> Cc: Dave Airlie <airlied at gmail.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> Reviewed-by: Rob Clark <robdclark at gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c    | 24 ++++++++++++------------
>  drivers/gpu/drm/i915/intel_fbdev.c | 21 ---------------------
>  2 files changed, 12 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index c1cb753..f16eed0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -491,14 +491,6 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>  	int i, j;
>  
>  	/*
> -	 * fbdev->blank can be called from irq context in case of a panic.
> -	 * Since we already have our own special panic handler which will
> -	 * restore the fbdev console mode completely, just bail out early.
> -	 */
> -	if (oops_in_progress)
> -		return;
> -
> -	/*
>  	 * For each CRTC in this fb, turn the connectors on/off.
>  	 */
>  	drm_modeset_lock_all(dev);
> @@ -531,6 +523,9 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>   */
>  int drm_fb_helper_blank(int blank, struct fb_info *info)
>  {
> +	if (oops_in_progress)
> +		return -EBUSY;
> +
>  	switch (blank) {
>  	/* Display: On; HSync: On, VSync: On */
>  	case FB_BLANK_UNBLANK:
> @@ -978,9 +973,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  	int i, j, rc = 0;
>  	int start;
>  
> -	if (__drm_modeset_lock_all(dev, !!oops_in_progress)) {
> +	if (oops_in_progress)
>  		return -EBUSY;
> -	}
> +
> +	drm_modeset_lock_all(dev);
>  	if (!drm_fb_helper_is_bound(fb_helper)) {
>  		drm_modeset_unlock_all(dev);
>  		return -EBUSY;
> @@ -1129,6 +1125,9 @@ int drm_fb_helper_set_par(struct fb_info *info)
>  	struct drm_fb_helper *fb_helper = info->par;
>  	struct fb_var_screeninfo *var = &info->var;
>  
> +	if (oops_in_progress)
> +		return -EBUSY;
> +
>  	if (var->pixclock != 0) {
>  		DRM_ERROR("PIXEL CLOCK SET\n");
>  		return -EINVAL;
> @@ -1154,9 +1153,10 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  	int ret = 0;
>  	int i;
>  
> -	if (__drm_modeset_lock_all(dev, !!oops_in_progress)) {
> +	if (oops_in_progress)
>  		return -EBUSY;
> -	}
> +
> +	drm_modeset_lock_all(dev);
>  	if (!drm_fb_helper_is_bound(fb_helper)) {
>  		drm_modeset_unlock_all(dev);
>  		return -EBUSY;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7eff33f..f786c9b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -55,13 +55,6 @@ static int intel_fbdev_set_par(struct fb_info *info)
>  	ret = drm_fb_helper_set_par(info);
>  
>  	if (ret == 0) {
> -		/*
> -		 * FIXME: fbdev presumes that all callbacks also work from
> -		 * atomic contexts and relies on that for emergency oops
> -		 * printing. KMS totally doesn't do that and the locking here is
> -		 * by far not the only place this goes wrong.  Ignore this for
> -		 * now until we solve this for real.
> -		 */
>  		mutex_lock(&fb_helper->dev->struct_mutex);
>  		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
>  		mutex_unlock(&fb_helper->dev->struct_mutex);
> @@ -80,13 +73,6 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
>  	ret = drm_fb_helper_blank(blank, info);
>  
>  	if (ret == 0) {
> -		/*
> -		 * FIXME: fbdev presumes that all callbacks also work from
> -		 * atomic contexts and relies on that for emergency oops
> -		 * printing. KMS totally doesn't do that and the locking here is
> -		 * by far not the only place this goes wrong.  Ignore this for
> -		 * now until we solve this for real.
> -		 */
>  		mutex_lock(&fb_helper->dev->struct_mutex);
>  		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
>  		mutex_unlock(&fb_helper->dev->struct_mutex);
> @@ -106,13 +92,6 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
>  	ret = drm_fb_helper_pan_display(var, info);
>  
>  	if (ret == 0) {
> -		/*
> -		 * FIXME: fbdev presumes that all callbacks also work from
> -		 * atomic contexts and relies on that for emergency oops
> -		 * printing. KMS totally doesn't do that and the locking here is
> -		 * by far not the only place this goes wrong.  Ignore this for
> -		 * now until we solve this for real.
> -		 */
>  		mutex_lock(&fb_helper->dev->struct_mutex);
>  		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
>  		mutex_unlock(&fb_helper->dev->struct_mutex);
> -- 
> 2.5.0
> 

That seems to simplify handling of the framebuffer on oops, something
which can only be positive.  Looks to do what is claimed.

Acked-by: Andy Whitcroft <apw at canonical.com>

-apw