hda - fix ELD memory leak

sound/pci/hda/patch_hdmi.c +5 -0

+			queue_delayed_work(codec->bus->workq,
+					   &eld->work,
+					   msecs_to_jiffies(300));
+		}
+	}

sound/pci/hda/patch_hdmi.c +7 -0

+	struct hdmi_spec_per_pin *per_pin =
+			container_of(eld, struct hdmi_spec_per_pin, sink_eld);
+	struct hda_codec *codec = eld->codec;
+
+	hdmi_present_sense(codec, per_pin->pin_nid, eld, false);
+}
+
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;

At Wed, 16 Nov 2011 00:57:08 +0800,

Review

Quoted 5 lines · Wu Fengguang

(quoted diff) +5 -0

+static void hda_eld_work(struct work_struct *work)
+{
+	struct hdmi_eld *eld = container_of(
+				container_of(work, struct delayed_work, work),
+				struct hdmi_eld, work);

Ugh, hardly to read. Slightly better to use to_delayed_work()?

	struct hdmi_eld *eld = container_of(to_delayed_work(work),
					    struct hdmi_eld, work);

Quoted 16 lines

(quoted diff) +8 -1

+	struct hdmi_spec_per_pin *per_pin =
+			container_of(eld, struct hdmi_spec_per_pin, sink_eld);
+	struct hda_codec *codec = eld->codec;
+
+	hdmi_present_sense(codec, per_pin->pin_nid, eld, false);
+}
+
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;

@@ -1227,7 +1245,7 @@ static int generic_hdmi_build_jack(struc
 	if (err < 0)
 		return err;
 
-	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld);
+	hdmi_present_sense(codec, per_pin->pin_nid, &per_pin->sink_eld, false);

...

Looking through the code, I wonder how about keeping work and codec pointer in struct hdmi_spec_per_pin instead of struct hdmi_eld. Then we don't need to expose it in hda_local.h, and changes would be slightly more compact.

hdmi_present_sense() can be modified to receive hdmi_sepc_per_pin* directly instead of individual nid and eld pointer.

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Signed-off-by Wu Fengguang <[email protected]>

---
 sound/pci/hda/hda_local.h  |    2 +
 sound/pci/hda/patch_hdmi.c |   49 ++++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 7 deletions(-)

sound/pci/hda/hda_local.h +2 -0

@@ -655,6 +655,8 @@ struct hdmi_eld {
 #ifdef CONFIG_PROC_FS
 	struct snd_info_entry *proc_entry;
 #endif
+	struct hda_codec *codec;
+	struct delayed_work work;
 };
 
 int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid);

sound/pci/hda/patch_hdmi.c +3 -3

@@ -746,7 +746,7 @@ static void hdmi_setup_audio_infoframe(s
  */
 
 static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld);
