[PATCH v4 0/5] Make HDMI ELD usable for hotplug

[PATCH v4 0/5] Make HDMI ELD usable for hotplug

[David Henningsson]

Changes since v3:
 * snd_hdmi_get_eld now does not take a full hdmi_eld struct

David Henningsson (5):
  ALSA: hda - hdmi: ELD shouldn't be valid after unplug
  ALSA: hda - hdmi: Do not expose eld data when eld is invalid
  ALSA: hda - hdmi: Refactor hdmi_eld into parsed_hdmi_eld
  ALSA: hda - hdmi: Protect ELD buffer
  ALSA: hda - hdmi: Notify userspace when ELD control changes

 sound/pci/hda/hda_eld.c    |   52 +++++++++++-----------
 sound/pci/hda/hda_local.h  |   28 +++++++-----
 sound/pci/hda/patch_hdmi.c |  102 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 124 insertions(+), 58 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/5] ALSA: hda - hdmi: ELD shouldn't be valid after unplug

[David Henningsson]

Currently, eld_valid is never set to false, except at kernel module
load time. This patch makes sure that eld is no longer valid when
the cable is (hot-)unplugged.

Cc: stable@kernel.org
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index b9af281b..32adaa6 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1176,6 +1176,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
 		codec->addr, pin_nid, eld->monitor_present, eld_valid);
 
+	eld->eld_valid = false;
 	if (eld_valid) {
 		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
 			snd_hdmi_show_eld(eld);
-- 
1.7.9.5

[PATCH v4 2/5] ALSA: hda - hdmi: Do not expose eld data when eld is invalid

[David Henningsson]

Previously, it was possible to read the eld data of the previous
monitor connected. This should not be allowed.

Also refactor the function slightly.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 32adaa6..7a73dfb 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -343,14 +343,16 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol,
 			struct snd_ctl_elem_info *uinfo)
 {
 	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
-	struct hdmi_spec *spec;
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_eld *eld;
 	int pin_idx;
 
-	spec = codec->spec;
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
 
 	pin_idx = kcontrol->private_value;
-	uinfo->count = spec->pins[pin_idx].sink_eld.eld_size;
+	eld = &spec->pins[pin_idx].sink_eld;
+
+	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
 
 	return 0;
 }
@@ -359,14 +361,21 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
 			struct snd_ctl_elem_value *ucontrol)
 {
 	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
-	struct hdmi_spec *spec;
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_eld *eld;
 	int pin_idx;
 
-	spec = codec->spec;
 	pin_idx = kcontrol->private_value;
+	eld = &spec->pins[pin_idx].sink_eld;
+
+	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
+		snd_BUG();
+		return -EINVAL;
+	}
 
-	memcpy(ucontrol->value.bytes.data,
-		spec->pins[pin_idx].sink_eld.eld_buffer, ELD_MAX_SIZE);
+	if (eld->eld_valid)
+		memcpy(ucontrol->value.bytes.data, eld->eld_buffer,
+		       eld->eld_size);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v4 3/5] ALSA: hda - hdmi: Refactor hdmi_eld into parsed_hdmi_eld

[David Henningsson]

For better readability, the information that is parsed out of the
ELD data is now put into a separate parsed_hdmi_eld struct.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_eld.c    |   46 ++++++++++++++++++++------------------------
 sound/pci/hda/hda_local.h  |   27 ++++++++++++++++----------
 sound/pci/hda/patch_hdmi.c |   28 ++++++++++++++++++---------
 3 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
index 4c054f4..16066d7 100644
--- a/sound/pci/hda/hda_eld.c
+++ b/sound/pci/hda/hda_eld.c
@@ -246,8 +246,8 @@ static void hdmi_update_short_audio_desc(struct cea_sad *a,
 /*
  * Be careful, ELD buf could be totally rubbish!
  */
-static int hdmi_update_eld(struct hdmi_eld *e,
-			   const unsigned char *buf, int size)
+int snd_hdmi_parse_eld(struct parsed_hdmi_eld *e,
+			  const unsigned char *buf, int size)
 {
 	int mnl;
 	int i;
@@ -260,7 +260,6 @@ static int hdmi_update_eld(struct hdmi_eld *e,
 		goto out_fail;
 	}
 
-	e->eld_size = size;
 	e->baseline_len = GRAB_BITS(buf, 2, 0, 8);
 	mnl		= GRAB_BITS(buf, 4, 0, 5);
 	e->cea_edid_ver	= GRAB_BITS(buf, 4, 5, 3);
@@ -305,7 +304,6 @@ static int hdmi_update_eld(struct hdmi_eld *e,
 	if (!e->spk_alloc)
 		e->spk_alloc = 0xffff;
 
-	e->eld_valid = true;
 	return 0;
 
 out_fail:
@@ -318,17 +316,16 @@ int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid)
 						 AC_DIPSIZE_ELD_BUF);
 }
 
