Skip to contentSkip to conversation

ALSA: hda: Unify codec construction

PATCH RESEND Applied Cezary Rojewski 3 participants 10 messages Aug 16, 2022 – Aug 18, 2022

Subsystems (4)
Sound ASoC Intel ASoC drivers Sound - sound open firmware (SOF) drivers
lore
Share & tools
lore

Cezary Rojewski

author maintainer
#0 lore raw

v2 cover ALSA: hda: Unify codec construction

A follow up to the recent HDAudio fixes series [1]. Given the recently reported regression [2], before the page fault occurring on codec shutdown can be fixed, codec construction procedure needs to be updated for skylake and sof-intel drivers. Drivers: pci-hda and avs need no changes - already making use of snd_hda_codec_device_init().

As subject touches code used by the sof-driver, additional review has been conducted on thesofproject/linux [3].

Changes in v2:

- dropped snd_hda_ext_core <-> snd_hda_codec dependency by calling snd_hda_codec_device_init() directly in skylake and sof drivers probe enumeration routines, as suggested by Takashi
- skylake/sof portion of the change has been split into two separate patches

- new functions that aim to replace hdac_ext codec init & exit functionality are added first - for skylake and sof drivers both
- third patch in the series now combines the "field -> pointer" change for hdac_hda_priv->codec plus the codec-enumeration adjustments for skylake and sof drivers
Both above are here to keep git bisect happy, as suggested by Pierre

[1]: https://lore.kernel.org/alsa-devel/[email protected]/
[2]: https://lore.kernel.org/alsa-devel/[email protected]/
[3]: https://github.com/thesofproject/linux/pull/3775

Cezary Rojewski (6):
ASoC: Intel: Skylake: Introduce HDA codec init and exit routines
ASoC: SOF: Intel: Introduce HDA codec init and exit routines
ASoC: Intel: Drop hdac_ext usage for codec device creation
ALSA: hda: Always free codec on the device release
ALSA: hda: Remove codec init and exit routines
ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

 include/sound/hda_codec.h                    |  2 -
 include/sound/hdaudio_ext.h                  |  3 --
 sound/hda/ext/hdac_ext_bus.c                 | 53 -------------------
 sound/pci/hda/hda_codec.c                    | 49 ++++++++---------
 sound/soc/codecs/hdac_hda.c                  | 26 ++++-----
 sound/soc/codecs/hdac_hda.h                  |  2 +-
 sound/soc/intel/boards/hda_dsp_common.c      |  2 +-
 sound/soc/intel/boards/skl_hda_dsp_generic.c |  2 +-
 sound/soc/intel/skylake/skl.c                | 53 ++++++++++++++-----
 sound/soc/sof/intel/hda-codec.c              | 55 ++++++++++++++------
 10 files changed, 113 insertions(+), 134 deletions(-)
Signature · 1 line
2.25.1
author maintainer
#1 lore raw

PATCH RESEND v2 1/6 ASoC: Intel: Skylake: Introduce HDA codec init and exit routines

Preliminary step in making snd_hda_codec_device_init() the only constructor for struct hda_codec instances. To do that, existing usage of hdac_ext equivalents has to be dropped.

Reviewed-by Kai Vehmanen <[email protected]>
Reviewed-by Pierre-Louis Bossart <[email protected]>
Signed-off-by Cezary Rojewski <[email protected]>
---
 sound/soc/intel/skylake/skl.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
sound/soc/intel/skylake/skl.c +29 -0
index aeca58246fc7..33b0ed6b0534 100644
@@ -689,6 +689,35 @@ static void load_codec_module(struct hda_codec *codec)
 
 #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */
 
+static void skl_codec_device_exit(struct device *dev)
+{
+	snd_hdac_device_exit(dev_to_hdac_dev(dev));
+}
+
+static __maybe_unused struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr)
+{
+	struct hda_codec *codec;
+	int ret;
+
+	codec = snd_hda_codec_device_init(to_hda_bus(bus), addr, "ehdaudio%dD%d", bus->idx, addr);
+	if (IS_ERR(codec)) {
+		dev_err(bus->dev, "device init failed for hdac device\n");
+		return codec;
+	}
+
+	codec->core.type = HDA_DEV_ASOC;
+	codec->core.dev.release = skl_codec_device_exit;
+
+	ret = snd_hdac_device_register(&codec->core);
+	if (ret) {
+		dev_err(bus->dev, "failed to register hdac device\n");
+		snd_hdac_device_exit(&codec->core);
+		return ERR_PTR(ret);
+	}
+
+	return codec;
+}
+
 /*
  * Probe the given codec address
  */