+			       struct hdmi_eld *eld, bool retry);
 
 static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 {

@@ -766,7 +766,7 @@ static void hdmi_intrinsic_event(struct 
 		return;
 	eld = &spec->pins[pin_idx].sink_eld;
 
-	hdmi_present_sense(codec, pin_nid, eld);
+	hdmi_present_sense(codec, pin_nid, eld, true);
 
 	/*
 	 * HDMI sink's ELD info cannot always be retrieved for now, e.g.

@@ -969,7 +969,7 @@ static int hdmi_read_pin_conn(struct hda
 }
 
 static void hdmi_present_sense(struct hda_codec *codec, hda_nid_t pin_nid,
-			       struct hdmi_eld *eld)
+			       struct hdmi_eld *eld, bool retry)
 {
 	/*
 	 * Always execute a GetPinSense verb here, even when called from

@@ -992,13 +992,31 @@ static void hdmi_present_sense(struct hd
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
 		codec->addr, pin_nid, eld->monitor_present, eld_valid);

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:

Quoted 5 lines

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Any idea why; is the graphics driver writing the ELD data to the audio HW after triggering the unsolicited even rather than before, or something like that?

250mS almost sounds like it's setting ELDV in the audio HW, then going and reading the EDID, then writing the EDID to the audio HW; perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's meant to be setting just PRESENT, and later setting ELDV?

From the debug dmesg, I'm pretty sure that the ELDV events are

Quoted 10 lines

Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:

Quoted 5 lines

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Any idea why; is the graphics driver writing the ELD data to the audio HW after triggering the unsolicited even rather than before, or something like that?

Nope. The graphics driver is doing

eld_valid = 0
write to eld buffer
eld_valid = 1

Quoted 4 lines

250mS almost sounds like it's setting ELDV in the audio HW, then going and reading the EDID, then writing the EDID to the audio HW; perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's meant to be setting just PRESENT, and later setting ELDV?

From the debug dmesg, I'm pretty sure that the ELDV events are triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register writes. Since the ELD data is already prepared, there is no EDID read in between.

Below is the dmesg representing a video mode set.

ELD writes from the graphics driver

[  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
[  424.257670] [drm:ironlake_write_eld], ELD on pipe B
[  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
[  424.262156] [drm:ironlake_write_eld], ELD size 13

ELD events received by audio driver (eld reads 0)

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

[  424.272415] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
[  424.274566] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.277027] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.283157] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0

graphics driver go on with its job

[  424.314331] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.367183] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.368960] [drm:ironlake_update_wm], FIFO watermarks For pipe A - plane 5, cursor: 6
[  424.370700] [drm:ironlake_update_wm], FIFO watermarks For pipe B - plane 42, cursor: 6
[  424.424056] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.476906] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.479169] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x100
[  424.481084] [drm:ironlake_fdi_link_train], FDI train 1 done.
[  424.483452] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x600
[  424.485444] [drm:ironlake_fdi_link_train], FDI train 2 done.
[  424.486765] [drm:ironlake_fdi_link_train], FDI train done
[  424.490820] [drm:intel_update_fbc],
[  424.491841] [drm:drm_crtc_helper_set_config], Setting connector DPMS state to on
[  424.494449] [drm:drm_crtc_helper_set_config],        [CONNECTOR:12:HDMI-A-2] set DPMS on
[  424.505904] [drm:intel_prepare_page_flip], preparing flip with no unpin work?

audio driver repoll the ELD after 300ms (eld data readable now)

[  424.574763] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.580867] HDMI: detected monitor RX-V1800 at connection type HDMI
[  424.582413] HDMI: available speakers: FL/FR LFE FC RL/RR RC RLC/RRC

Quoted 15 lines

Quoted 5 lines

250mS almost sounds like it's setting ELDV in the audio HW, then going and reading the EDID, then writing the EDID to the audio HW; perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's meant to be setting just PRESENT, and later setting ELDV?

From the debug dmesg, I'm pretty sure that the ELDV events are triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register writes. Since the ELD data is already prepared, there is no EDID read in between.

Below is the dmesg representing a video mode set.

ELD writes from the graphics driver

Quoted 8 lines

[  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
[  424.257670] [drm:ironlake_write_eld], ELD on pipe B
[  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
[  424.262156] [drm:ironlake_write_eld], ELD size 13

ELD events received by audio driver (eld reads 0)

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

That line makes sense.

Quoted 1 line

[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

I don't /think/ it's related to this issue, but I wonder why ELDV==1 in that message; it seems that the unsolicited response contains the correct data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

At Wed, 16 Nov 2011 07:51:28 -0800, Stephen Warren wrote:

Quoted 4 lines

Quoted 1 line

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

That line makes sense.

Quoted 5 lines

Quoted 1 line

[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

I don't /think/ it's related to this issue, but I wonder why ELDV==1 in that message; it seems that the unsolicited response contains the correct data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

Note that this bit isn't from GET_PIN_SENSE verb but the bit in the unsol event argument.

Takashi Iwai wrote at Wednesday, November 16, 2011 8:58 AM:

Quoted 1 line

At Wed, 16 Nov 2011 07:51:28 -0800, Stephen Warren wrote:

...

Quoted 10 lines

Quoted 9 lines

Quoted 1 line

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

That line makes sense.

Quoted 1 line

[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

I don't /think/ it's related to this issue, but I wonder why ELDV==1 in that message; it seems that the unsolicited response contains the correct data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

Quoted 2 lines

Note that this bit isn't from GET_PIN_SENSE verb but the bit in the unsol event argument.

The first message above prints data from the unsolicited event. The second message above is derived from the GET_PIN_SENSE verb.

Wu Fengguang wrote at Tuesday, November 15, 2011 7:48 PM:

Quoted 12 lines · Stephen Warren

Quoted 10 lines

Wu Fengguang wrote at Tuesday, November 15, 2011 7:33 AM:

Quoted 5 lines

The Intel HDMI chips (ironlake at least) are found to have ~250ms delay between the ELD_Valid=1 hotplug event is send and the ELD buffer becomes actually readable. During the time the ELD buffer is mysteriously all 0.

Fix it by scheduling a delayed work to re-read ELD buffer after 300ms.

Any idea why; is the graphics driver writing the ELD data to the audio HW after triggering the unsolicited even rather than before, or something like that?

Quoted 5 lines

Nope. The graphics driver is doing

eld_valid = 0
write to eld buffer
eld_valid = 1

OK, that sounds fine.

Quoted 14 lines

Quoted 4 lines

250mS almost sounds like it's setting ELDV in the audio HW, then going and reading the EDID, then writing the EDID to the audio HW; perhaps the graphics driver is accidentally setting PRESENT+ELDV when it's meant to be setting just PRESENT, and later setting ELDV?

From the debug dmesg, I'm pretty sure that the ELDV events are triggered exactly by the "eld_valid = 0" and "eld_valid = 1" register writes. Since the ELD data is already prepared, there is no EDID read in between.

Below is the dmesg representing a video mode set.

ELD writes from the graphics driver

Quoted 8 lines

[  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
[  424.257670] [drm:ironlake_write_eld], ELD on pipe B
[  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
[  424.262156] [drm:ironlake_write_eld], ELD size 13

ELD events received by audio driver (eld reads 0)

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

That line makes sense.

Quoted 1 line

[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

I don't /think/ it's related to this issue, but I wonder why ELDV==1 in that message; it seems that the unsolicited response contains the correct data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

Quoted 25 lines

[  424.272415] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0
[  424.274566] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.277027] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.283157] ALSA hda_eld.c:259 HDMI: Unknown ELD version 0

graphics driver go on with its job

[  424.314331] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.367183] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.368960] [drm:ironlake_update_wm], FIFO watermarks For pipe A - plane 5, cursor: 6
[  424.370700] [drm:ironlake_update_wm], FIFO watermarks For pipe B - plane 42, cursor: 6
[  424.424056] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.476906] [drm:intel_wait_for_vblank], vblank wait timed out
[  424.479169] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x100
[  424.481084] [drm:ironlake_fdi_link_train], FDI train 1 done.
[  424.483452] [drm:ironlake_fdi_link_train], FDI_RX_IIR 0x600
[  424.485444] [drm:ironlake_fdi_link_train], FDI train 2 done.
[  424.486765] [drm:ironlake_fdi_link_train], FDI train done
[  424.490820] [drm:intel_update_fbc],
[  424.491841] [drm:drm_crtc_helper_set_config], Setting connector DPMS state to on
[  424.494449] [drm:drm_crtc_helper_set_config],        [CONNECTOR:12:HDMI-A-2] set DPMS on
[  424.505904] [drm:intel_prepare_page_flip], preparing flip with no unpin work?

audio driver repoll the ELD after 300ms (eld data readable now)

Quoted 3 lines

[  424.574763] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1
[  424.580867] HDMI: detected monitor RX-V1800 at connection type HDMI
[  424.582413] HDMI: available speakers: FL/FR LFE FC RL/RR RC RLC/RRC

...

OK, well I guess this make it work, and I don't think the patch will cause any issues on HW without this issue, so the patch is fine. I'd still love to understand why this is happening though.

Quoted 13 lines

Quoted 4 lines

Below is the dmesg representing a video mode set.

ELD writes from the graphics driver

Quoted 8 lines

[  424.254958] [drm:intel_write_eld], ELD on [CONNECTOR:12:HDMI-A-2], [ENCODER:11:TMDS-11]
[  424.257670] [drm:ironlake_write_eld], ELD on pipe B
[  424.259833] [drm:ironlake_write_eld], Audio directed to unknown port
[  424.262156] [drm:ironlake_write_eld], ELD size 13

ELD events received by audio driver (eld reads 0)

[  424.263258] HDMI hot plug event: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=0

Quoted 7 lines

That line makes sense.

Quoted 1 line

[  424.265877] HDMI status: Codec=3 Pin=6 Presence_Detect=1 ELD_Valid=1

I don't /think/ it's related to this issue, but I wonder why ELDV==1 in that message; it seems that the unsolicited response contains the correct data, but AC_VERB_GET_PIN_SENSE contains ELDV==1 all the time. That's odd.

It depends on timing. When audio driver receives the unsolicited event, graphics driver has finished with "eld_valid = 1", hence AC_VERB_GET_PIN_SENSE returns ELDV=1.

It's not happening /all the time/ though. For example here is another dmesg showing a different timing on the same test box.

[  467.561516] [drm:intel_dp_mode_set], Enabling DP audio on pipe B
[  467.567503] [drm:intel_write_eld], ELD on [CONNECTOR:26:DP-3], [ENCODER:27:TMDS-27]
[  467.574207] [drm:ironlake_write_eld], ELD on pipe B
[  467.579346] [drm:ironlake_write_eld], Audio directed to unknown port
[  467.584724] [drm:ironlake_write_eld], ELD: DisplayPort detected
[  467.586540] HDMI hot plug event: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=0
[  467.599608] [drm:ironlake_write_eld],                    
[  467.599922] HDMI status: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=0
[  467.605834] ELD size 9                                           
[  467.610434] [drm:sandybridge_update_wm], FIFO watermarks For pipe A - plane 7, cursor: 6
[  467.612365] HDMI hot plug event: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=1
[  467.620654] [drm:sandybridge_update_wm],                 
[  467.620765] HDMI status: Codec=3 Pin=7 Presence_Detect=1 ELD_Valid=1

memset(eld) clears eld->proc_entry which will leak the struct snd_info_entry when unloading the module.

Fix it by
- remove memset(eld)
- set eld->eld_valid to true _after_ all eld fields have been filled
- don't access the other eld fields when (eld->eld_valid == false)

Signed-off-by Wu Fengguang <[email protected]>

---
 sound/pci/hda/hda_eld.c    |    5 +----
 sound/pci/hda/patch_hdmi.c |   11 +++++------
 2 files changed, 6 insertions(+), 10 deletions(-)

sound/pci/hda/hda_eld.c +1 -4

@@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
 					buf + ELD_FIXED_BYTES + mnl + 3 * i);
 	}
 
+	e->eld_valid = true;
 	return 0;
 
 out_fail:
-	e->eld_ver = 0;
 	return -EINVAL;
 }
 

@@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
 	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
 	 */
 
-	if (!eld->eld_valid)
-		return -ENOENT;
-
 	size = snd_hdmi_get_eld_size(codec, nid);
 	if (size == 0) {
 		/* wfg: workaround for ASUS P5E-VM HDMI board */

sound/pci/hda/patch_hdmi.c +5 -6

@@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present = snd_hda_pin_sense(codec, pin_nid);
+	bool eld_valid = false;
 
-	memset(eld, 0, sizeof(*eld));
+	eld->eld_valid = false;
 
 	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
 	if (eld->monitor_present)
-		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
-	else
-		eld->eld_valid	= 0;
+		eld_valid	= !!(present & AC_PINSENSE_ELDV);
 
 	printk(KERN_INFO
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
-		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
+		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld->eld_valid)
+	if (eld_valid)
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
 

At Tue, 15 Nov 2011 22:31:55 +0800,

Quoted 9 lines · Wu Fengguang

memset(eld) clears eld->proc_entry which will leak the struct snd_info_entry when unloading the module.

Fix it by
- remove memset(eld)
- set eld->eld_valid to true _after_ all eld fields have been filled
- don't access the other eld fields when (eld->eld_valid == false)

Quoted 1 line

Signed-off-by Wu Fengguang <[email protected]>

This should be send to stable kernel, too, right?
It appears in 3.1, at least.

Quoted 58 lines

---
 sound/pci/hda/hda_eld.c    |    5 +----
 sound/pci/hda/patch_hdmi.c |   11 +++++------
 2 files changed, 6 insertions(+), 10 deletions(-)

sound/pci/hda/hda_eld.c +1 -4

@@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
 					buf + ELD_FIXED_BYTES + mnl + 3 * i);
 	}
 
+	e->eld_valid = true;
 	return 0;
 
 out_fail:
-	e->eld_ver = 0;
 	return -EINVAL;
 }
 

