| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <871qrvgbsr.wl-tiwai@suse.de>
Date: Wed, 28 Sep 2022 10:09:40 +0200
From: Takashi Iwai <tiwai@...e.de>
To: "Lu, Brent" <brent.lu@...el.com>
Cc: "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"Jaroslav Kysela" <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
Kai Vehmanen <kai.vehmanen@...ux.intel.com>,
Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
Mohan Kumar <mkumard@...dia.com>,
Ville Syrjälä
<ville.syrjala@...ux.intel.com>, "Zhi, Yong" <yong.zhi@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ALSA: hda/hdmi: run eld notify in delay work
On Wed, 28 Sep 2022 09:14:30 +0200,
Takashi Iwai wrote:
>
> On Wed, 28 Sep 2022 04:06:45 +0200,
> Lu, Brent wrote:
> >
> > > >
> > > > During resolution change, display driver would disable HDMI audio then
> > > > enable it in a short time. There is possibility that eld notify for
> > > > HDMI audio enable is called when previous runtime suspend is still
> > > > running. In this case, the elf nofity just returns and not updating
> > > > the status of corresponding HDMI pin/port. Here we move the eld nofity
> > > > to a delay work so we don't lose it.
> > > >
> > > > Signed-off-by: Brent Lu <brent.lu@...el.com>
> > >
> > > We have already a dedicated per-pin work for the delayed ELD check.
> > > Can we reuse it instead of inventing yet another work?
> > > More work needs more cares, and better to avoid unless really needed (e.g.
> > > you forgot cleanup at suspend/removal in this patch).
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> > Hi Takashi,
> >
> > I've checked the hdmi_repoll_eld() and check_presence_and_report() function to see
> > if we can reuse the per-pin work. I've some questions about reusing the per-pin work:
> >
> > 1. hdmi_repoll_eld() calls snd_hda_jack_tbl_get_mst() function while
> > check_presence_and_report() doesn't. Is it ok?
>
> For the system with the audio component, there is no jack entry, hence
> this will be ignored.
>
> > 2. snd_hdac_i915_set_bclk() is called in intel_pin_eld_notify() function. Since it's
> > skipped, we need to call it in the per-pin work. Need to add a flag in hdmi_spec_per_pin
> > to indicate this situation.
>
> Yeah, I guess this was already a bug. It implies that the set_bclk()
> call is missing in the suspend/resume case, too. We need to call it
> more consistently.
>
> > 3. We can schedule the per-pin work in intel_pin_eld_notify() when snd_hdac_is_in_pm()
> > returns true but there is no guarantee the runtime suspend will finished when the per-pin
> > work is schedule to run.
>
> On the second thought, we may simply proceed the notification if it's
> in a valid context. The only period to prohibit the update is during
> the suspend/resume until the ELD is updated by the resume itself.
> So, something like below may work instead. Could you give it a try?
A correction in the patch, it still has to check in-pm state;
otherwise it won't be handled when runtime-suspended.
Takashi
-- 8< --
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -83,6 +83,7 @@ struct hdmi_spec_per_pin {
int pcm_idx; /* which pcm is attached. -1 means no pcm is attached */
int repoll_count;
bool setup; /* the stream has been set up by prepare callback */
+ bool eld_update_frozen;
bool silent_stream;
int channels; /* current number of channels */
bool non_pcm;
@@ -788,16 +789,28 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll);
-static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid,
- int dev_id)
+static struct hdmi_spec_per_pin *
+get_pin_from_nid(struct hda_codec *codec, hda_nid_t nid, int dev_id)
{
struct hdmi_spec *spec = codec->spec;
int pin_idx = pin_id_to_pin_index(codec, nid, dev_id);
if (pin_idx < 0)
+ return NULL;
+ return get_pin(spec, pin_idx);
+}
+
+static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid,
+ int dev_id)
+{
+ struct hdmi_spec *spec = codec->spec;
+ struct hdmi_spec_per_pin *per_pin;
+
+ per_pin = get_pin_from_nid(codec, nid, dev_id);
+ if (!per_pin)
return;
mutex_lock(&spec->pcm_lock);
- hdmi_present_sense(get_pin(spec, pin_idx), 1);
+ hdmi_present_sense(per_pin, 1);
mutex_unlock(&spec->pcm_lock);
}
@@ -1582,6 +1595,7 @@ static void update_eld(struct hda_codec *codec,
snd_jack_report(pcm_jack,
(eld->monitor_present && eld->eld_valid) ?
SND_JACK_AVOUT : 0);
+ per_pin->eld_update_frozen = false;
}
/* update ELD and jack state via HD-audio verbs */
@@ -2494,6 +2508,7 @@ static int generic_hdmi_suspend(struct hda_codec *codec)
for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
cancel_delayed_work_sync(&per_pin->work);
+ per_pin->eld_update_frozen = true;
}
return 0;
}
@@ -2656,6 +2671,7 @@ static void generic_acomp_pin_eld_notify(void *audio_ptr, int port, int dev_id)
struct hda_codec *codec = audio_ptr;
struct hdmi_spec *spec = codec->spec;
hda_nid_t pin_nid = spec->port2pin(codec, port);
+ struct hdmi_spec_per_pin *per_pin;
if (!pin_nid)
return;
@@ -2667,7 +2683,9 @@ static void generic_acomp_pin_eld_notify(void *audio_ptr, int port, int dev_id)
if (codec->core.dev.power.power_state.event == PM_EVENT_SUSPEND)
return;
/* ditto during suspend/resume process itself */
- if (snd_hdac_is_in_pm(&codec->core))
+ per_pin = get_pin_from_nid(codec, pin_nid, dev_id);
+ if (!per_pin || (per_pin->eld_update_frozen &&
+ snd_hdac_is_in_pm(&codec->core)))
return;
check_presence_and_report(codec, pin_nid, dev_id);
@@ -2841,6 +2859,7 @@ static int intel_port2pin(struct hda_codec *codec, int port)
static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
{
struct hda_codec *codec = audio_ptr;
+ struct hdmi_spec_per_pin *per_pin;
int pin_nid;
int dev_id = pipe;
@@ -2853,7 +2872,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
if (codec->core.dev.power.power_state.event == PM_EVENT_SUSPEND)
return;
/* ditto during suspend/resume process itself */
- if (snd_hdac_is_in_pm(&codec->core))
+ per_pin = get_pin_from_nid(codec, pin_nid, dev_id);
+ if (!per_pin || (per_pin->eld_update_frozen &&
+ snd_hdac_is_in_pm(&codec->core)))
return;
snd_hdac_i915_set_bclk(&codec->bus->core);
Powered by blists - more mailing lists