drm/i915: add audio_ptr pointer check

Submitted by libin.yang@linux.intel.com on March 2, 2016, 2:24 a.m.

Details

Message ID 1456885471-29344-1-git-send-email-libin.yang@linux.intel.com
State New
Headers show
Series "drm/i915: add audio_ptr pointer check" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

libin.yang@linux.intel.com March 2, 2016, 2:24 a.m.
From: Libin Yang <libin.yang@linux.intel.com>

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

Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

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);
 }
 

Comments

On Wed, 02 Mar 2016 03:24:31 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> 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 <libin.yang@linux.intel.com>
> ---
>  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
>
Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, March 02, 2016 3:44 PM
> To: libin.yang@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com;
> jani.nikula@linux.intel.com; ville.syrjala@linux.intel.com; Vetter, Daniel;
> tiwai@suse.de; Yang, Libin
> Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> 
> On Wed, 02 Mar 2016 03:24:31 +0100,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > 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 <libin.yang@linux.intel.com>
> > ---
> >  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
> >
On Wed, 02 Mar 2016, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 02 Mar 2016 03:24:31 +0100,
> libin.yang@linux.intel.com wrote:
>> 
>> From: Libin Yang <libin.yang@linux.intel.com>
>> 
>> 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
On Wed, 02 Mar 2016 08:59:23 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, March 02, 2016 3:44 PM
> > To: libin.yang@linux.intel.com
> > Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com;
> > jani.nikula@linux.intel.com; ville.syrjala@linux.intel.com; Vetter, Daniel;
> > tiwai@suse.de; Yang, Libin
> > Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> > 
> > On Wed, 02 Mar 2016 03:24:31 +0100,
> > libin.yang@linux.intel.com wrote:
> > >
> > > From: Libin Yang <libin.yang@linux.intel.com>
> > >
> > > 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
Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, March 02, 2016 4:57 PM
> To: Yang, Libin
> Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> conselvan2@gmail.com; jani.nikula@linux.intel.com;
> ville.syrjala@linux.intel.com; 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:tiwai@suse.de]
> > > Sent: Wednesday, March 02, 2016 3:44 PM
> > > To: libin.yang@linux.intel.com
> > > Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com;
> > > jani.nikula@linux.intel.com; ville.syrjala@linux.intel.com; Vetter,
> Daniel;
> > > tiwai@suse.de; Yang, Libin
> > > Subject: Re: [PATCH] drm/i915: add audio_ptr pointer check
> > >
> > > On Wed, 02 Mar 2016 03:24:31 +0100,
> > > libin.yang@linux.intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > 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