@@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
 	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
 	 */
 
-	if (!eld->eld_valid)
-		return -ENOENT;
-
 	size = snd_hdmi_get_eld_size(codec, nid);
 	if (size == 0) {
 		/* wfg: workaround for ASUS P5E-VM HDMI board */

sound/pci/hda/patch_hdmi.c +5 -6

@@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present = snd_hda_pin_sense(codec, pin_nid);
+	bool eld_valid = false;
 
-	memset(eld, 0, sizeof(*eld));
+	eld->eld_valid = false;
 
 	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
 	if (eld->monitor_present)
-		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
-	else
-		eld->eld_valid	= 0;
+		eld_valid	= !!(present & AC_PINSENSE_ELDV);
 
 	printk(KERN_INFO
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
-		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
+		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld->eld_valid)
+	if (eld_valid)
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
 

Quoted 11 lines · Takashi Iwai

At Tue, 15 Nov 2011 22:31:55 +0800,

Quoted 9 lines · Wu Fengguang

memset(eld) clears eld->proc_entry which will leak the struct snd_info_entry when unloading the module.

Fix it by
- remove memset(eld)
- set eld->eld_valid to true _after_ all eld fields have been filled
- don't access the other eld fields when (eld->eld_valid == false)

Quoted 4 lines

Quoted 1 line

Signed-off-by Wu Fengguang <[email protected]>

This should be send to stable kernel, too, right?
It appears in 3.1, at least.

Yeah. Good point! I'll resend it when everything goes fine.

Thanks,
Fengguang

Quoted 58 lines

Quoted 58 lines

---
 sound/pci/hda/hda_eld.c    |    5 +----
 sound/pci/hda/patch_hdmi.c |   11 +++++------
 2 files changed, 6 insertions(+), 10 deletions(-)

sound/pci/hda/hda_eld.c +1 -4

@@ -297,10 +297,10 @@ static int hdmi_update_eld(struct hdmi_e
 					buf + ELD_FIXED_BYTES + mnl + 3 * i);
 	}
 