Signature · 1 line
2.25.1
maintainer Acked-by on v2 1/6
#2 lore raw

Re: [RESEND PATCH v2 1/6] ASoC: Intel: Skylake: Introduce HDA codec init and exit routines

Quoted 3 lines · Cezary Rojewski

Preliminary step in making snd_hda_codec_device_init() the only constructor for struct hda_codec instances. To do that, existing usage of hdac_ext equivalents has to be dropped.

Acked-by Mark Brown <[email protected]>
author maintainer
#3 lore raw

PATCH RESEND v2 2/6 ASoC: SOF: Intel: Introduce HDA codec init and exit routines

In reply to Cezary Rojewski

Preliminary step in making snd_hda_codec_device_init() the only constructor for struct hda_codec instances. To do that, existing usage of hdac_ext equivalents has to be dropped.

Reviewed-by Kai Vehmanen <[email protected]>
Reviewed-by Pierre-Louis Bossart <[email protected]>
Signed-off-by Cezary Rojewski <[email protected]>
Acked-by Mark Brown <[email protected]>
---
 sound/soc/sof/intel/hda-codec.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
sound/soc/sof/intel/hda-codec.c +30 -0
index 2f3f4a733d9e..4c128ba02340 100644
@@ -109,6 +109,36 @@ EXPORT_SYMBOL_NS(hda_codec_jack_check, SND_SOC_SOF_HDA_AUDIO_CODEC);
 #define is_generic_config(x)	0
 #endif
 
+static void hda_codec_device_exit(struct device *dev)
+{
+	snd_hdac_device_exit(dev_to_hdac_dev(dev));
+}
+
+static __maybe_unused struct hda_codec *
+hda_codec_device_init(struct hdac_bus *bus, int addr, int type)
+{
+	struct hda_codec *codec;
+	int ret;
+
+	codec = snd_hda_codec_device_init(to_hda_bus(bus), addr, "ehdaudio%dD%d", bus->idx, addr);
+	if (IS_ERR(codec)) {
+		dev_err(bus->dev, "device init failed for hdac device\n");
+		return codec;
+	}
+
+	codec->core.type = type;
+	codec->core.dev.release = hda_codec_device_exit;
+
+	ret = snd_hdac_device_register(&codec->core);
+	if (ret) {
+		dev_err(bus->dev, "failed to register hdac device\n");
+		snd_hdac_device_exit(&codec->core);
+		return ERR_PTR(ret);
+	}
+
+	return codec;
+}
+
 /* probe individual codec */
 static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 			   bool hda_codec_use_common_hdmi)
Signature · 1 line
2.25.1
author maintainer
#4 lore raw

PATCH RESEND v2 3/6 ASoC: Intel: Drop hdac_ext usage for codec device creation

In reply to Cezary Rojewski

To make snd_hda_codec_device_init() the only constructor for struct hda_codec instances remaining tasks are:

1) no struct may wrap struct hda_codec as its base type
2) bus drivers (skylake and sof) which are the current hdac_ext users need to be adjusted to make use of newly added codec init and exit routines instead
3) as bus drivers (skylake and sof) are to be responsible for creating codec device and assigning it to hdac_hda_priv->codec, hdac_hda_dev_probe() has to be freed of that job

To keep git bisect happy, all of these in made in one-go.

Reviewed-by Kai Vehmanen <[email protected]>
Reviewed-by Pierre-Louis Bossart <[email protected]>
Signed-off-by Cezary Rojewski <[email protected]>
Acked-by Mark Brown <[email protected]>
---
 sound/soc/codecs/hdac_hda.c                  | 26 +++++++-----------
 sound/soc/codecs/hdac_hda.h                  |  2 +-
 sound/soc/intel/boards/hda_dsp_common.c      |  2 +-
 sound/soc/intel/boards/skl_hda_dsp_generic.c |  2 +-
 sound/soc/intel/skylake/skl.c                | 26 ++++++++----------
 sound/soc/sof/intel/hda-codec.c              | 29 ++++++++------------
 6 files changed, 36 insertions(+), 51 deletions(-)
