* [PATCH 0/4] ALSA: hda: Unify codec construction
@ 2022-07-20 13:06 Cezary Rojewski
2022-07-20 13:06 ` [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec Cezary Rojewski
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 13:06 UTC (permalink / raw)
To: alsa-devel, broonie, tiwai
Cc: Cezary Rojewski, kai.vehmanen, yung-chuan.liao, lgirdwood,
pierre-louis.bossart, hdegoede, ranjani.sridharan,
amadeuszx.slawinski, peter.ujfalusi
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().
[1]: https://lore.kernel.org/alsa-devel/20220706120230.427296-7-cezary.rojewski@intel.com/
[2]: https://lore.kernel.org/alsa-devel/3c40df55-3aee-1e08-493b-7b30cd84dc00@linux.intel.com/
Cezary Rojewski (4):
ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec
ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
ALSA: hda: Always free codec on the device release
ALSA: hda: Fix page fault in snd_hda_codec_shutdown()
include/sound/hda_codec.h | 2 -
include/sound/hdaudio_ext.h | 4 +-
sound/hda/ext/hdac_ext_bus.c | 34 ++++++--------
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 | 24 ++++------
sound/soc/sof/intel/hda-codec.c | 29 +++++-------
10 files changed, 73 insertions(+), 101 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec
2022-07-20 13:06 [PATCH 0/4] ALSA: hda: Unify codec construction Cezary Rojewski
@ 2022-07-20 13:06 ` Cezary Rojewski
2022-07-20 13:56 ` Mark Brown
2022-07-20 13:06 ` [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor Cezary Rojewski
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 13:06 UTC (permalink / raw)
To: alsa-devel, broonie, tiwai
Cc: Cezary Rojewski, kai.vehmanen, yung-chuan.liao, lgirdwood,
pierre-louis.bossart, hdegoede, ranjani.sridharan,
amadeuszx.slawinski, peter.ujfalusi
Preliminary step in making snd_hda_codec_device_init() the only
constructor for struct hda_codec instances. To do that, no struct may
wrap struct hda_codec as its base type.
To drop hdac_to_hda_priv(), hdac_hda_dev_probe() now skips
dev_set_drvdata(). Driver private data will be assigned by owning bus
drivers instead.
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
include/sound/hda_codec.h | 2 --
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 | 6 ++---
sound/soc/sof/intel/hda-codec.c | 6 ++---
7 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 6d3c82c4b6ac..2a8fe7240f10 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -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) \
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 8debcee59224..77df4c5b274a 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -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;
diff --git a/sound/soc/codecs/hdac_hda.h b/sound/soc/codecs/hdac_hda.h
index d0efc5e254ae..fc19c34ca00e 100644
--- a/sound/soc/codecs/hdac_hda.h
+++ b/sound/soc/codecs/hdac_hda.h
@@ -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;
};
diff --git a/sound/soc/intel/boards/hda_dsp_common.c b/sound/soc/intel/boards/hda_dsp_common.c
index 83c7dfbccd9d..04b7d4f7f9e2 100644
--- a/sound/soc/intel/boards/hda_dsp_common.c
+++ b/sound/soc/intel/boards/hda_dsp_common.c
@@ -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);
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index 81144efb4b44..879ebba52832 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -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;
}
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index aeca58246fc7..7e573a887118 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -718,8 +718,8 @@ 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;
+ hda_codec->codec->bus = skl_to_hbus(skl);
+ hdev = &hda_codec->codec->core;
err = snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
if (err < 0)
@@ -728,7 +728,7 @@ static int probe_codec(struct hdac_bus *bus, int addr)
/* 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);
+ load_codec_module(hda_codec->codec);
}
return 0;
#else
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 2f3f4a733d9e..de7c53224ac3 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -142,9 +142,9 @@ 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;
+ 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)
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
2022-07-20 13:06 [PATCH 0/4] ALSA: hda: Unify codec construction Cezary Rojewski
2022-07-20 13:06 ` [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec Cezary Rojewski
@ 2022-07-20 13:06 ` Cezary Rojewski
2022-07-20 13:13 ` Takashi Iwai
2022-07-20 17:06 ` kernel test robot
2022-07-20 13:06 ` [PATCH 3/4] ALSA: hda: Always free codec on the device release Cezary Rojewski
2022-07-20 13:06 ` [PATCH 4/4] ALSA: hda: Fix page fault in snd_hda_codec_shutdown() Cezary Rojewski
3 siblings, 2 replies; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 13:06 UTC (permalink / raw)
To: alsa-devel, broonie, tiwai
Cc: Cezary Rojewski, kai.vehmanen, yung-chuan.liao, lgirdwood,
pierre-louis.bossart, hdegoede, ranjani.sridharan,
amadeuszx.slawinski, peter.ujfalusi
Refactor snd_hdac_ext_bus_device_init() so that it makes use of
snd_hda_codec_device_init() to create and initialize new codec device.
Causes the latter to become the sole codec device constructor.
Users of the refactored function are updated accordingly and now also
take responsibility for assigning driver's private data as that task is
no longer performed by hdac_hda_dev_probe().
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
include/sound/hdaudio_ext.h | 4 ++--
sound/hda/ext/hdac_ext_bus.c | 34 +++++++++++++++------------------
sound/soc/intel/skylake/skl.c | 24 ++++++++++-------------
sound/soc/sof/intel/hda-codec.c | 29 ++++++++++++----------------
4 files changed, 39 insertions(+), 52 deletions(-)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index d26234f9ee46..25c7b13db278 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -11,8 +11,8 @@ 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);
+struct hda_codec *
+snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type);
void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev);
void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus);
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index 765c40a6ccba..bd3c7124aca1 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -12,6 +12,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/io.h>
+#include <sound/hda_codec.h>
#include <sound/hdaudio_ext.h>
MODULE_DESCRIPTION("HDA extended core");
@@ -67,39 +68,34 @@ static void default_release(struct device *dev)
/**
* snd_hdac_ext_bus_device_init - initialize the HDA extended codec base device
- * @bus: hdac bus to attach to
+ * @bus: hda bus to attach to
* @addr: codec address
- * @hdev: hdac device to init
* @type: codec type (HDAC_DEV_*) to use for this device
*
- * Returns zero for success or a negative error code.
+ * Returns pointer to newly created codec or ERR_PTR.
*/
-int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
- struct hdac_device *hdev, int type)
+struct hda_codec *
+snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type)
{
- char name[15];
+ struct hda_codec *codec;
int ret;
- hdev->bus = bus;
-
- snprintf(name, sizeof(name), "ehdaudio%dD%d", bus->idx, addr);
-
- ret = snd_hdac_device_init(hdev, bus, name, addr);
- if (ret < 0) {
+ 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 ret;
+ return codec;
}
- hdev->type = type;
- hdev->dev.release = default_release;
+ codec->core.type = type;
+ codec->core.dev.release = default_release;
- ret = snd_hdac_device_register(hdev);
+ ret = snd_hdac_device_register(&codec->core);
if (ret) {
dev_err(bus->dev, "failed to register hdac device\n");
- snd_hdac_ext_bus_device_exit(hdev);
- return ret;
+ snd_hdac_ext_bus_device_exit(&codec->core);
+ return ERR_PTR(ret);
}
- return 0;
+ return codec;
}
EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 7e573a887118..5637292c2aa9 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -700,9 +700,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);
@@ -718,25 +717,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 = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC);
+ 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(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 = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC);
+ return PTR_ERR_OR_ZERO(codec);
#endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */
}
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index de7c53224ac3..7c3ea4a12d63 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -115,11 +115,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;
@@ -142,20 +141,19 @@ 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);
- if (ret < 0)
- return ret;
+ codec = snd_hdac_ext_bus_device_init(&hbus->core, address, type);
+ if (IS_ERR(codec))
+ return PTR_ERR(codec);
+
+ 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;
@@ -181,15 +179,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 = snd_hdac_ext_bus_device_init(&hbus->core, address, HDA_DEV_ASOC);
+ ret = PTR_ERR_OR_ZERO(codec);
#endif
return ret;
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] ALSA: hda: Always free codec on the device release
2022-07-20 13:06 [PATCH 0/4] ALSA: hda: Unify codec construction Cezary Rojewski
2022-07-20 13:06 ` [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec Cezary Rojewski
2022-07-20 13:06 ` [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor Cezary Rojewski
@ 2022-07-20 13:06 ` Cezary Rojewski
2022-07-20 13:06 ` [PATCH 4/4] ALSA: hda: Fix page fault in snd_hda_codec_shutdown() Cezary Rojewski
3 siblings, 0 replies; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 13:06 UTC (permalink / raw)
To: alsa-devel, broonie, tiwai
Cc: Cezary Rojewski, kai.vehmanen, yung-chuan.liao, lgirdwood,
pierre-louis.bossart, hdegoede, ranjani.sridharan,
amadeuszx.slawinski, peter.ujfalusi
With all HDAudio drivers aligned to make use of the same constructor,
have codec freed on the device release regardless of its type.
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
sound/pci/hda/hda_codec.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 7b2e62fa82d5..44395b1b734b 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -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
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] ALSA: hda: Fix page fault in snd_hda_codec_shutdown()
2022-07-20 13:06 [PATCH 0/4] ALSA: hda: Unify codec construction Cezary Rojewski
` (2 preceding siblings ...)
2022-07-20 13:06 ` [PATCH 3/4] ALSA: hda: Always free codec on the device release Cezary Rojewski
@ 2022-07-20 13:06 ` Cezary Rojewski
3 siblings, 0 replies; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 13:06 UTC (permalink / raw)
To: alsa-devel, broonie, tiwai
Cc: Cezary Rojewski, kai.vehmanen, yung-chuan.liao, lgirdwood,
pierre-louis.bossart, hdegoede, ranjani.sridharan,
amadeuszx.slawinski, peter.ujfalusi
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.
Iinitialization 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.
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
sound/pci/hda/hda_codec.c | 41 +++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 44395b1b734b..17ff79e971cb 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -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;
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
2022-07-20 13:06 ` [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor Cezary Rojewski
@ 2022-07-20 13:13 ` Takashi Iwai
2022-07-20 15:39 ` Cezary Rojewski
2022-07-20 17:06 ` kernel test robot
1 sibling, 1 reply; 11+ messages in thread
From: Takashi Iwai @ 2022-07-20 13:13 UTC (permalink / raw)
To: Cezary Rojewski
Cc: alsa-devel, kai.vehmanen, lgirdwood, yung-chuan.liao,
pierre-louis.bossart, tiwai, hdegoede, broonie,
ranjani.sridharan, amadeuszx.slawinski, peter.ujfalusi
On Wed, 20 Jul 2022 15:06:20 +0200,
Cezary Rojewski wrote:
>
> Refactor snd_hdac_ext_bus_device_init() so that it makes use of
> snd_hda_codec_device_init() to create and initialize new codec device.
> Causes the latter to become the sole codec device constructor.
>
> Users of the refactored function are updated accordingly and now also
> take responsibility for assigning driver's private data as that task is
> no longer performed by hdac_hda_dev_probe().
Hrm, this doesn't look really right. It means you'll introduce a hard
dependency chain in a reverse order: snd-hda-ext-core ->
snd-hda-codec.
Originally, the ext bus code was written completely independent from
the legacy HD-audio implementations, and hdac-hda driver was a kind of
wrapper / bridge for the legacy codec over the ext bus. If we want
change this rule and make the legacy HD-audio codec always tied with
the ext bus, a likely better way would be to call
snd_hda_codec_device_init() in the caller's side (e.g. skl or sof),
then pass the newly created codec object to
snd_hdac_ext_bus_device_init() for further initialization.
thanks,
Takashi
>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
> include/sound/hdaudio_ext.h | 4 ++--
> sound/hda/ext/hdac_ext_bus.c | 34 +++++++++++++++------------------
> sound/soc/intel/skylake/skl.c | 24 ++++++++++-------------
> sound/soc/sof/intel/hda-codec.c | 29 ++++++++++++----------------
> 4 files changed, 39 insertions(+), 52 deletions(-)
>
> diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
> index d26234f9ee46..25c7b13db278 100644
> --- a/include/sound/hdaudio_ext.h
> +++ b/include/sound/hdaudio_ext.h
> @@ -11,8 +11,8 @@ 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);
> +struct hda_codec *
> +snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type);
> void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev);
> void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus);
>
> diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
> index 765c40a6ccba..bd3c7124aca1 100644
> --- a/sound/hda/ext/hdac_ext_bus.c
> +++ b/sound/hda/ext/hdac_ext_bus.c
> @@ -12,6 +12,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/io.h>
> +#include <sound/hda_codec.h>
> #include <sound/hdaudio_ext.h>
>
> MODULE_DESCRIPTION("HDA extended core");
> @@ -67,39 +68,34 @@ static void default_release(struct device *dev)
>
> /**
> * snd_hdac_ext_bus_device_init - initialize the HDA extended codec base device
> - * @bus: hdac bus to attach to
> + * @bus: hda bus to attach to
> * @addr: codec address
> - * @hdev: hdac device to init
> * @type: codec type (HDAC_DEV_*) to use for this device
> *
> - * Returns zero for success or a negative error code.
> + * Returns pointer to newly created codec or ERR_PTR.
> */
> -int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
> - struct hdac_device *hdev, int type)
> +struct hda_codec *
> +snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type)
> {
> - char name[15];
> + struct hda_codec *codec;
> int ret;
>
> - hdev->bus = bus;
> -
> - snprintf(name, sizeof(name), "ehdaudio%dD%d", bus->idx, addr);
> -
> - ret = snd_hdac_device_init(hdev, bus, name, addr);
> - if (ret < 0) {
> + 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 ret;
> + return codec;
> }
> - hdev->type = type;
> - hdev->dev.release = default_release;
> + codec->core.type = type;
> + codec->core.dev.release = default_release;
>
> - ret = snd_hdac_device_register(hdev);
> + ret = snd_hdac_device_register(&codec->core);
> if (ret) {
> dev_err(bus->dev, "failed to register hdac device\n");
> - snd_hdac_ext_bus_device_exit(hdev);
> - return ret;
> + snd_hdac_ext_bus_device_exit(&codec->core);
> + return ERR_PTR(ret);
> }
>
> - return 0;
> + return codec;
> }
> EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init);
>
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 7e573a887118..5637292c2aa9 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -700,9 +700,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);
> @@ -718,25 +717,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 = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC);
> + 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(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 = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC);
> + return PTR_ERR_OR_ZERO(codec);
> #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */
> }
>
> diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
> index de7c53224ac3..7c3ea4a12d63 100644
> --- a/sound/soc/sof/intel/hda-codec.c
> +++ b/sound/soc/sof/intel/hda-codec.c
> @@ -115,11 +115,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;
> @@ -142,20 +141,19 @@ 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);
> - if (ret < 0)
> - return ret;
> + codec = snd_hdac_ext_bus_device_init(&hbus->core, address, type);
> + if (IS_ERR(codec))
> + return PTR_ERR(codec);
> +
> + 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;
> @@ -181,15 +179,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 = snd_hdac_ext_bus_device_init(&hbus->core, address, HDA_DEV_ASOC);
> + ret = PTR_ERR_OR_ZERO(codec);
> #endif
>
> return ret;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec
2022-07-20 13:06 ` [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec Cezary Rojewski
@ 2022-07-20 13:56 ` Mark Brown
0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2022-07-20 13:56 UTC (permalink / raw)
To: Cezary Rojewski
Cc: alsa-devel, kai.vehmanen, lgirdwood, yung-chuan.liao,
pierre-louis.bossart, tiwai, hdegoede, ranjani.sridharan,
amadeuszx.slawinski, peter.ujfalusi
[-- Attachment #1: Type: text/plain, Size: 288 bytes --]
On Wed, Jul 20, 2022 at 03:06:19PM +0200, Cezary Rojewski wrote:
> Preliminary step in making snd_hda_codec_device_init() the only
> constructor for struct hda_codec instances. To do that, no struct may
> wrap struct hda_codec as its base type.
Acked-by: Mark Brown <broonie@kernel.org>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
2022-07-20 13:13 ` Takashi Iwai
@ 2022-07-20 15:39 ` Cezary Rojewski
2022-07-20 16:00 ` Pierre-Louis Bossart
0 siblings, 1 reply; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 15:39 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, kai.vehmanen, lgirdwood, yung-chuan.liao,
pierre-louis.bossart, tiwai, hdegoede, broonie,
ranjani.sridharan, amadeuszx.slawinski, peter.ujfalusi
On 2022-07-20 3:13 PM, Takashi Iwai wrote:
> On Wed, 20 Jul 2022 15:06:20 +0200,
> Cezary Rojewski wrote:
>>
>> Refactor snd_hdac_ext_bus_device_init() so that it makes use of
>> snd_hda_codec_device_init() to create and initialize new codec device.
>> Causes the latter to become the sole codec device constructor.
>>
>> Users of the refactored function are updated accordingly and now also
>> take responsibility for assigning driver's private data as that task is
>> no longer performed by hdac_hda_dev_probe().
>
> Hrm, this doesn't look really right. It means you'll introduce a hard
> dependency chain in a reverse order: snd-hda-ext-core ->
> snd-hda-codec.
>
> Originally, the ext bus code was written completely independent from
> the legacy HD-audio implementations, and hdac-hda driver was a kind of
> wrapper / bridge for the legacy codec over the ext bus. If we want
> change this rule and make the legacy HD-audio codec always tied with
> the ext bus, a likely better way would be to call
> snd_hda_codec_device_init() in the caller's side (e.g. skl or sof),
> then pass the newly created codec object to
> snd_hdac_ext_bus_device_init() for further initialization.
Agree. That or drop the snd_hdac_ext_bus_device_init() entirely. Will
send an update soon.
In regard to the other subject, my plan:
- separate code used by both ALSA/ASoC into sound/hda (this includes
many hda_codec functions)
- combine hda_bus and hdac_bus
- combine hda_codec and hdac_device
- drop HDA_DEV_ASOC
- drop hdac_hda/hdac_hdmi (once skylake-driver is gone; sof will be
updated accordingly)
<story does not end here>
Regards,
Czarek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
2022-07-20 15:39 ` Cezary Rojewski
@ 2022-07-20 16:00 ` Pierre-Louis Bossart
2022-07-20 16:18 ` Cezary Rojewski
0 siblings, 1 reply; 11+ messages in thread
From: Pierre-Louis Bossart @ 2022-07-20 16:00 UTC (permalink / raw)
To: Cezary Rojewski, Takashi Iwai
Cc: alsa-devel, kai.vehmanen, lgirdwood, yung-chuan.liao, tiwai,
hdegoede, broonie, ranjani.sridharan, amadeuszx.slawinski,
peter.ujfalusi
On 7/20/22 10:39, Cezary Rojewski wrote:
> On 2022-07-20 3:13 PM, Takashi Iwai wrote:
>> On Wed, 20 Jul 2022 15:06:20 +0200,
>> Cezary Rojewski wrote:
>>>
>>> Refactor snd_hdac_ext_bus_device_init() so that it makes use of
>>> snd_hda_codec_device_init() to create and initialize new codec device.
>>> Causes the latter to become the sole codec device constructor.
>>>
>>> Users of the refactored function are updated accordingly and now also
>>> take responsibility for assigning driver's private data as that task is
>>> no longer performed by hdac_hda_dev_probe().
>>
>> Hrm, this doesn't look really right. It means you'll introduce a hard
>> dependency chain in a reverse order: snd-hda-ext-core ->
>> snd-hda-codec.
>>
>> Originally, the ext bus code was written completely independent from
>> the legacy HD-audio implementations, and hdac-hda driver was a kind of
>> wrapper / bridge for the legacy codec over the ext bus. If we want
>> change this rule and make the legacy HD-audio codec always tied with
>> the ext bus, a likely better way would be to call
>> snd_hda_codec_device_init() in the caller's side (e.g. skl or sof),
>> then pass the newly created codec object to
>> snd_hdac_ext_bus_device_init() for further initialization.
>
>
> Agree. That or drop the snd_hdac_ext_bus_device_init() entirely. Will
> send an update soon.
>
> In regard to the other subject, my plan:
> - separate code used by both ALSA/ASoC into sound/hda (this includes
> many hda_codec functions)
> - combine hda_bus and hdac_bus
> - combine hda_codec and hdac_device
> - drop HDA_DEV_ASOC
> - drop hdac_hda/hdac_hdmi (once skylake-driver is gone; sof will be
> updated accordingly)
the skylake driver cannot be removed until you have evidence that users
have switched, and SOF has other priorities that will likely conflict
with that goal. I don't even know what this 'drop hdac_hda' idea means
in detail, we need to keep an ASoC-based codec and the split between
platform/codec/machine. We are not going to move the HDaudio codec
management logic inside the SOF driver if that was the intent. The SOF
driver will focus on host/controller/DSP handling.
> <story does not end here>
I strongly recommend that we add no dependencies between hdac_ext and
hda_codec. To be clearer, we don't want to limit the hdac_ext bus and
stream management to platforms with HDaudio codecs.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
2022-07-20 16:00 ` Pierre-Louis Bossart
@ 2022-07-20 16:18 ` Cezary Rojewski
0 siblings, 0 replies; 11+ messages in thread
From: Cezary Rojewski @ 2022-07-20 16:18 UTC (permalink / raw)
To: Pierre-Louis Bossart, Takashi Iwai
Cc: alsa-devel, kai.vehmanen, lgirdwood, yung-chuan.liao, tiwai,
hdegoede, broonie, ranjani.sridharan, amadeuszx.slawinski,
peter.ujfalusi
On 2022-07-20 6:00 PM, Pierre-Louis Bossart wrote:
> On 7/20/22 10:39, Cezary Rojewski wrote:
>> On 2022-07-20 3:13 PM, Takashi Iwai wrote:
>>> On Wed, 20 Jul 2022 15:06:20 +0200,
>>> Cezary Rojewski wrote:
>>>>
>>>> Refactor snd_hdac_ext_bus_device_init() so that it makes use of
>>>> snd_hda_codec_device_init() to create and initialize new codec device.
>>>> Causes the latter to become the sole codec device constructor.
>>>>
>>>> Users of the refactored function are updated accordingly and now also
>>>> take responsibility for assigning driver's private data as that task is
>>>> no longer performed by hdac_hda_dev_probe().
>>>
>>> Hrm, this doesn't look really right. It means you'll introduce a hard
>>> dependency chain in a reverse order: snd-hda-ext-core ->
>>> snd-hda-codec.
>>>
>>> Originally, the ext bus code was written completely independent from
>>> the legacy HD-audio implementations, and hdac-hda driver was a kind of
>>> wrapper / bridge for the legacy codec over the ext bus. If we want
>>> change this rule and make the legacy HD-audio codec always tied with
>>> the ext bus, a likely better way would be to call
>>> snd_hda_codec_device_init() in the caller's side (e.g. skl or sof),
>>> then pass the newly created codec object to
>>> snd_hdac_ext_bus_device_init() for further initialization.
>>
>>
>> Agree. That or drop the snd_hdac_ext_bus_device_init() entirely. Will
>> send an update soon.
>>
>> In regard to the other subject, my plan:
>> - separate code used by both ALSA/ASoC into sound/hda (this includes
>> many hda_codec functions)
>> - combine hda_bus and hdac_bus
>> - combine hda_codec and hdac_device
>> - drop HDA_DEV_ASOC
>> - drop hdac_hda/hdac_hdmi (once skylake-driver is gone; sof will be
>> updated accordingly)
>
> the skylake driver cannot be removed until you have evidence that users
> have switched, and SOF has other priorities that will likely conflict
> with that goal. I don't even know what this 'drop hdac_hda' idea means
> in detail, we need to keep an ASoC-based codec and the split between
> platform/codec/machine. We are not going to move the HDaudio codec
> management logic inside the SOF driver if that was the intent. The SOF
> driver will focus on host/controller/DSP handling.
The evidence will be there : )
Also, there is nothing stopping us from adjusting skylake-driver however
we see fit along the road. sound/soc/codecs/hda.c is a clear winner here.
And SOF is just breaking compatibility in several places due to IPC4 and
stuff, no? There is no reason not to update sof along the road too - so
it makes use of the aforementioned codec driver.
>> <story does not end here>
>
> I strongly recommend that we add no dependencies between hdac_ext and
> hda_codec. To be clearer, we don't want to limit the hdac_ext bus and
> stream management to platforms with HDaudio codecs.
Yeah, we all agree here - snd_hdac_ext_bus_device_init() will be
removed. snd_hda_codec_device_init() + registration will be used
directly instead.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor
2022-07-20 13:06 ` [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor Cezary Rojewski
2022-07-20 13:13 ` Takashi Iwai
@ 2022-07-20 17:06 ` kernel test robot
1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2022-07-20 17:06 UTC (permalink / raw)
To: Cezary Rojewski; +Cc: llvm, kbuild-all
Hi Cezary,
I love your patch! Perhaps something to improve:
[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next linus/master v5.19-rc7 next-20220720]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cezary-Rojewski/ALSA-hda-Unify-codec-construction/20220720-210158
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: i386-randconfig-a006-20220718 (https://download.01.org/0day-ci/archive/20220721/202207210002.4Jtc7lBC-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project dd5635541cd7bbd62cd59b6694dfb759b6e9a0d8)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1f9a732d481feda1eeedda7bd843f2de3e624199
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Cezary-Rojewski/ALSA-hda-Unify-codec-construction/20220720-210158
git checkout 1f9a732d481feda1eeedda7bd843f2de3e624199
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash sound/soc/sof/intel/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> sound/soc/sof/intel/hda-codec.c:170:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (type == HDA_DEV_LEGACY) {
^~~~~~~~~~~~~~~~~~~~~~
sound/soc/sof/intel/hda-codec.c:181:6: note: uninitialized use occurs here
if (ret < 0) {
^~~
sound/soc/sof/intel/hda-codec.c:170:2: note: remove the 'if' if its condition is always true
if (type == HDA_DEV_LEGACY) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/soc/sof/intel/hda-codec.c:125:9: note: initialize the variable 'ret' to silence this warning
int ret, retry = 0;
^
= 0
1 warning generated.
vim +170 sound/soc/sof/intel/hda-codec.c
89d73ccab20a68 Kai Vehmanen 2020-05-29 111
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 112 /* probe individual codec */
80acdd4f8ff763 Ranjani Sridharan 2019-12-04 113 static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
80acdd4f8ff763 Ranjani Sridharan 2019-12-04 114 bool hda_codec_use_common_hdmi)
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 115 {
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 116 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 117 struct hdac_hda_priv *hda_priv;
163cd1059a85d2 Kai Vehmanen 2020-09-21 118 int type = HDA_DEV_LEGACY;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 119 #endif
fd15f2f5e27214 Rander Wang 2019-07-22 120 struct hda_bus *hbus = sof_to_hbus(sdev);
1f9a732d481fed Cezary Rojewski 2022-07-20 121 struct hda_codec *codec;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 122 u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) |
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 123 (AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 124 u32 resp = -1;
046aede2f84767 Hui Wang 2021-11-30 125 int ret, retry = 0;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 126
046aede2f84767 Hui Wang 2021-11-30 127 do {
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 128 mutex_lock(&hbus->core.cmd_mutex);
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 129 snd_hdac_bus_send_cmd(&hbus->core, hda_cmd);
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 130 snd_hdac_bus_get_response(&hbus->core, address, &resp);
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 131 mutex_unlock(&hbus->core.cmd_mutex);
046aede2f84767 Hui Wang 2021-11-30 132 } while (resp == -1 && retry++ < CODEC_PROBE_RETRIES);
046aede2f84767 Hui Wang 2021-11-30 133
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 134 if (resp == -1)
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 135 return -EIO;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 136 dev_dbg(sdev->dev, "HDA codec #%d probed OK: response: %x\n",
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 137 address, resp);
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 138
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 139 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
ef9bec27485fef Ranjani Sridharan 2019-06-26 140 hda_priv = devm_kzalloc(sdev->dev, sizeof(*hda_priv), GFP_KERNEL);
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 141 if (!hda_priv)
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 142 return -ENOMEM;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 143
163cd1059a85d2 Kai Vehmanen 2020-09-21 144 /* only probe ASoC codec drivers for HDAC-HDMI */
163cd1059a85d2 Kai Vehmanen 2020-09-21 145 if (!hda_codec_use_common_hdmi && (resp & 0xFFFF0000) == IDISP_VID_INTEL)
163cd1059a85d2 Kai Vehmanen 2020-09-21 146 type = HDA_DEV_ASOC;
163cd1059a85d2 Kai Vehmanen 2020-09-21 147
1f9a732d481fed Cezary Rojewski 2022-07-20 148 codec = snd_hdac_ext_bus_device_init(&hbus->core, address, type);
1f9a732d481fed Cezary Rojewski 2022-07-20 149 if (IS_ERR(codec))
1f9a732d481fed Cezary Rojewski 2022-07-20 150 return PTR_ERR(codec);
1f9a732d481fed Cezary Rojewski 2022-07-20 151
1f9a732d481fed Cezary Rojewski 2022-07-20 152 hda_priv->codec = codec;
1f9a732d481fed Cezary Rojewski 2022-07-20 153 dev_set_drvdata(&codec->core.dev, hda_priv);
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 154
71cc8abb6ec705 Kai Vehmanen 2020-02-20 155 if ((resp & 0xFFFF0000) == IDISP_VID_INTEL) {
1f9a732d481fed Cezary Rojewski 2022-07-20 156 if (!hbus->core.audio_component) {
71cc8abb6ec705 Kai Vehmanen 2020-02-20 157 dev_dbg(sdev->dev,
71cc8abb6ec705 Kai Vehmanen 2020-02-20 158 "iDisp hw present but no driver\n");
9c25af250214e4 Kai Vehmanen 2021-01-13 159 ret = -ENOENT;
9c25af250214e4 Kai Vehmanen 2021-01-13 160 goto out;
71cc8abb6ec705 Kai Vehmanen 2020-02-20 161 }
139c7febad1afa Kai Vehmanen 2019-10-29 162 hda_priv->need_display_power = true;
71cc8abb6ec705 Kai Vehmanen 2020-02-20 163 }
139c7febad1afa Kai Vehmanen 2019-10-29 164
89d73ccab20a68 Kai Vehmanen 2020-05-29 165 if (is_generic_config(hbus))
89d73ccab20a68 Kai Vehmanen 2020-05-29 166 codec->probe_id = HDA_CODEC_ID_GENERIC;
89d73ccab20a68 Kai Vehmanen 2020-05-29 167 else
89d73ccab20a68 Kai Vehmanen 2020-05-29 168 codec->probe_id = 0;
89d73ccab20a68 Kai Vehmanen 2020-05-29 169
163cd1059a85d2 Kai Vehmanen 2020-09-21 @170 if (type == HDA_DEV_LEGACY) {
89d73ccab20a68 Kai Vehmanen 2020-05-29 171 ret = hda_codec_load_module(codec);
2c63bea714780f Kai Vehmanen 2020-01-10 172 /*
2c63bea714780f Kai Vehmanen 2020-01-10 173 * handle ret==0 (no driver bound) as an error, but pass
2c63bea714780f Kai Vehmanen 2020-01-10 174 * other return codes without modification
2c63bea714780f Kai Vehmanen 2020-01-10 175 */
2c63bea714780f Kai Vehmanen 2020-01-10 176 if (ret == 0)
9c25af250214e4 Kai Vehmanen 2021-01-13 177 ret = -ENOENT;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 178 }
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 179
9c25af250214e4 Kai Vehmanen 2021-01-13 180 out:
9c25af250214e4 Kai Vehmanen 2021-01-13 181 if (ret < 0) {
1f9a732d481fed Cezary Rojewski 2022-07-20 182 snd_hdac_device_unregister(&codec->core);
1f9a732d481fed Cezary Rojewski 2022-07-20 183 put_device(&codec->core.dev);
9c25af250214e4 Kai Vehmanen 2021-01-13 184 }
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 185 #else
1f9a732d481fed Cezary Rojewski 2022-07-20 186 codec = snd_hdac_ext_bus_device_init(&hbus->core, address, HDA_DEV_ASOC);
1f9a732d481fed Cezary Rojewski 2022-07-20 187 ret = PTR_ERR_OR_ZERO(codec);
9c25af250214e4 Kai Vehmanen 2021-01-13 188 #endif
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 189
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 190 return ret;
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 191 }
5507b8103e2653 Pierre-Louis Bossart 2019-04-12 192
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-20 17:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 13:06 [PATCH 0/4] ALSA: hda: Unify codec construction Cezary Rojewski
2022-07-20 13:06 ` [PATCH 1/4] ASoC: hdac_hda: Disconnect struct hdac_hda_priv and hda_codec Cezary Rojewski
2022-07-20 13:56 ` Mark Brown
2022-07-20 13:06 ` [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor Cezary Rojewski
2022-07-20 13:13 ` Takashi Iwai
2022-07-20 15:39 ` Cezary Rojewski
2022-07-20 16:00 ` Pierre-Louis Bossart
2022-07-20 16:18 ` Cezary Rojewski
2022-07-20 17:06 ` kernel test robot
2022-07-20 13:06 ` [PATCH 3/4] ALSA: hda: Always free codec on the device release Cezary Rojewski
2022-07-20 13:06 ` [PATCH 4/4] ALSA: hda: Fix page fault in snd_hda_codec_shutdown() Cezary Rojewski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.