+	e->eld_valid = true;
 	return 0;
 
 out_fail:
-	e->eld_ver = 0;
 	return -EINVAL;
 }
 

@@ -323,9 +323,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *el
 	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
 	 */
 
-	if (!eld->eld_valid)
-		return -ENOENT;
-
 	size = snd_hdmi_get_eld_size(codec, nid);
 	if (size == 0) {
 		/* wfg: workaround for ASUS P5E-VM HDMI board */

sound/pci/hda/patch_hdmi.c +5 -6

@@ -980,20 +980,19 @@ static void hdmi_present_sense(struct hd
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present = snd_hda_pin_sense(codec, pin_nid);
+	bool eld_valid = false;
 
-	memset(eld, 0, sizeof(*eld));
+	eld->eld_valid = false;
 
 	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
 	if (eld->monitor_present)
-		eld->eld_valid	= !!(present & AC_PINSENSE_ELDV);
-	else
-		eld->eld_valid	= 0;
+		eld_valid	= !!(present & AC_PINSENSE_ELDV);
 
 	printk(KERN_INFO
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
-		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
+		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
-	if (eld->eld_valid)
+	if (eld_valid)
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
 

At Tue, 15 Nov 2011 22:41:16 +0800,

Quoted 15 lines · Wu Fengguang

Quoted 13 lines · Takashi Iwai

At Tue, 15 Nov 2011 22:31:55 +0800,

Quoted 10 lines · Wu Fengguang

memset(eld) clears eld->proc_entry which will leak the struct snd_info_entry when unloading the module.

Fix it by
- remove memset(eld)
- set eld->eld_valid to true _after_ all eld fields have been filled
- don't access the other eld fields when (eld->eld_valid == false)

Signed-off-by Wu Fengguang <[email protected]>

Quoted 4 lines

Quoted 2 lines

This should be send to stable kernel, too, right?
It appears in 3.1, at least.

Yeah. Good point! I'll resend it when everything goes fine.

Ah, no, you don't need to resend. I'd just need to put Cc to stable in the patch commit log. Then Greg will pick it up automatically when the tree is merged to the upstream.

But of course I need to know beforehand whether the patch is intended to be sent to stable or not.

memset(eld) clears eld->proc_entry which will leak the struct snd_info_entry when unloading the module.

Fix it by
- remove memset(eld)
- set eld->eld_valid to true _after_ all eld fields have been filled
- don't access the other eld fields when (eld->eld_valid == false)

Signed-off-by Wu Fengguang <[email protected]>