ALSA: hda - hdmi get jack from hda_jack_tbl when not dyn_pcm_assign

Commit Message

[e-mail] Feb. 18, 2016, 5:25 a.m. UTC

From: Libin Yang <[e-mail]>

On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
This may cause access invalid memory when calling snd_jack_report.

Please see more detail from:
https://bugs.freedesktop.org/show_bug.cgi?id=94079

Signed-off-by: Libin Yang <[e-mail]>
---
 sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Yang, Libin Feb. 18, 2016, 6:16 a.m. UTC | #1

Hi Takashi,

I submitted the patch which can fix a regression involved from my
jack patch:
commit 25e4abb33df3aafa7d1efba8f82f9178268efab1
Author: Libin Yang <[e-mail]>
Date:   Tue Jan 12 11:13:27 2016 +0800

    ALSA: hda - hdmi jack created based on pcm

I'm sorry for the inconvenience. 

As the dyn pcm patches are merged into the for-next branch, I'm asking
our QA to do the full test on it. They said they can start the stress test
from next week.

Regards,
Libin


> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> Sent: Thursday, February 18, 2016 1:25 PM
> To: [e-mail]; [e-mail]
> Cc: Lin, Mengdong; Yang, Libin; Libin Yang
> Subject: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when not
> dyn_pcm_assign
> 
> From: Libin Yang <[e-mail]>
> 
> On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> This may cause access invalid memory when calling snd_jack_report.
> 
> Please see more detail from:
> https://bugs.freedesktop.org/show_bug.cgi?id=94079
> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index f4443b5..3b47101 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct
> hda_codec *codec,
>  {
>  	struct hdmi_spec *spec = codec->spec;
>  	struct hdmi_eld *eld = &spec->temp_eld;
> +	struct hda_jack_tbl *jack_tbl;
>  	struct snd_jack *jack = NULL;
>  	int size;
> 
> @@ -1989,10 +1990,21 @@ static void sync_eld_via_acomp(struct
> hda_codec *codec,
>  	/* pcm_idx >=0 before update_eld() means it is in monitor
>  	 * disconnected event. Jack must be fetched before update_eld()
>  	 */
> -	if (per_pin->pcm_idx >= 0)
> +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> +	 * free snd_jack. This may cause access invalid memory
> +	 * when calling snd_jack_report
> +	 */
> +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
>  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> +	else {
> +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> >pin_nid);
> +		if (jack_tbl)
> +			jack = jack_tbl->jack;
> +	}
>  	update_eld(codec, per_pin, eld);
> -	if (jack == NULL && per_pin->pcm_idx >= 0)
> +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec-
> >dyn_pcm_assign)
>  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
>  	if (jack == NULL)
>  		goto unlock;
> --
> 1.9.1

Takashi Iwai Feb. 18, 2016, 7:53 a.m. UTC | #2

