Comment 8 for bug 1040030

At Wed, 22 Aug 2012 16:27:35 +0200,
David Henningsson wrote:
>
> On 08/22/2012 03:52 PM, Takashi Iwai wrote:
> > At Wed, 22 Aug 2012 15:46:59 +0200,
> > David Henningsson wrote:
> >>
> >> On 08/22/2012 02:58 PM, Takashi Iwai wrote:
> >>> At Wed, 22 Aug 2012 14:39:17 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>> On 08/22/2012 02:22 PM, Takashi Iwai wrote:
> >>>>> At Wed, 22 Aug 2012 14:01:41 +0200,
> >>>>> David Henningsson wrote:
> >>>>>>
> >>>>>> The HDMI codec (an NVIDIA one in this case) forgot that its pins
> >>>>>> were unsol enabled, while it was suspended. Therefore jack detection
> >>>>>> was broken after S3.
> >>>>>> With this patch, we reenable the unsol events on resume,
> >>>>>> and also do an extra check afterwards, to see if the HDMI monitor was
> >>>>>> plugged/unplugged while in S3.
> >>>>>>
> >>>>>> Cc: <email address hidden> (3.3+)
> >>>>>> BugLink: https://bugs.launchpad.net/bugs/1040030
> >>>>>> Signed-off-by: David Henningsson <email address hidden>
> >>>>>> ---
> >>>>>> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++
> >>>>>> 1 file changed, 13 insertions(+)
> >>>>>>
> >>>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >>>>>> index 8f23374..6a3ac05 100644
> >>>>>> --- a/sound/pci/hda/patch_hdmi.c
> >>>>>> +++ b/sound/pci/hda/patch_hdmi.c
> >>>>>> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec)
> >>>>>> return 0;
> >>>>>> }
> >>>>>>
> >>>>>> +#ifdef CONFIG_PM
> >>>>>> +static int generic_hdmi_resume(struct hda_codec *codec)
> >>>>>> +{
> >>>>>> + snd_hda_codec_resume_cache(codec);
> >>>>>> + snd_hda_jack_set_dirty_all(codec);
> >>>>>> + snd_hda_jack_report_sync(codec);
> >>>>>> + return 0;
> >>>>>
> >>>>> Hm, is this really needed?
> >>>>>
> >>>>> snd_hda_jack_set_dirty_all() is already called in
> >>>>> hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in
> >>>>> the init callback.
> >>>>
> >>>> The tester (who has the hardware) has gone for the day, so I can't
> >>>> really verify different scenarios right now, but after having looked at
> >>>> hda_call_codec_resume I see what you mean...
> >>>>
> >>>> I do notice one difference though - the order.
> >>>> snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs,
> >>>> should probably be before the set_dirty_all / report_sync. If not for
> >>>> anything else, so for the race condition of somebody plugging/unplugging
> >>>> the monitor after checking the jack but before the unsol is enabled.
> >>>
> >>> Calling snd_hda_jack_report_sync() in init means that all jacks are
> >>> checked at first, then the caches are resumed. So this won't change
> >>> ENABLE_UNSOL verb setups.
> >>
> >> I'm not sure I'm following. Here's the order in hda_call_codec_resume:
> >>
> >> snd_hda_jack_set_dirty_all()
> >> generic_hdmi_init -> snd_hda_jack_report_sync() - get_pin_sense
> >> snd_hda_codec_resume_cache() - set_unsol_enable
> >>
> >> With the (theoretical?) race condition being a change of pin sense
> >> between snd_hda_jack_report_sync() and snd_hda_codec_resume_cache(),
> >> which will not be picked up.
> >
> > In that race, yes. But this should be irrelevant with this bug :)
>
> Yes, the bug needs further verification.
>
> >> The patch changed the order to:
> >>
> >> snd_hda_codec_resume_cache() - set_unsol_enable
> >> snd_hda_jack_set_dirty_all()
> >> snd_hda_jack_report_sync() - get_pin_sense
> >>
> >> Which eliminates that race condition.
> >
> > Well, what I'm thinking now is rather to call
> > snd_hda_jack_report_sync() generically in hda_call_codec_resume().
> > The call doesn't have to be in all places.
> > Ditto for the initialization case. It'll clean up many lines.
>
> Fine with me - the more code being generic, the better.

OK, here we go.

Takashi

===

From: Takashi Iwai <email address hidden>
Subject: [PATCH] ALSA: hda - Call snd_hda_jack_report_sync() generically in hda_codec.c

