[alsa-devel] [PATCH] ALSA: HDA: Add jack detection for HDMI

[Takashi Iwai]

At Tue, 24 May 2011 10:27:45 -0700,
Stephen Warren wrote:
> 
> Takashi Iwai wrote at Monday, May 23, 2011 11:40 PM:
> > At Mon, 23 May 2011 14:49:15 -0700, Stephen Warren wrote:
> > > ...
> > > I've been testing a preliminary fix for this internally. I found that
> > > the /proc ELD files correctly report monitor_present and eld_valid when
> > > we hot unplug a monitor, but the other fields in the ELD file are not
> > > cleared out to their default state. As best I can tell, this is an ALSA
> > > driver issue, since our display driver only fills in the other fields
> > > when ELDV==1.
> > 
> > Then it's just showing stale data.  The eld fields are updated only
> > when the data is valid.  The patch below should fix the confusion.
> 
> > ---
> > diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
> > index 74b0560..cd96b1d 100644
> > --- a/sound/pci/hda/hda_eld.c
> > +++ b/sound/pci/hda/hda_eld.c
> > @@ -477,6 +477,8 @@ static void hdmi_print_eld_info(struct snd_info_entry
> > *entry,
> > 
> >  	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)
> > +		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]);
> 
> Takashi,
> 
> Your patch looks fine as far as the reporting side goes.
> 
> Should we also modify hdmi_intrinsic_event() as follows:
> 
> 	if (pind && eldv) {
> 		hdmi_get_show_eld(codec, spec->pin[index],
> 				  &spec->sink_eld[index]);
> 		/* TODO: do real things about ELD */
> 	}
> +	else {
> +		memset(&spec->sink_eld[index], 0, sizeof(spec->sink_eld[index]));
> +	}
> 
> ... to make sure that if something accidentally uses the ELD data
> without checking eld_valid, it'll get obviously bad data instead of
> valid-looking-but-stale data? Right now, this isn't an issue, but if
> we start pushing the ELD into user-space through a binary control,
> it'd be nice if the ELD content were consistent in this way.

It sounds safer, but I'd call memset before setting monitor_present
and eld_valid as below.

> Hmm. Actually, hdmi_eld isn't storing the raw ELD bytes, but a parsed
> representation of them, so this may not be entirely relevant.

True.  Still clearing it explicitly would be better for avoiding
unintentional leaks in future.


thanks,

Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 3229018..4dbe8d6 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -707,6 +707,7 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 	if (index < 0)
 		return;
 
+	memset(&spec->sink_eld[index], 0, sizeof(spec->sink_eld[index]));
 	if (spec->old_pin_detect) {
 		if (pind)
 			hdmi_present_sense(codec, tag, &spec->sink_eld[index]);