On Thu, 18 Feb 2016 06:25:02 +0100,
[e-mail] wrote:
> 
> From: Libin Yang <[e-mail]>
> 
> On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> This may cause access invalid memory when calling snd_jack_report.
> 
> Please see more detail from:
> https://bugs.freedesktop.org/show_bug.cgi?id=94079
> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index f4443b5..3b47101 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>  {
>  	struct hdmi_spec *spec = codec->spec;
>  	struct hdmi_eld *eld = &spec->temp_eld;
> +	struct hda_jack_tbl *jack_tbl;
>  	struct snd_jack *jack = NULL;
>  	int size;
>  
> @@ -1989,10 +1990,21 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>  	/* pcm_idx >=0 before update_eld() means it is in monitor
>  	 * disconnected event. Jack must be fetched before update_eld()
>  	 */
> -	if (per_pin->pcm_idx >= 0)
> +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> +	 * free snd_jack. This may cause access invalid memory
> +	 * when calling snd_jack_report
> +	 */
> +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
>  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> +	else {
> +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
> +		if (jack_tbl)
> +			jack = jack_tbl->jack;
> +	}
>  	update_eld(codec, per_pin, eld);
> -	if (jack == NULL && per_pin->pcm_idx >= 0)
> +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
>  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
>  	if (jack == NULL)
>  		goto unlock;

I'd create a separate small helper function, e.g. pin_to_jack()
doing like:

pin_idx_to_jack(codec, per_pin)
{
	if (spec->dyn_pcm_assign) {
		if (per_pin->pcm_idx < 0)
			return NULL;
		return spec->pcm_rec[per_pin->pcm_idx].jack;
	} else {
		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
		if (!jack_tbl)
			return NULL;
		return jack_tbl->jack;
	}
}

Then the code update_eld() will be cleaner,

	jack = pin_to_jack(codec, per_pin);
	update_eld(codec, per_pin, eld);
	if (!jack)
		jack = pin_to_jack(codec, per_pin);
	if (!jack)
		goto unlock;

Alternatively, assign pcm_rec[].jack beforehand for non-dyn PCM.
It's fixed, so you can assign it in initialization.


thanks,

Takashi

Takashi Iwai Feb. 18, 2016, 7:54 a.m. UTC | #3

On Thu, 18 Feb 2016 07:16:29 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> I submitted the patch which can fix a regression involved from my
> jack patch:
> commit 25e4abb33df3aafa7d1efba8f82f9178268efab1
> Author: Libin Yang <[e-mail]>
> Date:   Tue Jan 12 11:13:27 2016 +0800
> 
>     ALSA: hda - hdmi jack created based on pcm
> 
> I'm sorry for the inconvenience. 
> 
> As the dyn pcm patches are merged into the for-next branch, I'm asking
> our QA to do the full test on it. They said they can start the stress test
> from next week.

One concern is the report from CI test about the crash at unloading
the module.  I couldn't reproduce it locally.  Could you take a look?


Takashi

> 
> Regards,
> Libin
> 
> 
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]]
> > Sent: Thursday, February 18, 2016 1:25 PM
> > To: [e-mail]; [e-mail]
> > Cc: Lin, Mengdong; Yang, Libin; Libin Yang
> > Subject: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when not
> > dyn_pcm_assign
> > 
> > From: Libin Yang <[e-mail]>
> > 
> > On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> > NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> > This may cause access invalid memory when calling snd_jack_report.
> > 
> > Please see more detail from:
> > https://bugs.freedesktop.org/show_bug.cgi?id=94079
> > 
> > Signed-off-by: Libin Yang <[e-mail]>
> > ---
> >  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index f4443b5..3b47101 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct
> > hda_codec *codec,
> >  {
> >  	struct hdmi_spec *spec = codec->spec;
> >  	struct hdmi_eld *eld = &spec->temp_eld;
> > +	struct hda_jack_tbl *jack_tbl;
> >  	struct snd_jack *jack = NULL;
> >  	int size;
> > 
> > @@ -1989,10 +1990,21 @@ static void sync_eld_via_acomp(struct
> > hda_codec *codec,
> >  	/* pcm_idx >=0 before update_eld() means it is in monitor
> >  	 * disconnected event. Jack must be fetched before update_eld()
> >  	 */
> > -	if (per_pin->pcm_idx >= 0)
> > +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> > +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> > +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> > +	 * free snd_jack. This may cause access invalid memory
> > +	 * when calling snd_jack_report
> > +	 */
> > +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > +	else {
> > +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> > >pin_nid);
> > +		if (jack_tbl)
> > +			jack = jack_tbl->jack;
> > +	}
> >  	update_eld(codec, per_pin, eld);
> > -	if (jack == NULL && per_pin->pcm_idx >= 0)
> > +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec-
> > >dyn_pcm_assign)
> >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> >  	if (jack == NULL)
> >  		goto unlock;
> > --
> > 1.9.1
>