Instead of calling the jack sync in the init callback of each codec,
call it generically at initialization and resume. By calling it at
the last of resume sequence, a possible race between the jack sync and
the unsol event enablement in the current code will be closed, too.

Signed-off-by: Takashi Iwai <email address hidden>
---
 sound/pci/hda/hda_codec.c | 2 ++
 sound/pci/hda/patch_cirrus.c | 2 --
 sound/pci/hda/patch_conexant.c | 1 -
 sound/pci/hda/patch_hdmi.c | 2 --
 sound/pci/hda/patch_realtek.c | 2 --
 sound/pci/hda/patch_sigmatel.c | 2 --
 sound/pci/hda/patch_via.c | 1 -
 7 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index a6c34dc..4efd271 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3618,6 +3618,7 @@ static void hda_call_codec_resume(struct hda_codec *codec)
   snd_hda_codec_resume_amp(codec);
   snd_hda_codec_resume_cache(codec);
  }
+ snd_hda_jack_report_sync(codec);
  snd_hda_power_down(codec); /* flag down before returning */
 }
 #endif /* CONFIG_PM */
@@ -3663,6 +3664,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec)
   err = codec->patch_ops.build_controls(codec);
  if (err < 0)
   return err;
+ snd_hda_jack_report_sync(codec); /* call at the last init point */
  return 0;
 }

diff --git a/sound/pci/hda/patch_cirrus.c b/sound/pci/hda/patch_cirrus.c
index 0c4c1a6..0bddb3e 100644
--- a/sound/pci/hda/patch_cirrus.c
+++ b/sound/pci/hda/patch_cirrus.c
@@ -1193,7 +1193,6 @@ static int cs_init(struct hda_codec *codec)
  init_output(codec);
  init_input(codec);
  init_digital(codec);
- snd_hda_jack_report_sync(codec);

  return 0;
 }
@@ -1643,7 +1642,6 @@ static int cs421x_init(struct hda_codec *codec)
  init_output(codec);
  init_input(codec);
  init_cs421x_digital(codec);
- snd_hda_jack_report_sync(codec);

  return 0;
 }
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 5e22a8f..172895a 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -4061,7 +4061,6 @@ static int cx_auto_init(struct hda_codec *codec)
  cx_auto_init_output(codec);
  cx_auto_init_input(codec);
  cx_auto_init_digital(codec);
- snd_hda_jack_report_sync(codec);
  snd_hda_sync_vmaster_hook(&spec->vmaster_mute);
  return 0;
 }
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 8f23374..d9439c5 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1311,7 +1311,6 @@ static int generic_hdmi_init(struct hda_codec *codec)
   hdmi_init_pin(codec, pin_nid);
   snd_hda_jack_detect_enable(codec, pin_nid, pin_nid);
  }
- snd_hda_jack_report_sync(codec);
  return 0;
 }

@@ -1428,7 +1427,6 @@ static int simple_playback_init(struct hda_codec *codec)
   snd_hda_codec_write(codec, pin, 0, AC_VERB_SET_AMP_GAIN_MUTE,
         AMP_OUT_UNMUTE);
  snd_hda_jack_detect_enable(codec, pin, pin);
- snd_hda_jack_report_sync(codec);
  return 0;
 }

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 4f81dd4..ce99cc9 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -2053,8 +2053,6 @@ static int alc_init(struct hda_codec *codec)

  alc_apply_fixup(codec, ALC_FIXUP_ACT_INIT);

- snd_hda_jack_report_sync(codec);
-
  hda_call_check_power_status(codec, 0x01);
  return 0;
 }
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
index ea5775a..4352954 100644
--- a/sound/pci/hda/patch_sigmatel.c
+++ b/sound/pci/hda/patch_sigmatel.c
@@ -4418,8 +4418,6 @@ static int stac92xx_init(struct hda_codec *codec)
   stac_toggle_power_map(codec, nid, 0);
  }

- snd_hda_jack_report_sync(codec);
-
  /* sync mute LED */
  if (spec->gpio_led) {
   if (spec->vmaster_mute.hook)
diff --git a/sound/pci/hda/patch_via.c b/sound/pci/hda/patch_via.c
index 4307717..4b0796b 100644
--- a/sound/pci/hda/patch_via.c
+++ b/sound/pci/hda/patch_via.c
@@ -2815,7 +2815,6 @@ static int via_init(struct hda_codec *codec)

  via_hp_automute(codec);
  vt1708_update_hp_work(spec);
- snd_hda_jack_report_sync(codec);

  return 0;
 }
--
1.7.11.4