sound/soc/codecs/hdac_hda.c +10 -16
index 8debcee59224..77df4c5b274a 100644
@@ -246,7 +246,7 @@ static int hdac_hda_dai_hw_free(struct snd_pcm_substream *substream,
 		return -EINVAL;
 
 	hda_stream = &pcm->stream[substream->stream];
-	snd_hda_codec_cleanup(&hda_pvt->codec, hda_stream, substream);
+	snd_hda_codec_cleanup(hda_pvt->codec, hda_stream, substream);
 
 	return 0;
 }

@@ -264,7 +264,7 @@ static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
 	int ret = 0;
 
 	hda_pvt = snd_soc_component_get_drvdata(component);
-	hdev = &hda_pvt->codec.core;
+	hdev = &hda_pvt->codec->core;
 	pcm = snd_soc_find_pcm_from_dai(hda_pvt, dai);
 	if (!pcm)
 		return -EINVAL;

@@ -274,7 +274,7 @@ static int hdac_hda_dai_prepare(struct snd_pcm_substream *substream,
 	stream = hda_pvt->pcm[dai->id].stream_tag[substream->stream];
 	format_val = hda_pvt->pcm[dai->id].format_val[substream->stream];
 
-	ret = snd_hda_codec_prepare(&hda_pvt->codec, hda_stream,
+	ret = snd_hda_codec_prepare(hda_pvt->codec, hda_stream,
 				    stream, format_val, substream);
 	if (ret < 0)
 		dev_err(&hdev->dev, "codec prepare failed %d\n", ret);

@@ -299,7 +299,7 @@ static int hdac_hda_dai_open(struct snd_pcm_substream *substream,
 
 	hda_stream = &pcm->stream[substream->stream];
 
-	return hda_stream->ops.open(hda_stream, &hda_pvt->codec, substream);
+	return hda_stream->ops.open(hda_stream, hda_pvt->codec, substream);
 }
 
 static void hdac_hda_dai_close(struct snd_pcm_substream *substream,

@@ -317,7 +317,7 @@ static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
 
 	hda_stream = &pcm->stream[substream->stream];
 
-	hda_stream->ops.close(hda_stream, &hda_pvt->codec, substream);
+	hda_stream->ops.close(hda_stream, hda_pvt->codec, substream);
 
 	snd_hda_codec_pcm_put(pcm);
 }

@@ -325,7 +325,7 @@ static void hdac_hda_dai_close(struct snd_pcm_substream *substream,
 static struct hda_pcm *snd_soc_find_pcm_from_dai(struct hdac_hda_priv *hda_pvt,
 						 struct snd_soc_dai *dai)
 {
-	struct hda_codec *hcodec = &hda_pvt->codec;
+	struct hda_codec *hcodec = hda_pvt->codec;
 	struct hda_pcm *cpcm;
 	const char *pcm_name;
 

@@ -394,8 +394,8 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
 			snd_soc_component_get_drvdata(component);
 	struct snd_soc_dapm_context *dapm =
 			snd_soc_component_get_dapm(component);
-	struct hdac_device *hdev = &hda_pvt->codec.core;
-	struct hda_codec *hcodec = &hda_pvt->codec;
+	struct hdac_device *hdev = &hda_pvt->codec->core;
+	struct hda_codec *hcodec = hda_pvt->codec;
 	struct hdac_ext_link *hlink;
 	hda_codec_patch_t patch;
 	int ret;

@@ -515,8 +515,8 @@ static void hdac_hda_codec_remove(struct snd_soc_component *component)
 {
 	struct hdac_hda_priv *hda_pvt =
 		      snd_soc_component_get_drvdata(component);
-	struct hdac_device *hdev = &hda_pvt->codec.core;
-	struct hda_codec *codec = &hda_pvt->codec;
+	struct hdac_device *hdev = &hda_pvt->codec->core;
+	struct hda_codec *codec = hda_pvt->codec;
 	struct hdac_ext_link *hlink = NULL;
 
 	hlink = snd_hdac_ext_bus_get_link(hdev->bus, dev_name(&hdev->dev));

@@ -584,7 +584,6 @@ static const struct snd_soc_component_driver hdac_hda_codec = {
 static int hdac_hda_dev_probe(struct hdac_device *hdev)
 {
 	struct hdac_ext_link *hlink;
-	struct hdac_hda_priv *hda_pvt;
 	int ret;
 
 	/* hold the ref while we probe */

@@ -595,10 +594,6 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
 	}
 	snd_hdac_ext_bus_link_get(hdev->bus, hlink);
 
-	hda_pvt = hdac_to_hda_priv(hdev);
-	if (!hda_pvt)
-		return -ENOMEM;
-
 	/* ASoC specific initialization */
 	ret = devm_snd_soc_register_component(&hdev->dev,
 					 &hdac_hda_codec, hdac_hda_dais,

@@ -608,7 +603,6 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
 		return ret;
 	}
 
-	dev_set_drvdata(&hdev->dev, hda_pvt);
 	snd_hdac_ext_bus_link_put(hdev->bus, hlink);
 
 	return ret;
sound/soc/codecs/hdac_hda.h +1 -1
index d0efc5e254ae..fc19c34ca00e 100644
@@ -23,7 +23,7 @@ struct hdac_hda_pcm {
 };
 
 struct hdac_hda_priv {
-	struct hda_codec codec;
+	struct hda_codec *codec;
 	struct hdac_hda_pcm pcm[HDAC_LAST_DAI_ID];
 	bool need_display_power;
 };
sound/soc/intel/boards/hda_dsp_common.c +1 -1
index 83c7dfbccd9d..04b7d4f7f9e2 100644
@@ -54,7 +54,7 @@ int hda_dsp_hdmi_build_controls(struct snd_soc_card *card,
 		return -EINVAL;
 
 	hda_pvt = snd_soc_component_get_drvdata(comp);
-	hcodec = &hda_pvt->codec;
+	hcodec = hda_pvt->codec;
 
 	list_for_each_entry(hpcm, &hcodec->pcm_list_head, list) {
 		spcm = hda_dsp_hdmi_pcm_handle(card, i);
sound/soc/intel/boards/skl_hda_dsp_generic.c +1 -1
index 81144efb4b44..879ebba52832 100644
@@ -190,7 +190,7 @@ static void skl_set_hda_codec_autosuspend_delay(struct snd_soc_card *card)
 			 * all codecs are on the same bus, so it's sufficient
 			 * to look up only the first one
 			 */
-			snd_hda_set_power_save(hda_pvt->codec.bus,
+			snd_hda_set_power_save(hda_pvt->codec->bus,
 					       HDA_CODEC_AUTOSUSPEND_DELAY_MS);
 			break;
 		}
sound/soc/intel/skylake/skl.c +11 -15
index 33b0ed6b0534..c7c1cad2a753 100644
@@ -694,7 +694,7 @@ static void skl_codec_device_exit(struct device *dev)
 	snd_hdac_device_exit(dev_to_hdac_dev(dev));
 }
 
-static __maybe_unused struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr)
+static struct hda_codec *skl_codec_device_init(struct hdac_bus *bus, int addr)
 {
 	struct hda_codec *codec;
 	int ret;

@@ -729,9 +729,8 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	struct skl_dev *skl = bus_to_skl(bus);
 #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC)
 	struct hdac_hda_priv *hda_codec;
-	int err;
 #endif
-	struct hdac_device *hdev;
+	struct hda_codec *codec;
 
 	mutex_lock(&bus->cmd_mutex);
 	snd_hdac_bus_send_cmd(bus, cmd);

@@ -747,25 +746,22 @@ static int probe_codec(struct hdac_bus *bus, int addr)
 	if (!hda_codec)
 		return -ENOMEM;
 
-	hda_codec->codec.bus = skl_to_hbus(skl);
-	hdev = &hda_codec->codec.core;
+	codec = skl_codec_device_init(bus, addr);
+	if (IS_ERR(codec))
+		return PTR_ERR(codec);
 
-	err = snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
-	if (err < 0)
-		return err;
+	hda_codec->codec = codec;
+	dev_set_drvdata(&codec->core.dev, hda_codec);
 
 	/* use legacy bus only for HDA codecs, idisp uses ext bus */
 	if ((res & 0xFFFF0000) != IDISP_INTEL_VENDOR_ID) {
-		hdev->type = HDA_DEV_LEGACY;
-		load_codec_module(&hda_codec->codec);
+		codec->core.type = HDA_DEV_LEGACY;
+		load_codec_module(hda_codec->codec);
 	}
 	return 0;
 #else
-	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
-	if (!hdev)
-		return -ENOMEM;
-
-	return snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
+	codec = skl_codec_device_init(bus, addr);
+	return PTR_ERR_OR_ZERO(codec);
 #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */
 }
sound/soc/sof/intel/hda-codec.c +12 -17
index 4c128ba02340..73336648cd25 100644
@@ -114,8 +114,7 @@ static void hda_codec_device_exit(struct device *dev)
 	snd_hdac_device_exit(dev_to_hdac_dev(dev));
 }
 
-static __maybe_unused struct hda_codec *
-hda_codec_device_init(struct hdac_bus *bus, int addr, int type)
+static struct hda_codec *hda_codec_device_init(struct hdac_bus *bus, int addr, int type)
 {
 	struct hda_codec *codec;
 	int ret;

@@ -145,11 +144,10 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 {
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
 	struct hdac_hda_priv *hda_priv;
-	struct hda_codec *codec;
 	int type = HDA_DEV_LEGACY;
 #endif
 	struct hda_bus *hbus = sof_to_hbus(sdev);
-	struct hdac_device *hdev;
+	struct hda_codec *codec;
 	u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) |
 		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
 	u32 resp = -1;

@@ -172,20 +170,20 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 	if (!hda_priv)
 		return -ENOMEM;
 
-	hda_priv->codec.bus = hbus;
-	hdev = &hda_priv->codec.core;
-	codec = &hda_priv->codec;
-
 	/* only probe ASoC codec drivers for HDAC-HDMI */
 	if (!hda_codec_use_common_hdmi && (resp & 0xFFFF0000) == IDISP_VID_INTEL)
 		type = HDA_DEV_ASOC;
 
-	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, type);
+	codec = hda_codec_device_init(&hbus->core, address, type);
+	ret = PTR_ERR_OR_ZERO(codec);
 	if (ret < 0)
 		return ret;
 
+	hda_priv->codec = codec;
+	dev_set_drvdata(&codec->core.dev, hda_priv);
+
 	if ((resp & 0xFFFF0000) == IDISP_VID_INTEL) {
-		if (!hdev->bus->audio_component) {
+		if (!hbus->core.audio_component) {
 			dev_dbg(sdev->dev,
 				"iDisp hw present but no driver\n");
 			ret = -ENOENT;

@@ -211,15 +209,12 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 
 out:
 	if (ret < 0) {
-		snd_hdac_device_unregister(hdev);
-		put_device(&hdev->dev);
+		snd_hdac_device_unregister(&codec->core);
+		put_device(&codec->core.dev);
 	}
 #else
-	hdev = devm_kzalloc(sdev->dev, sizeof(*hdev), GFP_KERNEL);
-	if (!hdev)
-		return -ENOMEM;
-
-	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, HDA_DEV_ASOC);
+	codec = hda_codec_device_init(&hbus->core, address);
+	ret = PTR_ERR_OR_ZERO(codec);
 #endif
 
 	return ret;
Signature · 1 line
2.25.1
author maintainer
#5 lore raw

PATCH RESEND v2 4/6 ALSA: hda: Always free codec on the device release

In reply to Cezary Rojewski

With all HDAudio drivers aligned to make use of the same constructor, have codec freed on the device release regardless of its type.

Reviewed-by Kai Vehmanen <[email protected]>
Reviewed-by Pierre-Louis Bossart <[email protected]>
Signed-off-by Cezary Rojewski <[email protected]>
---
 sound/pci/hda/hda_codec.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
sound/pci/hda/hda_codec.c +1 -7
index 384426d7e9dd..aa7a362be290 100644
@@ -883,13 +883,7 @@ static void snd_hda_codec_dev_release(struct device *dev)
 	snd_hda_sysfs_clear(codec);
 	kfree(codec->modelname);
 	kfree(codec->wcaps);
-
-	/*
-	 * In the case of ASoC HD-audio, hda_codec is device managed.
-	 * It will be freed when the ASoC device is removed.
-	 */
-	if (codec->core.type == HDA_DEV_LEGACY)
-		kfree(codec);
+	kfree(codec);
 }
 
 #define DEV_NAME_LEN 31
Signature · 1 line
2.25.1
author maintainer
#6 lore raw

PATCH RESEND v2 5/6 ALSA: hda: Remove codec init and exit routines

In reply to Cezary Rojewski

There are no users for snd_hdac_ext_bus_device_init() and snd_hdac_ext_bus_device_exit().

While at it, remove hdac_to_hda_priv() too for the exact same reason.

Reviewed-by Kai Vehmanen <[email protected]>
Reviewed-by Pierre-Louis Bossart <[email protected]>
Signed-off-by Cezary Rojewski <[email protected]>
---
 include/sound/hda_codec.h    |  2 --
 include/sound/hdaudio_ext.h  |  3 --
 sound/hda/ext/hdac_ext_bus.c | 53 ------------------------------------
 3 files changed, 58 deletions(-)
include/sound/hda_codec.h +0 -2
index 6d3c82c4b6ac..2a8fe7240f10 100644
@@ -293,8 +293,6 @@ struct hda_codec {
 #define dev_to_hda_codec(_dev)	container_of(_dev, struct hda_codec, core.dev)
 #define hda_codec_dev(_dev)	(&(_dev)->core.dev)
 
-#define hdac_to_hda_priv(_hdac) \
-			container_of(_hdac, struct hdac_hda_priv, codec.core)
 #define hdac_to_hda_codec(_hdac) container_of(_hdac, struct hda_codec, core)
 
 #define list_for_each_codec(c, bus) \
include/sound/hdaudio_ext.h +0 -3
index d26234f9ee46..88ebb64fd8a5 100644
@@ -11,9 +11,6 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 		      const struct hdac_ext_bus_ops *ext_ops);
 
 void snd_hdac_ext_bus_exit(struct hdac_bus *bus);
-int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
-				struct hdac_device *hdev, int type);
-void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev);
 void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus);
 
 #define HDA_CODEC_REV_EXT_ENTRY(_vid, _rev, _name, drv_data) \
sound/hda/ext/hdac_ext_bus.c +0 -53

59 lines not on this page · raw message

Signature · 1 line
2.25.1
author maintainer
#7 lore raw

PATCH RESEND v2 6/6 ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

In reply to Cezary Rojewski

If early probe of HDAudio bus driver fails e.g.: due to missing firmware file, snd_hda_codec_shutdown() ends in manipulating uninitialized codec->pcm_list_head causing page fault.

Initialization of HDAudio codec in ASoC is split in two:
- snd_hda_codec_device_init()
- snd_hda_codec_device_new()

snd_hda_codec_device_init() is called during probe_codecs() by HDAudio bus driver while snd_hda_codec_device_new() is called by codec-component's ->probe(). The second call will not happen until all components required by related sound card are present within the ASoC framework. With firmware failing to load during the PCI's deferred initialization i.e.: probe_work(), no platform components are ever registered. HDAudio codec enumeration is done at that point though, so the codec components became registered to ASoC framework, calling snd_hda_codec_device_init() in the process.

Now, during platform reboot snd_hda_codec_shutdown() is called for every codec found on the HDAudio bus causing oops if any of them has not completed both of their initialization steps. Relocating field initialization fixes the issue.

Reviewed-by Kai Vehmanen <[email protected]>
Reviewed-by Pierre-Louis Bossart <[email protected]>
Signed-off-by Cezary Rojewski <[email protected]>
---
 sound/pci/hda/hda_codec.c | 41 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 21 deletions(-)
sound/pci/hda/hda_codec.c +20 -21
index aa7a362be290..b4d1e658c556 100644
@@ -925,8 +925,28 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
 	}
 
 	codec->bus = bus;
+	codec->depop_delay = -1;
+	codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
+	codec->core.dev.release = snd_hda_codec_dev_release;
+	codec->core.exec_verb = codec_exec_verb;
 	codec->core.type = HDA_DEV_LEGACY;
 
+	mutex_init(&codec->spdif_mutex);
+	mutex_init(&codec->control_mutex);
+	snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32);
+	snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32);
+	snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16);
+	snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16);
+	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
+	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
+	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
+	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
+	INIT_LIST_HEAD(&codec->conn_list);
+	INIT_LIST_HEAD(&codec->pcm_list_head);
+	INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
+	refcount_set(&codec->pcm_ref, 1);
+	init_waitqueue_head(&codec->remove_sleep);
+
 	return codec;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_device_init);