Yang, Libin Feb. 18, 2016, 8:02 a.m. UTC | #4

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Thursday, February 18, 2016 3:54 PM
> To: [e-mail]
> Cc: [e-mail]; Lin, Mengdong; Yang, Libin
> Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> not dyn_pcm_assign
> 
> On Thu, 18 Feb 2016 06:25:02 +0100,
> [e-mail] wrote:
> >
> > From: Libin Yang <[e-mail]>
> >
> > On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> > NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> > This may cause access invalid memory when calling snd_jack_report.
> >
> > Please see more detail from:
> > https://bugs.freedesktop.org/show_bug.cgi?id=94079
> >
> > Signed-off-by: Libin Yang <[e-mail]>
> > ---
> >  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> > index f4443b5..3b47101 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct
> hda_codec *codec,
> >  {
> >  	struct hdmi_spec *spec = codec->spec;
> >  	struct hdmi_eld *eld = &spec->temp_eld;
> > +	struct hda_jack_tbl *jack_tbl;
> >  	struct snd_jack *jack = NULL;
> >  	int size;
> >
> > @@ -1989,10 +1990,21 @@ static void sync_eld_via_acomp(struct
> hda_codec *codec,
> >  	/* pcm_idx >=0 before update_eld() means it is in monitor
> >  	 * disconnected event. Jack must be fetched before update_eld()
> >  	 */
> > -	if (per_pin->pcm_idx >= 0)
> > +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> > +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> > +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> > +	 * free snd_jack. This may cause access invalid memory
> > +	 * when calling snd_jack_report
> > +	 */
> > +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > +	else {
> > +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> >pin_nid);
> > +		if (jack_tbl)
> > +			jack = jack_tbl->jack;
> > +	}
> >  	update_eld(codec, per_pin, eld);
> > -	if (jack == NULL && per_pin->pcm_idx >= 0)
> > +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec-
> >dyn_pcm_assign)
> >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> >  	if (jack == NULL)
> >  		goto unlock;
> 
> I'd create a separate small helper function, e.g. pin_to_jack()
> doing like:
> 
> pin_idx_to_jack(codec, per_pin)
> {
> 	if (spec->dyn_pcm_assign) {
> 		if (per_pin->pcm_idx < 0)
> 			return NULL;
> 		return spec->pcm_rec[per_pin->pcm_idx].jack;
> 	} else {
> 		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> >pin_nid);
> 		if (!jack_tbl)
> 			return NULL;
> 		return jack_tbl->jack;
> 	}
> }
> 
> Then the code update_eld() will be cleaner,
> 
> 	jack = pin_to_jack(codec, per_pin);
> 	update_eld(codec, per_pin, eld);
> 	if (!jack)
> 		jack = pin_to_jack(codec, per_pin);
> 	if (!jack)
> 		goto unlock;
> 
> Alternatively, assign pcm_rec[].jack beforehand for non-dyn PCM.
> It's fixed, so you can assign it in initialization.

Get it. So your patch will be merged to for-next soon? Thanks.

Regards,
Libin

> 
> 
> thanks,
> 
> Takashi

Yang, Libin Feb. 18, 2016, 8:06 a.m. UTC | #5

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Thursday, February 18, 2016 3:55 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; Lin,
> Mengdong
> Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> not dyn_pcm_assign
> 
> On Thu, 18 Feb 2016 07:16:29 +0100,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > I submitted the patch which can fix a regression involved from my
> > jack patch:
> > commit 25e4abb33df3aafa7d1efba8f82f9178268efab1
> > Author: Libin Yang <[e-mail]>
> > Date:   Tue Jan 12 11:13:27 2016 +0800
> >
> >     ALSA: hda - hdmi jack created based on pcm
> >
> > I'm sorry for the inconvenience.
> >
> > As the dyn pcm patches are merged into the for-next branch, I'm asking
> > our QA to do the full test on it. They said they can start the stress test
> > from next week.
> 
> One concern is the report from CI test about the crash at unloading
> the module.  I couldn't reproduce it locally.  Could you take a look?

There is the same oops when unloading snd modules on my side
(it happens occasionally).
With the patch, the oops is fixed. But I didn't meet the crash. 
The system is still alive.

I'm asking for their test case. As soon as I get the test case, I will
do the test.

Regards,
Libin
> 
> 
> Takashi
> 
> >
> > Regards,
> > Libin
> >
> >
> > > -----Original Message-----
> > > From: [e-mail] [mailto:[e-mail]]
> > > Sent: Thursday, February 18, 2016 1:25 PM
> > > To: [e-mail]; [e-mail]
> > > Cc: Lin, Mengdong; Yang, Libin; Libin Yang
> > > Subject: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> not
> > > dyn_pcm_assign
> > >
> > > From: Libin Yang <[e-mail]>
> > >
> > > On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> > > NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> > > This may cause access invalid memory when calling snd_jack_report.
> > >
> > > Please see more detail from:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=94079
> > >
> > > Signed-off-by: Libin Yang <[e-mail]>
> > > ---
> > >  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> > > index f4443b5..3b47101 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct
> > > hda_codec *codec,
> > >  {
> > >  	struct hdmi_spec *spec = codec->spec;
> > >  	struct hdmi_eld *eld = &spec->temp_eld;
> > > +	struct hda_jack_tbl *jack_tbl;
> > >  	struct snd_jack *jack = NULL;
> > >  	int size;
> > >
> > > @@ -1989,10 +1990,21 @@ static void sync_eld_via_acomp(struct
> > > hda_codec *codec,
> > >  	/* pcm_idx >=0 before update_eld() means it is in monitor
> > >  	 * disconnected event. Jack must be fetched before update_eld()
> > >  	 */
> > > -	if (per_pin->pcm_idx >= 0)
> > > +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> > > +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> > > +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> > > +	 * free snd_jack. This may cause access invalid memory
> > > +	 * when calling snd_jack_report
> > > +	 */
> > > +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> > >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > > +	else {
> > > +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> > > >pin_nid);
> > > +		if (jack_tbl)
> > > +			jack = jack_tbl->jack;
> > > +	}
> > >  	update_eld(codec, per_pin, eld);
> > > -	if (jack == NULL && per_pin->pcm_idx >= 0)
> > > +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec-
> > > >dyn_pcm_assign)
> > >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > >  	if (jack == NULL)
> > >  		goto unlock;
> > > --
> > > 1.9.1
> >

Takashi Iwai Feb. 18, 2016, 8:07 a.m. UTC | #6

On Thu, 18 Feb 2016 09:02:24 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Thursday, February 18, 2016 3:54 PM
> > To: [e-mail]
> > Cc: [e-mail]; Lin, Mengdong; Yang, Libin
> > Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> > not dyn_pcm_assign
> > 
> > On Thu, 18 Feb 2016 06:25:02 +0100,
> > [e-mail] wrote:
> > >
> > > From: Libin Yang <[e-mail]>
> > >
> > > On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> > > NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> > > This may cause access invalid memory when calling snd_jack_report.
> > >
> > > Please see more detail from:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=94079
> > >
> > > Signed-off-by: Libin Yang <[e-mail]>
> > > ---
> > >  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c
> > b/sound/pci/hda/patch_hdmi.c
> > > index f4443b5..3b47101 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct
> > hda_codec *codec,
> > >  {
> > >  	struct hdmi_spec *spec = codec->spec;
> > >  	struct hdmi_eld *eld = &spec->temp_eld;
> > > +	struct hda_jack_tbl *jack_tbl;
> > >  	struct snd_jack *jack = NULL;
> > >  	int size;
> > >
> > > @@ -1989,10 +1990,21 @@ static void sync_eld_via_acomp(struct
> > hda_codec *codec,
> > >  	/* pcm_idx >=0 before update_eld() means it is in monitor
> > >  	 * disconnected event. Jack must be fetched before update_eld()
> > >  	 */
> > > -	if (per_pin->pcm_idx >= 0)
> > > +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> > > +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> > > +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> > > +	 * free snd_jack. This may cause access invalid memory
> > > +	 * when calling snd_jack_report
> > > +	 */
> > > +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> > >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > > +	else {
> > > +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> > >pin_nid);
> > > +		if (jack_tbl)
> > > +			jack = jack_tbl->jack;
> > > +	}
> > >  	update_eld(codec, per_pin, eld);
> > > -	if (jack == NULL && per_pin->pcm_idx >= 0)
> > > +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec-
> > >dyn_pcm_assign)
> > >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > >  	if (jack == NULL)
> > >  		goto unlock;
> > 
> > I'd create a separate small helper function, e.g. pin_to_jack()
> > doing like:
> > 
> > pin_idx_to_jack(codec, per_pin)
> > {
> > 	if (spec->dyn_pcm_assign) {
> > 		if (per_pin->pcm_idx < 0)
> > 			return NULL;
> > 		return spec->pcm_rec[per_pin->pcm_idx].jack;
> > 	} else {
> > 		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> > >pin_nid);
> > 		if (!jack_tbl)
> > 			return NULL;
> > 		return jack_tbl->jack;
> > 	}
> > }
> > 
> > Then the code update_eld() will be cleaner,
> > 
> > 	jack = pin_to_jack(codec, per_pin);
> > 	update_eld(codec, per_pin, eld);
> > 	if (!jack)
> > 		jack = pin_to_jack(codec, per_pin);
> > 	if (!jack)
> > 		goto unlock;
> > 
> > Alternatively, assign pcm_rec[].jack beforehand for non-dyn PCM.
> > It's fixed, so you can assign it in initialization.
> 
> Get it. So your patch will be merged to for-next soon? Thanks.

I *would* do, but I won't write it but wait for your renewed patch ;)


Takashi

Takashi Iwai Feb. 18, 2016, 8:08 a.m. UTC | #7

On Thu, 18 Feb 2016 09:06:47 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Thursday, February 18, 2016 3:55 PM
> > To: Yang, Libin
> > Cc: [e-mail]; [e-mail]; Lin,
> > Mengdong
> > Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> > not dyn_pcm_assign
> > 
> > On Thu, 18 Feb 2016 07:16:29 +0100,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > > I submitted the patch which can fix a regression involved from my
> > > jack patch:
> > > commit 25e4abb33df3aafa7d1efba8f82f9178268efab1
> > > Author: Libin Yang <[e-mail]>
> > > Date:   Tue Jan 12 11:13:27 2016 +0800
> > >
> > >     ALSA: hda - hdmi jack created based on pcm
> > >
> > > I'm sorry for the inconvenience.
> > >
> > > As the dyn pcm patches are merged into the for-next branch, I'm asking
> > > our QA to do the full test on it. They said they can start the stress test
> > > from next week.
> > 
> > One concern is the report from CI test about the crash at unloading
> > the module.  I couldn't reproduce it locally.  Could you take a look?
> 
> There is the same oops when unloading snd modules on my side
> (it happens occasionally).
> With the patch, the oops is fixed. But I didn't meet the crash. 
> The system is still alive.

Hmm.  I wonder it because the report was done for HSW, so your patch
(I suppose you meaning the patch 'ALSA: hda - hdmi get jack from
hda_jack_tbl when not dyn_pcm_assign') shouldn't change anything.


Takashi

Yang, Libin Feb. 18, 2016, 8:11 a.m. UTC | #8

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Thursday, February 18, 2016 4:07 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; Lin,
> Mengdong
> Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> not dyn_pcm_assign
> 
> On Thu, 18 Feb 2016 09:02:24 +0100,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:[e-mail]]
> > > Sent: Thursday, February 18, 2016 3:54 PM
> > > To: [e-mail]
> > > Cc: [e-mail]; Lin, Mengdong; Yang, Libin
> > > Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl
> when
> > > not dyn_pcm_assign
> > >
> > > On Thu, 18 Feb 2016 06:25:02 +0100,
> > > [e-mail] wrote:
> > > >
> > > > From: Libin Yang <[e-mail]>
> > > >
> > > > On Intel platform, if !dyn_pcm_assign, spec->pcm_rec[].jack is not
> > > > NULL even after snd_hda_jack_tbl_clear() is called to free snd_jack.
> > > > This may cause access invalid memory when calling snd_jack_report.
> > > >
> > > > Please see more detail from:
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=94079
> > > >
> > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > ---
> > > >  sound/pci/hda/patch_hdmi.c | 16 ++++++++++++++--
> > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/sound/pci/hda/patch_hdmi.c
> > > b/sound/pci/hda/patch_hdmi.c
> > > > index f4443b5..3b47101 100644
> > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > @@ -1962,6 +1962,7 @@ static void sync_eld_via_acomp(struct
> > > hda_codec *codec,
> > > >  {
> > > >  	struct hdmi_spec *spec = codec->spec;
> > > >  	struct hdmi_eld *eld = &spec->temp_eld;
> > > > +	struct hda_jack_tbl *jack_tbl;
> > > >  	struct snd_jack *jack = NULL;
> > > >  	int size;
> > > >
> > > > @@ -1989,10 +1990,21 @@ static void
> sync_eld_via_acomp(struct
> > > hda_codec *codec,
> > > >  	/* pcm_idx >=0 before update_eld() means it is in monitor
> > > >  	 * disconnected event. Jack must be fetched before update_eld()
> > > >  	 */
> > > > -	if (per_pin->pcm_idx >= 0)
> > > > +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> > > > +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> > > > +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> > > > +	 * free snd_jack. This may cause access invalid memory
> > > > +	 * when calling snd_jack_report
> > > > +	 */
> > > > +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> > > >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > > > +	else {
> > > > +		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> > > >pin_nid);
> > > > +		if (jack_tbl)
> > > > +			jack = jack_tbl->jack;
> > > > +	}
> > > >  	update_eld(codec, per_pin, eld);
> > > > -	if (jack == NULL && per_pin->pcm_idx >= 0)
> > > > +	if (jack == NULL && per_pin->pcm_idx >= 0 && spec-
> > > >dyn_pcm_assign)
> > > >  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> > > >  	if (jack == NULL)
> > > >  		goto unlock;
> > >
> > > I'd create a separate small helper function, e.g. pin_to_jack()
> > > doing like:
> > >
> > > pin_idx_to_jack(codec, per_pin)
> > > {
> > > 	if (spec->dyn_pcm_assign) {
> > > 		if (per_pin->pcm_idx < 0)
> > > 			return NULL;
> > > 		return spec->pcm_rec[per_pin->pcm_idx].jack;
> > > 	} else {
> > > 		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> > > >pin_nid);
> > > 		if (!jack_tbl)
> > > 			return NULL;
> > > 		return jack_tbl->jack;
> > > 	}
> > > }
> > >
> > > Then the code update_eld() will be cleaner,
> > >
> > > 	jack = pin_to_jack(codec, per_pin);
> > > 	update_eld(codec, per_pin, eld);
> > > 	if (!jack)
> > > 		jack = pin_to_jack(codec, per_pin);
> > > 	if (!jack)
> > > 		goto unlock;
> > >
> > > Alternatively, assign pcm_rec[].jack beforehand for non-dyn PCM.
> > > It's fixed, so you can assign it in initialization.
> >
> > Get it. So your patch will be merged to for-next soon? Thanks.
> 
> I *would* do, but I won't write it but wait for your renewed patch ;)

I misunderstand that you have already written such patch.

I will refine the patch. :)

Regards,
Libin

> 
> 
> Takashi

