hda - fail ELD reading early
PATCH 1/1 hda - fail ELD reading early
With the ELD repoll mechanism, we can (and should) fail the ELD reading immediately when find something obviously wrong and let the caller retry after some delay.
--- sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)
sound/pci/hda/hda_eld.c +19 -9
@@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *elfor (i =0 ; i < size; i++) {unsigned int val =hdmi_get_eld_data (codec, nid, i); +/* +* Graphics driver might be writing to ELD buffer right now. +* Just abort. The caller will repoll after a while. +*/ if (!(val &AC_ELDD_ELD_VALID )) { -if (!i) { -snd_printd (KERN_INFO -"HDMI: invalid ELD data\n" ); - ret = -EINVAL ; - goto error; - }snd_printd (KERN_INFO "HDMI: invalid ELD data byte %d\n" , i); - val =0 ; - }else - val &=AC_ELDD_ELD_DATA ; + ret = -EINVAL ; + goto error; + } + val &=AC_ELDD_ELD_DATA ; +/* +* The first byte cannot be zero. This can happen on some DVI +* connections. Some Intel chips may also need some 250ms delay +* to return non-zero ELD data, even when the graphics driver +* correctly writes ELD content before setting ELD_valid bit. +*/ +if (!val && !i) { +snd_printdd (KERN_INFO "HDMI: 0 ELD data\n" ); + ret = -EINVAL ; + goto error; + } buf[i] = val; }
At Tue, 22 Nov 2011 16:46:23 +0800,
Quoted 37 lines · Wu Fengguang
With the ELD repoll mechanism, we can (and should) fail the ELD reading immediately when find something obviously wrong and let the caller retry after some delay.
Signed-off-by Wu Fengguang <[email protected]>--- sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)sound/pci/hda/hda_eld.c +9
-9@@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *elfor (i =0 ; i < size; i++) {unsigned int val =hdmi_get_eld_data (codec, nid, i); +/* +* Graphics driver might be writing to ELD buffer right now. +* Just abort. The caller will repoll after a while. +*/ if (!(val &AC_ELDD_ELD_VALID )) { -if (!i) { -snd_printd (KERN_INFO -"HDMI: invalid ELD data\n" ); - ret = -EINVAL ; - goto error; - }snd_printd (KERN_INFO "HDMI: invalid ELD data byte %d\n" , i); - val =0 ; - }else - val &=AC_ELDD_ELD_DATA ; + ret = -EINVAL ; + goto error; + } + val &=AC_ELDD_ELD_DATA ; +/*
sound/pci/hda/hda_eld.c int snd_hdmi_get_eld(struct hdmi_eld *el · line 371
Quoted 10 lines
sound/pci/hda/hda_eld.c +10
-0+* The first byte cannot be zero. This can happen on some DVI +* connections. Some Intel chips may also need some 250ms delay +* to return non-zero ELD data, even when the graphics driver +* correctly writes ELD content before setting ELD_valid bit. +*/ +if (!val && !i) { +snd_printdd (KERN_INFO "HDMI: 0 ELD data\n" ); + ret = -EINVAL ; + goto error; + }
Shouldn't this zero-check be before the valid-bit check? Otherwise it'll never reach there.
thanks,
Takashi
Quoted 44 lines · Takashi Iwai
At Tue, 22 Nov 2011 16:46:23 +0800,
Quoted 42 lines · Wu Fengguang
With the ELD repoll mechanism, we can (and should) fail the ELD reading immediately when find something obviously wrong and let the caller retry after some delay.
Signed-off-by Wu Fengguang <[email protected]>--- sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)sound/pci/hda/hda_eld.c +14
-9@@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *elfor (i =0 ; i < size; i++) {unsigned int val =hdmi_get_eld_data (codec, nid, i); +/* +* Graphics driver might be writing to ELD buffer right now. +* Just abort. The caller will repoll after a while. +*/ if (!(val &AC_ELDD_ELD_VALID )) { -if (!i) { -snd_printd (KERN_INFO -"HDMI: invalid ELD data\n" ); - ret = -EINVAL ; - goto error; - }snd_printd (KERN_INFO "HDMI: invalid ELD data byte %d\n" , i); - val =0 ; - }else - val &=AC_ELDD_ELD_DATA ; + ret = -EINVAL ; + goto error; + } + val &=AC_ELDD_ELD_DATA ; +/* +* The first byte cannot be zero. This can happen on some DVI +* connections. Some Intel chips may also need some 250ms delay +* to return non-zero ELD data, even when the graphics driver +* correctly writes ELD content before setting ELD_valid bit. +*/
Quoted 8 lines
sound/pci/hda/hda_eld.c int snd_hdmi_get_eld(struct hdmi_eld *el · line 371
Quoted 5 lines
sound/pci/hda/hda_eld.c +5
-0+if (!val && !i) { +snd_printdd (KERN_INFO "HDMI: 0 ELD data\n" ); + ret = -EINVAL ; + goto error; + }Shouldn't this zero-check be before the valid-bit check? Otherwise it'll never reach there.
It does reach there:
[ 1191.016746] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[ 1191.019309] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
==> [ 1191.021803] ALSA hda_eld.c:368 HDMI: 0 ELD data
[ 1191.324661] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[ 1191.333236] HDMI: detected monitor SONY TV at connection type HDMI
[ 1191.335020] HDMI: available speakers: FL/FR
[ 1191.335996] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, bits = 16 20 24The funny thing is, it's reporting (invalid) 0 ELD data that has the AC_ELDD_ELD_VALID bit set.
Thanks,
Fengguang
At Tue, 22 Nov 2011 18:53:18 +0800,
Quoted 65 lines · Wu Fengguang
Quoted 44 lines · Takashi Iwai
At Tue, 22 Nov 2011 16:46:23 +0800,
Quoted 42 lines · Wu Fengguang
With the ELD repoll mechanism, we can (and should) fail the ELD reading immediately when find something obviously wrong and let the caller retry after some delay.
Signed-off-by Wu Fengguang <[email protected]>--- sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)sound/pci/hda/hda_eld.c +14
-9@@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *elfor (i =0 ; i < size; i++) {unsigned int val =hdmi_get_eld_data (codec, nid, i); +/* +* Graphics driver might be writing to ELD buffer right now. +* Just abort. The caller will repoll after a while. +*/ if (!(val &AC_ELDD_ELD_VALID )) { -if (!i) { -snd_printd (KERN_INFO -"HDMI: invalid ELD data\n" ); - ret = -EINVAL ; - goto error; - }snd_printd (KERN_INFO "HDMI: invalid ELD data byte %d\n" , i); - val =0 ; - }else - val &=AC_ELDD_ELD_DATA ; + ret = -EINVAL ; + goto error; + } + val &=AC_ELDD_ELD_DATA ; +/* +* The first byte cannot be zero. This can happen on some DVI +* connections. Some Intel chips may also need some 250ms delay +* to return non-zero ELD data, even when the graphics driver +* correctly writes ELD content before setting ELD_valid bit. +*/ Quoted 8 lines
sound/pci/hda/hda_eld.c int snd_hdmi_get_eld(struct hdmi_eld *el · line 371
Quoted 5 lines
sound/pci/hda/hda_eld.c +5
-0+if (!val && !i) { +snd_printdd (KERN_INFO "HDMI: 0 ELD data\n" ); + ret = -EINVAL ; + goto error; + }Shouldn't this zero-check be before the valid-bit check? Otherwise it'll never reach there.
It does reach there:
[ 1191.016746] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 1191.019309] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 ==> [ 1191.021803] ALSA hda_eld.c:368 HDMI: 0 ELD data [ 1191.324661] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 1191.333236] HDMI: detected monitor SONY TV at connection type HDMI [ 1191.335020] HDMI: available speakers: FL/FR [ 1191.335996] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, bits = 16 20 24
Quoted 2 lines
The funny thing is, it's reporting (invalid) 0 ELD data that has the
AC_ELDD_ELD_VALIDbit set.
Ah OK. Another slight concern is that your patch gives always the error when ELD_VALID isn't set, so it makes the check more strict in practice. I guess it'd be OK, so I'll take it in. But we need to carefully hear whether anyone cries with this.
thanks,
Takashi
Quoted 67 lines · Takashi Iwai
At Tue, 22 Nov 2011 18:53:18 +0800,
Quoted 65 lines · Wu Fengguang
Quoted 44 lines · Takashi Iwai
At Tue, 22 Nov 2011 16:46:23 +0800,
Quoted 42 lines · Wu Fengguang
With the ELD repoll mechanism, we can (and should) fail the ELD reading immediately when find something obviously wrong and let the caller retry after some delay.
Signed-off-by Wu Fengguang <[email protected]>--- sound/pci/hda/hda_eld.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)sound/pci/hda/hda_eld.c +14
-9@@ -347,18 +347,28 @@ int snd_hdmi_get_eld(struct hdmi_eld *elfor (i =0 ; i < size; i++) {unsigned int val =hdmi_get_eld_data (codec, nid, i); +/* +* Graphics driver might be writing to ELD buffer right now. +* Just abort. The caller will repoll after a while. +*/ if (!(val &AC_ELDD_ELD_VALID )) { -if (!i) { -snd_printd (KERN_INFO -"HDMI: invalid ELD data\n" ); - ret = -EINVAL ; - goto error; - }snd_printd (KERN_INFO "HDMI: invalid ELD data byte %d\n" , i); - val =0 ; - }else - val &=AC_ELDD_ELD_DATA ; + ret = -EINVAL ; + goto error; + } + val &=AC_ELDD_ELD_DATA ; +/* +* The first byte cannot be zero. This can happen on some DVI +* connections. Some Intel chips may also need some 250ms delay +* to return non-zero ELD data, even when the graphics driver +* correctly writes ELD content before setting ELD_valid bit. +*/ Quoted 8 lines
sound/pci/hda/hda_eld.c int snd_hdmi_get_eld(struct hdmi_eld *el · line 371
Quoted 5 lines
sound/pci/hda/hda_eld.c +5
-0+if (!val && !i) { +snd_printdd (KERN_INFO "HDMI: 0 ELD data\n" ); + ret = -EINVAL ; + goto error; + }Shouldn't this zero-check be before the valid-bit check? Otherwise it'll never reach there.
It does reach there:
[ 1191.016746] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 1191.019309] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 ==> [ 1191.021803] ALSA hda_eld.c:368 HDMI: 0 ELD data [ 1191.324661] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 1191.333236] HDMI: detected monitor SONY TV at connection type HDMI [ 1191.335020] HDMI: available speakers: FL/FR [ 1191.335996] HDMI: supports coding type LPCM: channels = 2, rates = 32000 44100 48000, bits = 16 20 24
Quoted 7 lines
Quoted 2 lines
The funny thing is, it's reporting (invalid) 0 ELD data that has the
AC_ELDD_ELD_VALIDbit set.Ah OK. Another slight concern is that your patch gives always the error when
ELD_VALIDisn't set, so it makes the check more strict in practice. I guess it'd be OK, so I'll take it in. But we need to carefully hear whether anyone cries with this.
Agreed, the original code simply sets data to 0 when !ELD_VALID. If the hardware does the illogical thing (who knows) of conditional setting !ELD_VALID on some parts of the ELD buffer which happen to contain 0, we'll be in bad luck...
Thanks,
Fengguang