[PATCH v2 0/4] Make HDMI ELD usable for hotplug

[PATCH v2 0/4] Make HDMI ELD usable for hotplug

[David Henningsson]

New day, new patches. This time there is a proper lock around
access to the ELD buffer. I hope this satisfies the previous
comments.

I've also tested the patches together with some PulseAudio patches
I'm writing and the notification works just fine. The ELD data
shows up 300 ms after hotplug, so it's likely the first repoll
giving me new ELD data).

David Henningsson (4):
  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: protect access to ELD buffer
  ALSA: hda - hdmi: Notify userspace when ELD control changes

 sound/pci/hda/hda_eld.c    |   12 ++++++--
 sound/pci/hda/hda_local.h  |    3 ++
 sound/pci/hda/patch_hdmi.c |   67 +++++++++++++++++++++++++++++++++++---------
 3 files changed, 66 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] 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 v2 2/4] 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 v2 3/4] ALSA: hda - hdmi: protect access to 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 spinlock to guarantee consistency.

To avoid holding the spinlock 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    |   12 +++++++++---
 sound/pci/hda/hda_local.h  |    3 +++
 sound/pci/hda/patch_hdmi.c |   32 +++++++++++++++++++++++++-------
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
index 4c054f4..766f011 100644
--- a/sound/pci/hda/hda_eld.c
+++ b/sound/pci/hda/hda_eld.c
@@ -490,7 +490,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 	struct hdmi_eld *e = entry->private_data;
 	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 +505,18 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
 		[4 ... 7] = "reserved"
 	};
 
+	spin_lock(&e->lock);
 	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)
+	if (!e->eld_valid) {
+		spin_unlock(&e->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]);
 	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);
@@ -530,6 +533,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);
+	spin_unlock(&e->lock);
 }
 
 static void hdmi_write_eld_info(struct snd_info_entry *entry,
@@ -542,6 +546,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
 	long long val;
 	unsigned int n;
 
+	spin_lock(&e->lock);
 	while (!snd_info_get_line(buffer, line, sizeof(line))) {
 		if (sscanf(line, "%s %llx", name, &val) != 2)
 			continue;
@@ -593,6 +598,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
 				e->sad_count = n + 1;
 		}
 	}
+	spin_unlock(&e->lock);
 }
 
 
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 05f1d59..b2d6a9e 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -712,8 +712,11 @@ struct cea_sad {
 
 /*
  * ELD: EDID Like Data
+ *
+ * Note: The order of these fields is used in hdmi_present_sense
  */
 struct hdmi_eld {
+	spinlock_t lock;
 	bool	monitor_present;
 	bool	eld_valid;
 	int	eld_size;
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 7a73dfb..52b1afc 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;
 
+	spin_lock(&eld->lock);
 	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
+	spin_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;
 
+	spin_lock(&eld->lock);
 	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
+		spin_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);
+	spin_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,28 +1181,37 @@ 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;
 
 	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);
+		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
 
 	_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 (!snd_hdmi_get_eld(eld, codec, pin_nid))
+	if (eld->eld_valid) {
+		if (!snd_hdmi_get_eld(eld, codec, pin_nid)) {
 			snd_hdmi_show_eld(eld);
+			update_eld = true;
+		}
 		else if (repoll) {
 			queue_delayed_work(codec->bus->workq,
 					   &per_pin->work,
 					   msecs_to_jiffies(300));
 		}
 	}
+
+	spin_lock(&pin_eld->lock);
+	if (pin_eld->eld_valid && !eld->eld_valid)
+		update_eld = true;
+	if (update_eld)
+		memcpy(&pin_eld->monitor_present, &eld->monitor_present,
+		       sizeof(struct hdmi_eld) - sizeof(spinlock_t));
+	spin_unlock(&pin_eld->lock);
 }
 
 static void hdmi_repoll_eld(struct work_struct *work)
@@ -1662,6 +1679,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec)
 		struct hdmi_eld *eld = &per_pin->sink_eld;
 
 		per_pin->codec = codec;
+		spin_lock_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 v2 4/4] 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 |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 52b1afc..bfaa0ee 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;
 }
 
@@ -1182,6 +1184,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	 */
 	int present = snd_hda_pin_sense(codec, pin_nid);
 	bool update_eld = false;
+	bool eld_changed = false;
 
 	memset(eld, 0, offsetof(struct hdmi_eld, eld_buffer));
 
@@ -1208,10 +1211,20 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 	spin_lock(&pin_eld->lock);
 	if (pin_eld->eld_valid && !eld->eld_valid)
 		update_eld = true;