-int snd_hdmi_get_eld(struct hdmi_eld *eld,
-		     struct hda_codec *codec, hda_nid_t nid)
+int snd_hdmi_get_eld(struct hda_codec *codec, hda_nid_t nid,
+		     unsigned char *buf, int *eld_size)
 {
 	int i;
 	int ret;
 	int size;
-	unsigned char *buf;
 
 	/*
 	 * ELD size is initialized to zero in caller function. If no errors and
-	 * ELD is valid, actual eld_size is assigned in hdmi_update_eld()
+	 * ELD is valid, actual eld_size is assigned.
 	 */
 
 	size = snd_hdmi_get_eld_size(codec, nid);
@@ -343,8 +340,6 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
 	}
 
 	/* set ELD buffer */
-	buf = eld->eld_buffer;
-
 	for (i = 0; i < size; i++) {
 		unsigned int val = hdmi_get_eld_data(codec, nid, i);
 		/*
@@ -372,8 +367,7 @@ int snd_hdmi_get_eld(struct hdmi_eld *eld,
 		buf[i] = val;
 	}
 
-	ret = hdmi_update_eld(eld, buf, size);
-
+	*eld_size = size;
 error:
 	return ret;
 }
@@ -438,7 +432,7 @@ void snd_print_channel_allocation(int spk_alloc, char *buf, int buflen)
 	buf[j] = '\0';	/* necessary when j == 0 */
 }
 