@@ -979,29 +999,8 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
 	if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
 		return -EINVAL;
 
-	codec->core.dev.release = snd_hda_codec_dev_release;
-	codec->core.exec_verb = codec_exec_verb;
-
 	codec->card = card;
 	codec->addr = codec_addr;
-	mutex_init(&codec->spdif_mutex);
-	mutex_init(&codec->control_mutex);
-	snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32);
-	snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32);
-	snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16);
-	snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16);
-	snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
-	snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
-	snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16);
-	snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
-	INIT_LIST_HEAD(&codec->conn_list);
-	INIT_LIST_HEAD(&codec->pcm_list_head);
-	refcount_set(&codec->pcm_ref, 1);
-	init_waitqueue_head(&codec->remove_sleep);
-
-	INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
-	codec->depop_delay = -1;
-	codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
 
 #ifdef CONFIG_PM
 	codec->power_jiffies = jiffies;
Signature · 1 line
2.25.1
author maintainer
#8 lore raw
In reply to Cezary Rojewski

On 2022-08-16 1:17 PM, Cezary Rojewski wrote:

...

Quoted 7 lines

Cezary Rojewski (6):
ASoC: Intel: Skylake: Introduce HDA codec init and exit routines
ASoC: SOF: Intel: Introduce HDA codec init and exit routines
ASoC: Intel: Drop hdac_ext usage for codec device creation
ALSA: hda: Always free codec on the device release
ALSA: hda: Remove codec init and exit routines
ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