-	if (update_eld)
-		memcpy(&pin_eld->monitor_present, &eld->monitor_present,
-		       sizeof(struct hdmi_eld) - sizeof(spinlock_t));
+	if (update_eld) {
+		void *dest = &pin_eld->monitor_present;
+		void *src = &eld->monitor_present;
+		size_t size = sizeof(struct hdmi_eld) - sizeof(spinlock_t);
+		eld_changed = memcmp(dest, src, size) != 0;
+		if (eld_changed)
+			memcpy(dest, src, size);
+	}
 	spin_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 v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer

[Takashi Iwai]

At Tue, 19 Feb 2013 09:47:36 +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 spinlock to guarantee consistency.

We can't use a spinlock for snd_iprintf(), as this involves with
vmalloc().  Use a mutex intead.


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

Looking through the patches, it's better to split to a few parts now:

struct parsed_hdmi_eld {
	bool monitor_present;
	....
};

struct hdmi_eld {
	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
};

In that way, you can avoid the hack like offset() calculation in
patch_hdmi.c.  The comparison can be done only for eld_buffer[], too.


thanks,

Takashi

> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/hda_eld.c    |   12 +++++++++---
>  sound/pci/hda/hda_local.h  |    3 +++
>  sound/pci/hda/patch_hdmi.c |   32 +++++++++++++++++++++++++-------
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> index 4c054f4..766f011 100644
> --- a/sound/pci/hda/hda_eld.c
> +++ b/sound/pci/hda/hda_eld.c
> @@ -490,7 +490,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
>  	struct hdmi_eld *e = entry->private_data;
>  	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 +505,18 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
>  		[4 ... 7] = "reserved"
>  	};
>  
> +	spin_lock(&e->lock);
>  	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)
> +	if (!e->eld_valid) {
> +		spin_unlock(&e->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]);
>  	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);
> @@ -530,6 +533,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);
> +	spin_unlock(&e->lock);
>  }
>  
>  static void hdmi_write_eld_info(struct snd_info_entry *entry,
> @@ -542,6 +546,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
>  	long long val;
>  	unsigned int n;
>  
> +	spin_lock(&e->lock);
>  	while (!snd_info_get_line(buffer, line, sizeof(line))) {
>  		if (sscanf(line, "%s %llx", name, &val) != 2)
>  			continue;
> @@ -593,6 +598,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
>  				e->sad_count = n + 1;
>  		}
>  	}
> +	spin_unlock(&e->lock);
>  }
>  
>  
> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
> index 05f1d59..b2d6a9e 100644
> --- a/sound/pci/hda/hda_local.h
> +++ b/sound/pci/hda/hda_local.h
> @@ -712,8 +712,11 @@ struct cea_sad {
>  
>  /*
>   * ELD: EDID Like Data
> + *
> + * Note: The order of these fields is used in hdmi_present_sense
>   */
>  struct hdmi_eld {
> +	spinlock_t lock;
>  	bool	monitor_present;
>  	bool	eld_valid;
>  	int	eld_size;
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 7a73dfb..52b1afc 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;
>  
> +	spin_lock(&eld->lock);
>  	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
> +	spin_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;
>  
> +	spin_lock(&eld->lock);
>  	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
> +		spin_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);
> +	spin_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,28 +1181,37 @@ 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;
>  
>  	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);
> +		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
>  
>  	_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 (!snd_hdmi_get_eld(eld, codec, pin_nid))
> +	if (eld->eld_valid) {
> +		if (!snd_hdmi_get_eld(eld, codec, pin_nid)) {
>  			snd_hdmi_show_eld(eld);
> +			update_eld = true;
> +		}
>  		else if (repoll) {
>  			queue_delayed_work(codec->bus->workq,
>  					   &per_pin->work,
>  					   msecs_to_jiffies(300));
>  		}
>  	}
> +
> +	spin_lock(&pin_eld->lock);
> +	if (pin_eld->eld_valid && !eld->eld_valid)
> +		update_eld = true;
> +	if (update_eld)
> +		memcpy(&pin_eld->monitor_present, &eld->monitor_present,
> +		       sizeof(struct hdmi_eld) - sizeof(spinlock_t));
> +	spin_unlock(&pin_eld->lock);
>  }
>  
>  static void hdmi_repoll_eld(struct work_struct *work)
> @@ -1662,6 +1679,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec)
>  		struct hdmi_eld *eld = &per_pin->sink_eld;
>  
>  		per_pin->codec = codec;
> +		spin_lock_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
> 

Re: [PATCH v2 3/4] ALSA: hda - hdmi: protect access to ELD buffer

[David Henningsson]

On 02/19/2013 10:15 AM, Takashi Iwai wrote:
> At Tue, 19 Feb 2013 09:47:36 +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 spinlock to guarantee consistency.
>
> We can't use a spinlock for snd_iprintf(), as this involves with
> vmalloc().  Use a mutex intead.

Ok, thanks for spotting.

