drm/i915: add audio_ptr pointer check

Commit Message

[e-mail] March 2, 2016, 2:24 a.m. UTC

From: Libin Yang <[e-mail]>

check to make sure audio_ptr is not NULL before
using it.

Signed-off-by: Libin Yang <[e-mail]>
---
 drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Takashi Iwai March 2, 2016, 7:44 a.m. UTC | #1

On Wed, 02 Mar 2016 03:24:31 +0100,
[e-mail] wrote:
> 
> From: Libin Yang <[e-mail]>
> 
> check to make sure audio_ptr is not NULL before
> using it.

I don't think non-NULL is mandatory.  If any invalid access is seen,
it should be fixed rather in the audio side.


thanks,

Takashi

> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 31f6d21..667596d 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -529,7 +529,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  	intel_dig_port->audio_connector = connector;
>  	mutex_unlock(&dev_priv->av_mutex);
>  
> -	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> +	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
> +	    acomp->audio_ops->audio_ptr)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
>  }
>  
> @@ -556,7 +557,8 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  	intel_dig_port->audio_connector = NULL;
>  	mutex_unlock(&dev_priv->av_mutex);
>  
> -	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> +	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
> +	    acomp->audio_ops->audio_ptr)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
>  }
>  
> -- 
> 1.9.1
>

Yang, Libin March 2, 2016, 7:59 a.m. UTC | #2

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Wednesday, March 02, 2016 3:44 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Vetter, Daniel;
> [e-mail]; Yang, Libin
> Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> 
> On Wed, 02 Mar 2016 03:24:31 +0100,
> [e-mail] wrote:
> >
> > From: Libin Yang <[e-mail]>
> >
> > check to make sure audio_ptr is not NULL before
> > using it.
> 
> I don't think non-NULL is mandatory.  If any invalid access is seen,
> it should be fixed rather in the audio side.

When I enable MST with other patches (not submitted so far),
I found system may be random hang when boot up.

After apply this patch and another patch (I sent the patch in
ALSA Mail list: ALSA: hda - hdmi defer to register acomp eld notifier),
the hang issue is fixed.

I make the patch because in sound driver side, setting audio_ptr and
audio_ops is not in the lock. I'm wondering that the compiler or CPU
may optimize and change the setting order (the gfx code and alsa code
may run parallelly?).

Regards,
Libin

> 
> 
> thanks,
> 
> Takashi
> 
> >
> > Signed-off-by: Libin Yang <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index 31f6d21..667596d 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -529,7 +529,8 @@ void intel_audio_codec_enable(struct
> intel_encoder *intel_encoder)
> >  	intel_dig_port->audio_connector = connector;
> >  	mutex_unlock(&dev_priv->av_mutex);
> >
> > -	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> > +	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify &&
> > +	    acomp->audio_ops->audio_ptr)
> >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr, (int) port);
> >  }
> >
> > @@ -556,7 +557,8 @@ void intel_audio_codec_disable(struct
> intel_encoder *intel_encoder)
> >  	intel_dig_port->audio_connector = NULL;
> >  	mutex_unlock(&dev_priv->av_mutex);
> >
> > -	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> > +	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify &&
> > +	    acomp->audio_ops->audio_ptr)
> >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr, (int) port);
> >  }
> >
> > --
> > 1.9.1
> >

Jani Nikula March 2, 2016, 8 a.m. UTC | #3

On Wed, 02 Mar 2016, Takashi Iwai <[e-mail]> wrote:
> On Wed, 02 Mar 2016 03:24:31 +0100,
> [e-mail] wrote:
>> 
>> From: Libin Yang <[e-mail]>
>> 
>> check to make sure audio_ptr is not NULL before
>> using it.
>
> I don't think non-NULL is mandatory.  If any invalid access is seen,
> it should be fixed rather in the audio side.

Agreed.

BR,
Jani

Takashi Iwai March 2, 2016, 8:56 a.m. UTC | #4

On Wed, 02 Mar 2016 08:59:23 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Wednesday, March 02, 2016 3:44 PM
> > To: [e-mail]
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Vetter, Daniel;
> > [e-mail]; Yang, Libin
> > Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> > 
> > On Wed, 02 Mar 2016 03:24:31 +0100,
> > [e-mail] wrote:
> > >
> > > From: Libin Yang <[e-mail]>
> > >
> > > check to make sure audio_ptr is not NULL before
> > > using it.
> > 
> > I don't think non-NULL is mandatory.  If any invalid access is seen,
> > it should be fixed rather in the audio side.
> 
> When I enable MST with other patches (not submitted so far),
> I found system may be random hang when boot up.
> 
> After apply this patch and another patch (I sent the patch in
> ALSA Mail list: ALSA: hda - hdmi defer to register acomp eld notifier),
> the hang issue is fixed.
> 
> I make the patch because in sound driver side, setting audio_ptr and
> audio_ops is not in the lock. I'm wondering that the compiler or CPU
> may optimize and change the setting order (the gfx code and alsa code
> may run parallelly?).

There are several ways to deal with such a thing.  An easy one would
be the memory barrier.


Takashi

Yang, Libin March 3, 2016, 3:01 a.m. UTC | #5

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Wednesday, March 02, 2016 4:57 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Vetter, Daniel
> Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> 
> On Wed, 02 Mar 2016 08:59:23 +0100,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:[e-mail]]
> > > Sent: Wednesday, March 02, 2016 3:44 PM
> > > To: [e-mail]
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; Vetter,
> Daniel;
> > > [e-mail]; Yang, Libin
> > > Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> > >
> > > On Wed, 02 Mar 2016 03:24:31 +0100,
> > > [e-mail] wrote:
> > > >
> > > > From: Libin Yang <[e-mail]>
> > > >
> > > > check to make sure audio_ptr is not NULL before
> > > > using it.
> > >
> > > I don't think non-NULL is mandatory.  If any invalid access is seen,
> > > it should be fixed rather in the audio side.
> >
> > When I enable MST with other patches (not submitted so far),
> > I found system may be random hang when boot up.
> >
> > After apply this patch and another patch (I sent the patch in
> > ALSA Mail list: ALSA: hda - hdmi defer to register acomp eld notifier),
> > the hang issue is fixed.
> >
> > I make the patch because in sound driver side, setting audio_ptr and
> > audio_ops is not in the lock. I'm wondering that the compiler or CPU
> > may optimize and change the setting order (the gfx code and alsa code
> > may run parallelly?).
> 
> There are several ways to deal with such a thing.  An easy one would
> be the memory barrier.

Yes, agree to use mb. It will be easier and safer.

I think wmb() should be enough for this case, right?

I will do the stress test on mb.

Regards,
Libin

> 
> 
> Takashi

Patch

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 31f6d21..667596d 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -529,7 +529,8 @@  void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 	intel_dig_port->audio_connector = connector;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }
 
@@ -556,7 +557,8 @@  void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 	intel_dig_port->audio_connector = NULL;
 	mutex_unlock(&dev_priv->av_mutex);
 
-	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify &&
+	    acomp->audio_ops->audio_ptr)
 		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port);
 }