Yang, Libin Feb. 18, 2016, 8:23 a.m. UTC | #9

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Thursday, February 18, 2016 4:09 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; Lin,
> Mengdong
> Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> not dyn_pcm_assign
> 
> On Thu, 18 Feb 2016 09:06:47 +0100,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:[e-mail]]
> > > Sent: Thursday, February 18, 2016 3:55 PM
> > > To: Yang, Libin
> > > Cc: [e-mail]; [e-mail]; Lin,
> > > Mengdong
> > > Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl
> when
> > > not dyn_pcm_assign
> > >
> > > On Thu, 18 Feb 2016 07:16:29 +0100,
> > > Yang, Libin wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > > I submitted the patch which can fix a regression involved from my
> > > > jack patch:
> > > > commit 25e4abb33df3aafa7d1efba8f82f9178268efab1
> > > > Author: Libin Yang <[e-mail]>
> > > > Date:   Tue Jan 12 11:13:27 2016 +0800
> > > >
> > > >     ALSA: hda - hdmi jack created based on pcm
> > > >
> > > > I'm sorry for the inconvenience.
> > > >
> > > > As the dyn pcm patches are merged into the for-next branch, I'm
> asking
> > > > our QA to do the full test on it. They said they can start the stress
> test
> > > > from next week.
> > >
> > > One concern is the report from CI test about the crash at unloading
> > > the module.  I couldn't reproduce it locally.  Could you take a look?
> >
> > There is the same oops when unloading snd modules on my side
> > (it happens occasionally).
> > With the patch, the oops is fixed. But I didn't meet the crash.
> > The system is still alive.
> 
> Hmm.  I wonder it because the report was done for HSW, so your patch
> (I suppose you meaning the patch 'ALSA: hda - hdmi get jack from
> hda_jack_tbl when not dyn_pcm_assign') shouldn't change anything.

If so, my patch may not help.

BTW: could you tell me how you know that the report was done 
from the dmesg?

> 
> 
> Takashi

Takashi Iwai Feb. 18, 2016, 9:03 a.m. UTC | #10

On Thu, 18 Feb 2016 09:23:43 +0100,
Yang, Libin wrote:
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Thursday, February 18, 2016 4:09 PM
> > To: Yang, Libin
> > Cc: [e-mail]; [e-mail]; Lin,
> > Mengdong
> > Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl when
> > not dyn_pcm_assign
> > 
> > On Thu, 18 Feb 2016 09:06:47 +0100,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:[e-mail]]
> > > > Sent: Thursday, February 18, 2016 3:55 PM
> > > > To: Yang, Libin
> > > > Cc: [e-mail]; [e-mail]; Lin,
> > > > Mengdong
> > > > Subject: Re: [PATCH] ALSA: hda - hdmi get jack from hda_jack_tbl
> > when
> > > > not dyn_pcm_assign
> > > >
> > > > On Thu, 18 Feb 2016 07:16:29 +0100,
> > > > Yang, Libin wrote:
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > > I submitted the patch which can fix a regression involved from my
> > > > > jack patch:
> > > > > commit 25e4abb33df3aafa7d1efba8f82f9178268efab1
> > > > > Author: Libin Yang <[e-mail]>
> > > > > Date:   Tue Jan 12 11:13:27 2016 +0800
> > > > >
> > > > >     ALSA: hda - hdmi jack created based on pcm
> > > > >
> > > > > I'm sorry for the inconvenience.
> > > > >
> > > > > As the dyn pcm patches are merged into the for-next branch, I'm
> > asking
> > > > > our QA to do the full test on it. They said they can start the stress
> > test
> > > > > from next week.
> > > >
> > > > One concern is the report from CI test about the crash at unloading
> > > > the module.  I couldn't reproduce it locally.  Could you take a look?
> > >
> > > There is the same oops when unloading snd modules on my side
> > > (it happens occasionally).
> > > With the patch, the oops is fixed. But I didn't meet the crash.
> > > The system is still alive.
> > 
> > Hmm.  I wonder it because the report was done for HSW, so your patch
> > (I suppose you meaning the patch 'ALSA: hda - hdmi get jack from
> > hda_jack_tbl when not dyn_pcm_assign') shouldn't change anything.
> 
> If so, my patch may not help.
> 
> BTW: could you tell me how you know that the report was done 
> from the dmesg?

Gabriel posted lspci output, too:

$lspci -nn
00:03.0 Audio device [0403]: Intel Corporation Xeon E3-1200 v3/4th Gen 
Core Processor HD Audio Controller [8086:0c0c] (rev 06)
00:1b.0 Audio device [0403]: Intel Corporation 8 Series/C220 Series 
Chipset High Definition Audio Controller [8086:8c20] (rev 05)


Takashi

Patch

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index f4443b5..3b47101 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1962,6 +1962,7 @@  static void sync_eld_via_acomp(struct hda_codec *codec,
 {
 	struct hdmi_spec *spec = codec->spec;
 	struct hdmi_eld *eld = &spec->temp_eld;
+	struct hda_jack_tbl *jack_tbl;
 	struct snd_jack *jack = NULL;
 	int size;
 
@@ -1989,10 +1990,21 @@  static void sync_eld_via_acomp(struct hda_codec *codec,
 	/* pcm_idx >=0 before update_eld() means it is in monitor
 	 * disconnected event. Jack must be fetched before update_eld()
 	 */
-	if (per_pin->pcm_idx >= 0)
+	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
+	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
+	 * NULL even after snd_hda_jack_tbl_clear() is called to
+	 * free snd_jack. This may cause access invalid memory
+	 * when calling snd_jack_report
+	 */
+	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
 		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
+	else {
+		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
+		if (jack_tbl)
+			jack = jack_tbl->jack;
+	}
 	update_eld(codec, per_pin, eld);
-	if (jack == NULL && per_pin->pcm_idx >= 0)
+	if (jack == NULL && per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
 		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
 	if (jack == NULL)
 		goto unlock;