>> To avoid holding the spinlock while reading the ELD info from the
>> codec, we introduce a temporary eld buffer.
>
> Looking through the patches, it's better to split to a few parts now:
>
> struct parsed_hdmi_eld {
> 	bool monitor_present;
> 	....
> };
>
> struct hdmi_eld {
> 	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
> };
>
> In that way, you can avoid the hack like offset() calculation in
> patch_hdmi.c.  The comparison can be done only for eld_buffer[], too.

This sounds like a good idea. I will do it. I think eld_valid and 
monitor_present should be in hdmi_eld and not parsed_hdmi_eld, because 
those fields do not come from the raw ELD info.



>
>
> thanks,
>
> Takashi
>
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>> ---
>>   sound/pci/hda/hda_eld.c    |   12 +++++++++---
>>   sound/pci/hda/hda_local.h  |    3 +++
>>   sound/pci/hda/patch_hdmi.c |   32 +++++++++++++++++++++++++-------
>>   3 files changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
>> index 4c054f4..766f011 100644
>> --- a/sound/pci/hda/hda_eld.c
>> +++ b/sound/pci/hda/hda_eld.c
>> @@ -490,7 +490,7 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
>>   	struct hdmi_eld *e = entry->private_data;
>>   	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 +505,18 @@ static void hdmi_print_eld_info(struct snd_info_entry *entry,
>>   		[4 ... 7] = "reserved"
>>   	};
>>
>> +	spin_lock(&e->lock);
>>   	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)
>> +	if (!e->eld_valid) {
>> +		spin_unlock(&e->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]);
>>   	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);
>> @@ -530,6 +533,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);
>> +	spin_unlock(&e->lock);
>>   }
>>
>>   static void hdmi_write_eld_info(struct snd_info_entry *entry,
>> @@ -542,6 +546,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
>>   	long long val;
>>   	unsigned int n;
>>
>> +	spin_lock(&e->lock);
>>   	while (!snd_info_get_line(buffer, line, sizeof(line))) {
>>   		if (sscanf(line, "%s %llx", name, &val) != 2)
>>   			continue;
>> @@ -593,6 +598,7 @@ static void hdmi_write_eld_info(struct snd_info_entry *entry,
>>   				e->sad_count = n + 1;
>>   		}
>>   	}
>> +	spin_unlock(&e->lock);
>>   }
>>
>>
>> diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
>> index 05f1d59..b2d6a9e 100644
>> --- a/sound/pci/hda/hda_local.h
>> +++ b/sound/pci/hda/hda_local.h
>> @@ -712,8 +712,11 @@ struct cea_sad {
>>
>>   /*
>>    * ELD: EDID Like Data
>> + *
>> + * Note: The order of these fields is used in hdmi_present_sense
>>    */
>>   struct hdmi_eld {
>> +	spinlock_t lock;
>>   	bool	monitor_present;
>>   	bool	eld_valid;
>>   	int	eld_size;
>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>> index 7a73dfb..52b1afc 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;
>>
>> +	spin_lock(&eld->lock);
>>   	uinfo->count = eld->eld_valid ? eld->eld_size : 0;
>> +	spin_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;
>>
>> +	spin_lock(&eld->lock);
>>   	if (eld->eld_size > ARRAY_SIZE(ucontrol->value.bytes.data)) {
>> +		spin_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);
>> +	spin_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,28 +1181,37 @@ 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;
>>
>>   	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);
>> +		eld->eld_valid  = !!(present & AC_PINSENSE_ELDV);
>>
>>   	_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 (!snd_hdmi_get_eld(eld, codec, pin_nid))
>> +	if (eld->eld_valid) {
>> +		if (!snd_hdmi_get_eld(eld, codec, pin_nid)) {
>>   			snd_hdmi_show_eld(eld);
>> +			update_eld = true;
>> +		}
>>   		else if (repoll) {
>>   			queue_delayed_work(codec->bus->workq,
>>   					   &per_pin->work,
>>   					   msecs_to_jiffies(300));
>>   		}
>>   	}
>> +
>> +	spin_lock(&pin_eld->lock);
>> +	if (pin_eld->eld_valid && !eld->eld_valid)
>> +		update_eld = true;
>> +	if (update_eld)
>> +		memcpy(&pin_eld->monitor_present, &eld->monitor_present,
>> +		       sizeof(struct hdmi_eld) - sizeof(spinlock_t));
>> +	spin_unlock(&pin_eld->lock);
>>   }
>>
>>   static void hdmi_repoll_eld(struct work_struct *work)
>> @@ -1662,6 +1679,7 @@ static int generic_hdmi_init_per_pins(struct hda_codec *codec)
>>   		struct hdmi_eld *eld = &per_pin->sink_eld;
>>
>>   		per_pin->codec = codec;
>> +		spin_lock_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
>>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic