[PATCH] drm/i915: Don't oops during modeset shutdown after lpe audio deinit

[PATCH] drm/i915: Don't oops during modeset shutdown after lpe audio deinit

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We deinit the lpe audio device before we call
drm_atomic_helper_shutdown(), which means the platform device
may already be gone when it comes time to shut down the crtc.
As we don't know when the last reference to the platform
device gets dropped by the audio driver we can't assume that
the device and its data are still around when turning off the
crtc. Mark the platform device as gone as soon as we do the
audio deinit.

Cc: stable@vger.kernel.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lpe_audio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
index cdf19553ffac..5d5336fbe7b0 100644
--- a/drivers/gpu/drm/i915/intel_lpe_audio.c
+++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
@@ -297,8 +297,10 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
 	lpe_audio_platdev_destroy(dev_priv);
 
 	irq_free_desc(dev_priv->lpe_audio.irq);
-}
 
+	dev_priv->lpe_audio.irq = -1;
+	dev_priv->lpe_audio.platdev = NULL;
+}
 
 /**
  * intel_lpe_audio_notify() - notify lpe audio event
-- 
2.18.1

Re: [Intel-gfx] [PATCH] drm/i915: Don't oops during modeset shutdown after lpe audio deinit

[Chris Wilson]

Quoting Ville Syrjala (2018-11-05 19:46:04)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We deinit the lpe audio device before we call
> drm_atomic_helper_shutdown(), which means the platform device
> may already be gone when it comes time to shut down the crtc.

Doesn't this mean that we fail to notify the audio codec of it being
turned off? I'm wondering if we shouldn't do the display/gt idling first
(like a i915_driver_unload_prepare).

> As we don't know when the last reference to the platform
> device gets dropped by the audio driver we can't assume that
> the device and its data are still around when turning off the
> crtc. Mark the platform device as gone as soon as we do the
> audio deinit.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> index cdf19553ffac..5d5336fbe7b0 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -297,8 +297,10 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>         lpe_audio_platdev_destroy(dev_priv);
>  
>         irq_free_desc(dev_priv->lpe_audio.irq);
> -}
>  
> +       dev_priv->lpe_audio.irq = -1;
> +       dev_priv->lpe_audio.platdev = NULL;
> +}

This thanks to HAS_LPE_AUDIO() (confusing macro of the day) will prevent
a use-after-free during crtc shutdown.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [Intel-gfx] [PATCH] drm/i915: Don't oops during modeset shutdown after lpe audio deinit

[Ville Syrjälä]

On Mon, Nov 05, 2018 at 08:01:17PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-11-05 19:46:04)
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > 
> > We deinit the lpe audio device before we call
> > drm_atomic_helper_shutdown(), which means the platform device
> > may already be gone when it comes time to shut down the crtc.
> 
> Doesn't this mean that we fail to notify the audio codec of it being
> turned off? I'm wondering if we shouldn't do the display/gt idling first
> (like a i915_driver_unload_prepare).

Yeah, would probably be better to do the display off earlier.
Not too keen on diving into that rabbit hole at this time however.

> 
> > As we don't know when the last reference to the platform
> > device gets dropped by the audio driver we can't assume that
> > the device and its data are still around when turning off the
> > crtc. Mark the platform device as gone as soon as we do the
> > audio deinit.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index cdf19553ffac..5d5336fbe7b0 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -297,8 +297,10 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
> >         lpe_audio_platdev_destroy(dev_priv);
> >  
> >         irq_free_desc(dev_priv->lpe_audio.irq);
> > -}
> >  
> > +       dev_priv->lpe_audio.irq = -1;
> > +       dev_priv->lpe_audio.platdev = NULL;
> > +}
> 
> This thanks to HAS_LPE_AUDIO() (confusing macro of the day) will prevent
> a use-after-free during crtc shutdown.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks. Pushed.

-- 
Ville Syrj�l�
Intel