-void snd_hdmi_show_eld(struct hdmi_eld *e)
+void snd_hdmi_show_eld(struct parsed_hdmi_eld *e)
 {
 	int i;
 
@@ -487,10 +481,11 @@ static void hdmi_print_sad_info(int i, struct cea_sad *a,
 static void hdmi_print_eld_info(struct snd_info_entry *entry,
 				struct snd_info_buffer *buffer)
 {
-	struct hdmi_eld *e = entry->private_data;
+	struct hdmi_eld *eld = entry->private_data;
+	struct parsed_hdmi_eld *e = &eld->info;
 	char buf[SND_PRINT_CHANNEL_ALLOCATION_ADVISED_BUFSIZE];
 	int i;
-	static char *eld_versoin_names[32] = {
+	static char *eld_version_names[32] = {
 		"reserved",
 		"reserved",
 		"CEA-861D or below",
@@ -505,15 +500,15 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 		[4 ... 7] = "reserved"
 	};
 
-	snd_iprintf(buffer, "monitor_present\t\t%d\n", e->monitor_present);
-	snd_iprintf(buffer, "eld_valid\t\t%d\n", e->eld_valid);
-	if (!e->eld_valid)
+	snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
+	snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
+	if (!eld->eld_valid)
 		return;
 	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
 	snd_iprintf(buffer, "connection_type\t\t%s\n",
 				eld_connection_type_names[e->conn_type]);
 	snd_iprintf(buffer, "eld_version\t\t[0x%x] %s\n", e->eld_ver,
-					eld_versoin_names[e->eld_ver]);
+					eld_version_names[e->eld_ver]);
 	snd_iprintf(buffer, "edid_version\t\t[0x%x] %s\n", e->cea_edid_ver,
 				cea_edid_version_names[e->cea_edid_ver]);
 	snd_iprintf(buffer, "manufacture_id\t\t0x%x\n", e->manufacture_id);
@@ -535,7 +530,8 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 static void hdmi_write_eld_info(struct snd_info_entry *entry,
 				struct snd_info_buffer *buffer)
 {
-	struct hdmi_eld *e = entry->private_data;
+	struct hdmi_eld *eld = entry->private_data;
+	struct parsed_hdmi_eld *e = &eld->info;
 	char line[64];
 	char name[64];
 	char *sname;
@@ -551,9 +547,9 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
 		 * 	eld_version edid_version
 		 */
 		if (!strcmp(name, "monitor_present"))
-			e->monitor_present = val;
+			eld->monitor_present = val;
 		else if (!strcmp(name, "eld_valid"))
-			e->eld_valid = val;
+			eld->eld_valid = val;
 		else if (!strcmp(name, "connection_type"))
 			e->conn_type = val;
 		else if (!strcmp(name, "port_id"))
@@ -627,7 +623,7 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld)
 #endif /* CONFIG_PROC_FS */
 
 /* update PCM info based on ELD */
-void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld,
+void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e,
 			      struct hda_pcm_stream *hinfo)
 {
 	u32 rates;
@@ -644,8 +640,8 @@ void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld,
 	formats = SNDRV_PCM_FMTBIT_S16_LE;
 	maxbps = 16;
 	channels_max = 2;
-	for (i = 0; i < eld->sad_count; i++) {
-		struct cea_sad *a = &eld->sad[i];
+	for (i = 0; i < e->sad_count; i++) {
+		struct cea_sad *a = &e->sad[i];
 		rates |= a->rates;
 		if (a->channels > channels_max)
 			channels_max = a->channels;
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 05f1d59..363cd48 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -713,10 +713,10 @@ struct cea_sad {
 /*
  * ELD: EDID Like Data
  */
-struct hdmi_eld {
-	bool	monitor_present;
-	bool	eld_valid;
-	int	eld_size;
+struct parsed_hdmi_eld {
+	/*
+	 * all fields will be cleared before updating ELD
+	 */
 	int	baseline_len;
 	int	eld_ver;
 	int	cea_edid_ver;
@@ -731,19 +731,26 @@ struct hdmi_eld {
 	int	spk_alloc;
 	int	sad_count;
 	struct cea_sad sad[ELD_MAX_SAD];
-	/*
-	 * all fields above eld_buffer will be cleared before updating ELD
-	 */
+};
+
+struct hdmi_eld {
+	bool	monitor_present;
+	bool	eld_valid;
+	int	eld_size;
 	char    eld_buffer[ELD_MAX_SIZE];
+	struct parsed_hdmi_eld info;
 #ifdef CONFIG_PROC_FS
 	struct snd_info_entry *proc_entry;
 #endif
 };
 
 int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid);
-int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t);
-void snd_hdmi_show_eld(struct hdmi_eld *eld);
-void snd_hdmi_eld_update_pcm_info(struct hdmi_eld *eld,
+int snd_hdmi_get_eld(struct hda_codec *codec, hda_nid_t nid,
+		     unsigned char *buf, int *eld_size);
+int snd_hdmi_parse_eld(struct parsed_hdmi_eld *e,
+		       const unsigned char *buf, int size);
+void snd_hdmi_show_eld(struct parsed_hdmi_eld *e);
+void snd_hdmi_eld_update_pcm_info(struct parsed_hdmi_eld *e,
 			      struct hda_pcm_stream *hinfo);
 
 #ifdef CONFIG_PROC_FS
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 7a73dfb..d157528 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -529,7 +529,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels)
 	 * expand ELD's notions to match the ones used by Audio InfoFrame.
 	 */
 	for (i = 0; i < ARRAY_SIZE(eld_speaker_allocation_bits); i++) {
-		if (eld->spk_alloc & (1 << i))
+		if (eld->info.spk_alloc & (1 << i))
 			spk_mask |= eld_speaker_allocation_bits[i];
 	}
 
@@ -543,7 +543,7 @@ static int hdmi_channel_allocation(struct hdmi_eld *eld, int channels)
 		}
 	}
 
-	snd_print_channel_allocation(eld->spk_alloc, buf, sizeof(buf));
+	snd_print_channel_allocation(eld->info.spk_alloc, buf, sizeof(buf));
 	snd_printdd("HDMI: select CA 0x%x for %d-channel allocation: %s\n",
 		    ca, channels, buf);
 
@@ -884,7 +884,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
 		ca = 0;
 
 	memset(&ai, 0, sizeof(ai));
-	if (eld->conn_type == 0) { /* HDMI */
+	if (eld->info.conn_type == 0) { /* HDMI */
 		struct hdmi_audio_infoframe *hdmi_ai = &ai.hdmi;
 
 		hdmi_ai->type		= 0x84;
@@ -893,7 +893,7 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
 		hdmi_ai->CC02_CT47	= channels - 1;
 		hdmi_ai->CA		= ca;
 		hdmi_checksum_audio_infoframe(hdmi_ai);
-	} else if (eld->conn_type == 1) { /* DisplayPort */
+	} else if (eld->info.conn_type == 1) { /* DisplayPort */
 		struct dp_audio_infoframe *dp_ai = &ai.dp;
 
 		dp_ai->type		= 0x84;
@@ -1114,7 +1114,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 
 	/* Restrict capabilities by ELD if this isn't disabled */
 	if (!static_hdmi_pcm && eld->eld_valid) {
-		snd_hdmi_eld_update_pcm_info(eld, hinfo);
+		snd_hdmi_eld_update_pcm_info(&eld->info, hinfo);
 		if (hinfo->channels_min > hinfo->channels_max ||
 		    !hinfo->rates || !hinfo->formats) {
 			per_cvt->assigned = 0;
@@ -1175,8 +1175,6 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	int present = snd_hda_pin_sense(codec, pin_nid);
 	bool eld_valid = false;
 
-	memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
-
 	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
 	if (eld->monitor_present)
 		eld_valid	= !!(present & AC_PINSENSE_ELDV);
@@ -1187,8 +1185,20 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 
 	eld->eld_valid = false;
 	if (eld_valid) {
-		if (!snd_hdmi_get_eld(eld, codec, pin_nid))
-			snd_hdmi_show_eld(eld);
+		if (snd_hdmi_get_eld(codec, pin_nid, eld->eld_buffer,
+						     &eld->eld_size) < 0)
+			eld_valid = false;
+		else {
+			memset(&eld->info, 0, sizeof(struct parsed_hdmi_eld));
+			if (snd_hdmi_parse_eld(&eld->info, eld->eld_buffer,
+						    eld->eld_size) < 0)
+				eld_valid = false;
+		}
+
+		if (eld_valid) {
+			snd_hdmi_show_eld(&eld->info);
+			eld->eld_valid = true;
+		}
 		else if (repoll) {
 			queue_delayed_work(codec->bus->workq,
 					   &per_pin->work,
-- 
1.7.9.5

[PATCH v4 4/5] ALSA: hda - hdmi: Protect ELD buffer

[David Henningsson]

Because the eld buffer can be simultaneously accessed from both
workqueue context (updating) and process context (kcontrol read),
we need to protect it with a mutex to guarantee consistency.

To avoid holding the mutex while reading the ELD info from the
codec, we introduce a temporary eld buffer.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_eld.c    |    8 ++++++-
 sound/pci/hda/hda_local.h  |    1 +
 sound/pci/hda/patch_hdmi.c |   50 +++++++++++++++++++++++++++++++++-----------
 3 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
index 16066d7..7dd8463 100644
--- a/sound/pci/hda/hda_eld.c
+++ b/sound/pci/hda/hda_eld.c
@@ -500,10 +500,13 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 		[4 ... 7] = "reserved"
 	};
 
+	mutex_lock(&eld->lock);
 	snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
 	snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
-	if (!eld->eld_valid)
+	if (!eld->eld_valid) {
+		mutex_unlock(&eld->lock);
 		return;
+	}
 	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
 	snd_iprintf(buffer, "connection_type\t\t%s\n",
 				eld_connection_type_names[e->conn_type]);
@@ -525,6 +528,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 
 	for (i = 0; i < e->sad_count; i++)
 		hdmi_print_sad_info(i, e->sad + i, buffer);
+	mutex_unlock(&eld->lock);
 }
 
 static void hdmi_write_eld_info(struct snd_info_entry *entry,
@@ -538,6 +542,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
 	long long val;
 	unsigned int n;
 
+	mutex_lock(&eld->lock);
 	while (!snd_info_get_line(buffer, line, sizeof(line))) {
 		if (sscanf(line, "%s %llx", name, &val) != 2)
 			continue;
@@ -589,6 +594,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
 				e->sad_count = n + 1;
 		}
 	}
+	mutex_unlock(&eld->lock);
 }
 
 
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 363cd48..83b7486 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -739,6 +739,7 @@ struct hdmi_eld {
 	int	eld_size;
 	char    eld_buffer[ELD_MAX_SIZE];
 	struct parsed_hdmi_eld info;
+	struct mutex lock;
 #ifdef CONFIG_PROC_FS
 	struct snd_info_entry *proc_entry;
 #endif
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index d157528..ec3ff3f 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -91,6 +91,7 @@ struct hdmi_spec {
 	struct hda_pcm pcm_rec[MAX_HDMI_PINS];
 	unsigned int channels_max; /* max over all cvts */
 
+	struct hdmi_eld temp_eld;
 	/*
 	 * Non-generic ATI/NVIDIA specific
 	 */
@@ -352,7 +353,9 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol,
 	pin_idx = kcontrol->private_value;
 	eld = &spec->pins[pin_idx].sink_eld;
 
+	mutex_lock(&eld->lock);
 	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
+	mutex_unlock(&eld->lock);
 
 	return 0;
 }
@@ -368,7 +371,9 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
 	pin_idx = kcontrol->private_value;
 	eld = &spec->pins[pin_idx].sink_eld;
 
+	mutex_lock(&eld->lock);
 	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
+		mutex_unlock(&eld->lock);
 		snd_BUG();
 		return -EINVAL;
 	}
@@ -376,6 +381,7 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
 	if (eld->eld_valid)
 		memcpy(ucontrol->value.bytes.data, eld->eld_buffer,
 		       eld->eld_size);
+	mutex_unlock(&eld->lock);
 
 	return 0;
 }
@@ -1162,7 +1168,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 {
 	struct hda_codec *codec = per_pin->codec;
-	struct hdmi_eld *eld = &per_pin->sink_eld;
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_eld *eld = &spec->temp_eld;
+	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
 	hda_nid_t pin_nid = per_pin->pin_nid;
 	/*
 	 * Always execute a GetPinSense verb here, even when called from
@@ -1173,31 +1181,33 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present = snd_hda_pin_sense(codec, pin_nid);
-	bool eld_valid = false;
+	bool update_eld = false;
+	bool eld_changed = false;
 
-	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
-	if (eld->monitor_present)
-		eld_valid	= !!(present & AC_PINSENSE_ELDV);
+	pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
+	if (pin_eld->monitor_present)
+		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
+	else
+		eld->eld_valid = false;
 
 	_snd_printd(SND_PR_VERBOSE,
 		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
-		codec->addr, pin_nid, eld->monitor_present, eld_valid);
+		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
 
-	eld->eld_valid = false;
-	if (eld_valid) {
+	if (eld->eld_valid) {
 		if (snd_hdmi_get_eld(codec, pin_nid, eld->eld_buffer,
 						     &eld->eld_size) < 0)
-			eld_valid = false;
+			eld->eld_valid = false;
 		else {
 			memset(&eld->info, 0, sizeof(struct parsed_hdmi_eld));
 			if (snd_hdmi_parse_eld(&eld->info, eld->eld_buffer,
 						    eld->eld_size) < 0)
-				eld_valid = false;
+				eld->eld_valid = false;
 		}
 
-		if (eld_valid) {
+		if (eld->eld_valid) {
 			snd_hdmi_show_eld(&eld->info);
-			eld->eld_valid = true;
+			update_eld = true;
 		}
 		else if (repoll) {
 			queue_delayed_work(codec->bus->workq,
@@ -1205,6 +1215,21 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 					   msecs_to_jiffies(300));
 		}
 	}
+
+	mutex_lock(&pin_eld->lock);
+	if (pin_eld->eld_valid && !eld->eld_valid)
+		update_eld = true;
+	if (update_eld) {
+		pin_eld->eld_valid = eld->eld_valid;
+		eld_changed = memcmp(pin_eld->eld_buffer, eld->eld_buffer,
+				     eld->eld_size) != 0;
+		if (eld_changed)
+			memcpy(pin_eld->eld_buffer, eld->eld_buffer,
+			       eld->eld_size);
+		pin_eld->eld_size = eld->eld_size;
+		pin_eld->info = eld->info;
+	}
+	mutex_unlock(&pin_eld->lock);
 }
 
 static void hdmi_repoll_eld(struct work_struct *work)
@@ -1672,6 +1697,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec)
 		struct hdmi_eld *eld = &per_pin->sink_eld;
 
 		per_pin->codec = codec;
+		mutex_init(&eld->lock);
 		INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld);
 		snd_hda_eld_proc_new(codec, eld, pin_idx);
 	}
-- 
1.7.9.5

[PATCH v4 5/5] ALSA: hda - hdmi: Notify userspace when ELD control changes

[David Henningsson]

ELD validity can change during the lifetime of a presence detect,
so we need to be able to listen for changes on the ELD control.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index ec3ff3f..a6b5ad0 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -75,6 +75,7 @@ struct hdmi_spec_per_pin {
 	struct hda_codec *codec;
 	struct hdmi_eld sink_eld;
 	struct delayed_work work;
+	struct snd_kcontrol *eld_ctl;
 	int repoll_count;
 	bool non_pcm;
 	bool chmap_set;		/* channel-map override by ALSA API? */
@@ -411,6 +412,7 @@ static int hdmi_create_eld_ctl(struct hda_codec *codec, int pin_idx,
 	if (err < 0)
 		return err;
 
+	spec->pins[pin_idx].eld_ctl = kctl;
 	return 0;
 }
 
@@ -1217,19 +1219,27 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	}
 
 	mutex_lock(&pin_eld->lock);
-	if (pin_eld->eld_valid && !eld->eld_valid)
+	if (pin_eld->eld_valid && !eld->eld_valid) {
 		update_eld = true;
+		eld_changed = true;
+	}
 	if (update_eld) {
 		pin_eld->eld_valid = eld->eld_valid;
-		eld_changed = memcmp(pin_eld->eld_buffer, eld->eld_buffer,
+		eld_changed |= memcmp(pin_eld->eld_buffer, eld->eld_buffer,
 				     eld->eld_size) != 0;
 		if (eld_changed)
 			memcpy(pin_eld->eld_buffer, eld->eld_buffer,
 			       eld->eld_size);
+		eld_changed |= pin_eld->eld_size != eld->eld_size;
 		pin_eld->eld_size = eld->eld_size;
 		pin_eld->info = eld->info;
 	}
 	mutex_unlock(&pin_eld->lock);
+
+	if (eld_changed)
+		snd_ctl_notify(codec->bus->card,
+			       SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO,
+			       &per_pin->eld_ctl->id);
 }
 
 static void hdmi_repoll_eld(struct work_struct *work)
-- 
1.7.9.5

Re: [PATCH v4 2/5] ALSA: hda - hdmi: Do not expose eld data when eld is invalid

[Takashi Iwai]

At Tue, 19 Feb 2013 13:23:02 +0100,
David Henningsson wrote:
> 
> Previously, it was possible to read the eld data of the previous
> monitor connected. This should not be allowed.
> 
> Also refactor the function slightly.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/patch_hdmi.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 32adaa6..7a73dfb 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -343,14 +343,16 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol,
>  			struct snd_ctl_elem_info *uinfo)
>  {
>  	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> -	struct hdmi_spec *spec;
> +	struct hdmi_spec *spec = codec->spec;
> +	struct hdmi_eld *eld;
>  	int pin_idx;
>  
> -	spec = codec->spec;
>  	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
>  
>  	pin_idx = kcontrol->private_value;
> -	uinfo->count = spec->pins[pin_idx].sink_eld.eld_size;
> +	eld = &spec->pins[pin_idx].sink_eld;
> +
> +	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
>  
>  	return 0;
>  }
> @@ -359,14 +361,21 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
>  			struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> -	struct hdmi_spec *spec;
> +	struct hdmi_spec *spec = codec->spec;
> +	struct hdmi_eld *eld;
>  	int pin_idx;
>  
> -	spec = codec->spec;
>  	pin_idx = kcontrol->private_value;
> +	eld = &spec->pins[pin_idx].sink_eld;
> +
> +	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
> +		snd_BUG();
> +		return -EINVAL;
> +	}
>  
> -	memcpy(ucontrol->value.bytes.data,
> -		spec->pins[pin_idx].sink_eld.eld_buffer, ELD_MAX_SIZE);
> +	if (eld->eld_valid)
> +		memcpy(ucontrol->value.bytes.data, eld->eld_buffer,
> +		       eld->eld_size);

Looking at the code again, I think it's safer to clear the array at
first.  In theory, it's possible that the ELD changes between info()
and get().  For example, when it's unplugged between info() and get()
calls, info() returns certain ELD bytes while get() leaves just
uninitialized.  If it's all zero, at least, it's easier to see that
something is wrong.


thanks,

Takashi

Re: [PATCH v4 4/5] ALSA: hda - hdmi: Protect ELD buffer

[Takashi Iwai]

At Tue, 19 Feb 2013 13:23:04 +0100,
David Henningsson wrote:
> 
> Because the eld buffer can be simultaneously accessed from both
> workqueue context (updating) and process context (kcontrol read),
> we need to protect it with a mutex to guarantee consistency.
> 
> To avoid holding the mutex while reading the ELD info from the
> codec, we introduce a temporary eld buffer.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>

Looks almost good to me.

One remaining question is whether we should copy the data when the
probe is being repolled.  During repolling, it's essentially an
"unknown" state: the display is detected, but ELD isn't.  Since you'll
very get a valid ELD soon later, wouldn't it be better to just
postpone the update / copy of eld to pin_eld until the successful
repoll?  i.e. simply add "return" after queue_delayed_work().


thanks,

Takashi

> ---
>  sound/pci/hda/hda_eld.c    |    8 ++++++-
>  sound/pci/hda/hda_local.h  |    1 +
>  sound/pci/hda/patch_hdmi.c |   50 +++++++++++++++++++++++++++++++++-----------
>  3 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> index 16066d7..7dd8463 100644
> --- a/sound/pci/hda/hda_eld.c
> +++ b/sound/pci/hda/hda_eld.c
> @@ -500,10 +500,13 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
>  		[4 ... 7] = "reserved"
>  	};
>  
> +	mutex_lock(&eld->lock);
>  	snd_iprintf(buffer, "monitor_present\t\t%d\n", eld->monitor_present);
>  	snd_iprintf(buffer, "eld_valid\t\t%d\n", eld->eld_valid);
> -	if (!eld->eld_valid)
> +	if (!eld->eld_valid) {
> +		mutex_unlock(&eld->lock);
>  		return;
> +	}
>  	snd_iprintf(buffer, "monitor_name\t\t%s\n", e->monitor_name);
>  	snd_iprintf(buffer, "connection_type\t\t%s\n",
>  				eld_connection_type_names[e->conn_type]);
> @@ -525,6 +528,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
>  
>  	for (i = 0; i < e->sad_count; i++)
>  		hdmi_print_sad_info(i, e->sad + i, buffer);
> +	mutex_unlock(&eld->lock);
>  }
>  
>  static void hdmi_write_eld_info(struct snd_info_entry *entry,
> @@ -538,6 +542,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
>  	long long val;
>  	unsigned int n;
>  
> +	mutex_lock(&eld->lock);
>  	while (!snd_info_get_line(buffer, line, sizeof(line))) {
>  		if (sscanf(line, "%s %llx", name, &val) != 2)
>  			continue;
> @@ -589,6 +594,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
>  				e->sad_count = n + 1;
>  		}
>  	}
> +	mutex_unlock(&eld->lock);
>  }
>  
>  
> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
> index 363cd48..83b7486 100644
> --- a/sound/pci/hda/hda_local.h
> +++ b/sound/pci/hda/hda_local.h
> @@ -739,6 +739,7 @@ struct hdmi_eld {
>  	int	eld_size;
>  	char    eld_buffer[ELD_MAX_SIZE];
>  	struct parsed_hdmi_eld info;
> +	struct mutex lock;
>  #ifdef CONFIG_PROC_FS
>  	struct snd_info_entry *proc_entry;
>  #endif
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index d157528..ec3ff3f 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -91,6 +91,7 @@ struct hdmi_spec {
>  	struct hda_pcm pcm_rec[MAX_HDMI_PINS];
>  	unsigned int channels_max; /* max over all cvts */
>  
> +	struct hdmi_eld temp_eld;
>  	/*
>  	 * Non-generic ATI/NVIDIA specific
>  	 */
> @@ -352,7 +353,9 @@ static int hdmi_eld_ctl_info(struct snd_kcontrol *kcontrol,
>  	pin_idx = kcontrol->private_value;
>  	eld = &spec->pins[pin_idx].sink_eld;
>  
> +	mutex_lock(&eld->lock);
>  	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
> +	mutex_unlock(&eld->lock);
>  
>  	return 0;
>  }
> @@ -368,7 +371,9 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
>  	pin_idx = kcontrol->private_value;
>  	eld = &spec->pins[pin_idx].sink_eld;
>  
> +	mutex_lock(&eld->lock);
>  	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
> +		mutex_unlock(&eld->lock);
>  		snd_BUG();
>  		return -EINVAL;
>  	}
> @@ -376,6 +381,7 @@ static int hdmi_eld_ctl_get(struct snd_kcontrol *kcontrol,
>  	if (eld->eld_valid)
>  		memcpy(ucontrol->value.bytes.data, eld->eld_buffer,
>  		       eld->eld_size);
> +	mutex_unlock(&eld->lock);
>  
>  	return 0;
>  }
> @@ -1162,7 +1168,9 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
>  static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  {
>  	struct hda_codec *codec = per_pin->codec;
> -	struct hdmi_eld *eld = &per_pin->sink_eld;
> +	struct hdmi_spec *spec = codec->spec;
> +	struct hdmi_eld *eld = &spec->temp_eld;
> +	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
>  	hda_nid_t pin_nid = per_pin->pin_nid;
>  	/*
>  	 * Always execute a GetPinSense verb here, even when called from
> @@ -1173,31 +1181,33 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  	 * the unsolicited response to avoid custom WARs.
>  	 */
>  	int present = snd_hda_pin_sense(codec, pin_nid);
> -	bool eld_valid = false;
> +	bool update_eld = false;
> +	bool eld_changed = false;
>  
> -	eld->monitor_present	= !!(present & AC_PINSENSE_PRESENCE);
> -	if (eld->monitor_present)
> -		eld_valid	= !!(present & AC_PINSENSE_ELDV);
> +	pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE);
> +	if (pin_eld->monitor_present)
> +		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
> +	else
> +		eld->eld_valid = false;
>  
>  	_snd_printd(SND_PR_VERBOSE,
>  		"HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n",
> -		codec->addr, pin_nid, eld->monitor_present, eld_valid);
> +		codec->addr, pin_nid, eld->monitor_present, eld->eld_valid);
>  
> -	eld->eld_valid = false;
> -	if (eld_valid) {
> +	if (eld->eld_valid) {
>  		if (snd_hdmi_get_eld(codec, pin_nid, eld->eld_buffer,
>  						     &eld->eld_size) < 0)
> -			eld_valid = false;
> +			eld->eld_valid = false;
>  		else {
>  			memset(&eld->info, 0, sizeof(struct parsed_hdmi_eld));
>  			if (snd_hdmi_parse_eld(&eld->info, eld->eld_buffer,
>  						    eld->eld_size) < 0)
> -				eld_valid = false;
> +				eld->eld_valid = false;
>  		}
>  
> -		if (eld_valid) {
> +		if (eld->eld_valid) {
>  			snd_hdmi_show_eld(&eld->info);
> -			eld->eld_valid = true;
> +			update_eld = true;
>  		}
>  		else if (repoll) {
>  			queue_delayed_work(codec->bus->workq,
> @@ -1205,6 +1215,21 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  					   msecs_to_jiffies(300));
>  		}
>  	}
> +
> +	mutex_lock(&pin_eld->lock);
> +	if (pin_eld->eld_valid && !eld->eld_valid)
> +		update_eld = true;
> +	if (update_eld) {
> +		pin_eld->eld_valid = eld->eld_valid;
> +		eld_changed = memcmp(pin_eld->eld_buffer, eld->eld_buffer,
> +				     eld->eld_size) != 0;
> +		if (eld_changed)
> +			memcpy(pin_eld->eld_buffer, eld->eld_buffer,
> +			       eld->eld_size);
> +		pin_eld->eld_size = eld->eld_size;
> +		pin_eld->info = eld->info;
> +	}
> +	mutex_unlock(&pin_eld->lock);
>  }
>  
>  static void hdmi_repoll_eld(struct work_struct *work)
> @@ -1672,6 +1697,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec)
>  		struct hdmi_eld *eld = &per_pin->sink_eld;
>  
>  		per_pin->codec = codec;
> +		mutex_init(&eld->lock);
>  		INIT_DELAYED_WORK(&per_pin->work, hdmi_repoll_eld);
>  		snd_hda_eld_proc_new(codec, eld, pin_idx);
>  	}
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH v4 5/5] ALSA: hda - hdmi: Notify userspace when ELD control changes

[Takashi Iwai]

At Tue, 19 Feb 2013 13:23:05 +0100,
David Henningsson wrote:
> 
> ELD validity can change during the lifetime of a presence detect,
> so we need to be able to listen for changes on the ELD control.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/patch_hdmi.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index ec3ff3f..a6b5ad0 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -75,6 +75,7 @@ struct hdmi_spec_per_pin {
>  	struct hda_codec *codec;
>  	struct hdmi_eld sink_eld;
>  	struct delayed_work work;
> +	struct snd_kcontrol *eld_ctl;
>  	int repoll_count;
>  	bool non_pcm;
>  	bool chmap_set;		/* channel-map override by ALSA API? */
> @@ -411,6 +412,7 @@ static int hdmi_create_eld_ctl(struct hda_codec *codec, int pin_idx,
>  	if (err < 0)
>  		return err;
>  
> +	spec->pins[pin_idx].eld_ctl = kctl;
>  	return 0;
>  }
>  
> @@ -1217,19 +1219,27 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  	}
>  
>  	mutex_lock(&pin_eld->lock);
> -	if (pin_eld->eld_valid && !eld->eld_valid)
> +	if (pin_eld->eld_valid && !eld->eld_valid) {
>  		update_eld = true;
> +		eld_changed = true;
> +	}
>  	if (update_eld) {
>  		pin_eld->eld_valid = eld->eld_valid;
> -		eld_changed = memcmp(pin_eld->eld_buffer, eld->eld_buffer,
> +		eld_changed |= memcmp(pin_eld->eld_buffer, eld->eld_buffer,
>  				     eld->eld_size) != 0;
>  		if (eld_changed)
>  			memcpy(pin_eld->eld_buffer, eld->eld_buffer,
>  			       eld->eld_size);
> +		eld_changed |= pin_eld->eld_size != eld->eld_size;

The size check should be done before memcmp().  memcmp() can be
skipped if it's already set true.

 	if (update_eld) {
 		pin_eld->eld_valid = eld->eld_valid;
		eld_changed |= pin_eld->eld_size != eld->eld_size;
		if (!eld_changed)
			eld_changed = memcmp(pin_eld->eld_buffer, eld->eld_buffer,
 				     eld->eld_size) != 0;
		if (eld_changed)
			memcpy(pin_eld->eld_buffer, eld->eld_buffer,
			       eld->eld_size);

 		pin_eld->eld_size = eld->eld_size;
 		pin_eld->info = eld->info;
 	}


Takashi