All of these landed on lore.kernel.org without problems now.

The only thing I can think of that could offended the server is the incorrect email address that I had appended to hsw-bdw related series [1] which I'd sent before the unify-hda-ctor v2 one. Kai's email was provided twice there - by mistake - and the second copy is missing 'm' in '@linux.intel.com'. Perhaps these topics are not connected but it's the only offending action that I've managed to find.

Regards,
Czarek

Applied
#9 lore raw
In reply to Cezary Rojewski
Quoted 29 lines · Cezary Rojewski

A follow up to the recent HDAudio fixes series [1]. Given the recently reported regression [2], before the page fault occurring on codec shutdown can be fixed, codec construction procedure needs to be updated for skylake and sof-intel drivers. Drivers: pci-hda and avs need no changes - already making use of snd_hda_codec_device_init().

As subject touches code used by the sof-driver, additional review has been conducted on thesofproject/linux [3].

Changes in v2:

- dropped snd_hda_ext_core <-> snd_hda_codec dependency by calling snd_hda_codec_device_init() directly in skylake and sof drivers probe enumeration routines, as suggested by Takashi
- skylake/sof portion of the change has been split into two separate patches

- new functions that aim to replace hdac_ext codec init & exit functionality are added first - for skylake and sof drivers both
- third patch in the series now combines the "field -> pointer" change for hdac_hda_priv->codec plus the codec-enumeration adjustments for skylake and sof drivers
Both above are here to keep git bisect happy, as suggested by Pierre

[1]: https://lore.kernel.org/alsa-devel/[email protected]/
[2]: https://lore.kernel.org/alsa-devel/[email protected]/
[3]: https://github.com/thesofproject/linux/pull/3775

Quoted 7 lines

Cezary Rojewski (6):
ASoC: Intel: Skylake: Introduce HDA codec init and exit routines
ASoC: SOF: Intel: Introduce HDA codec init and exit routines
ASoC: Intel: Drop hdac_ext usage for codec device creation
ALSA: hda: Always free codec on the device release
ALSA: hda: Remove codec init and exit routines
ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

Applied all patches now to for-next branch. Thanks.

Takashi