[PATCH v2 0/6] 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/20220706120230.427296-7-cezary.rojewski@i... [2]: https://lore.kernel.org/alsa-devel/3c40df55-3aee-1e08-493b-7b30cd84dc00@linu... [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(-)
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 kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/sof/intel/hda-codec.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 2f3f4a733d9e..4c128ba02340 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -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)
On Mon, Aug 15, 2022 at 07:42:23PM +0200, Cezary Rojewski wrote:
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 kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
Acked-by: Mark Brown broonie@kernel.org
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 kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com 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 384426d7e9dd..aa7a362be290 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
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 kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- 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(-)
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/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index d26234f9ee46..88ebb64fd8a5 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -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) \ diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index 765c40a6ccba..6004ea1c373e 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -60,59 +60,6 @@ void snd_hdac_ext_bus_exit(struct hdac_bus *bus) } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_exit);
-static void default_release(struct device *dev) -{ - snd_hdac_ext_bus_device_exit(dev_to_hdac_dev(dev)); -} - -/** - * snd_hdac_ext_bus_device_init - initialize the HDA extended codec base device - * @bus: hdac 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. - */ -int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, - struct hdac_device *hdev, int type) -{ - char name[15]; - 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) { - dev_err(bus->dev, "device init failed for hdac device\n"); - return ret; - } - hdev->type = type; - hdev->dev.release = default_release; - - ret = snd_hdac_device_register(hdev); - if (ret) { - dev_err(bus->dev, "failed to register hdac device\n"); - snd_hdac_ext_bus_device_exit(hdev); - return ret; - } - - return 0; -} -EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init); - -/** - * snd_hdac_ext_bus_device_exit - clean up a HD-audio extended codec base device - * @hdev: hdac device to clean up - */ -void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev) -{ - snd_hdac_device_exit(hdev); -} -EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_exit); - /** * snd_hdac_ext_bus_device_remove - remove HD-audio extended codec base devices *
On 2022-08-15 7:42 PM, Cezary Rojewski wrote:
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().
...
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()
I've received a message yesterday that patches: 1/6, 3/6 and 6/6 were not delivered to recipient: alsa-devel@alsa-projet.org. None else is listed there. lore.kernel.org also shows only four letters (3 + cover-letter) rather than seven. Given Mark's Ack on patch 3/6 it makes me believe that indeed just the alsa-devel did not received some of the messages.
What should I do in such situation? Is the server admin about to "recover them from the void" or should I resend the entire series as: "[RESEND PATCH v2]" as see how it goes this time?
Regards, Czarek
On Tue, 16 Aug 2022 10:15:44 +0200, Cezary Rojewski wrote:
On 2022-08-15 7:42 PM, Cezary Rojewski wrote:
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().
...
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()
I've received a message yesterday that patches: 1/6, 3/6 and 6/6 were not delivered to recipient: alsa-devel@alsa-projet.org. None else is listed there. lore.kernel.org also shows only four letters (3 + cover-letter) rather than seven. Given Mark's Ack on patch 3/6 it makes me believe that indeed just the alsa-devel did not received some of the messages.
What should I do in such situation? Is the server admin about to "recover them from the void" or should I resend the entire series as: "[RESEND PATCH v2]" as see how it goes this time?
I guess the full resubmission would be better.
Takashi
On 2022-08-16 11:59 AM, Takashi Iwai wrote:
On Tue, 16 Aug 2022 10:15:44 +0200, Cezary Rojewski wrote:
...
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()
I've received a message yesterday that patches: 1/6, 3/6 and 6/6 were not delivered to recipient: alsa-devel@alsa-projet.org. None else is listed there. lore.kernel.org also shows only four letters (3 + cover-letter) rather than seven. Given Mark's Ack on patch 3/6 it makes me believe that indeed just the alsa-devel did not received some of the messages.
What should I do in such situation? Is the server admin about to
Meant to say 'able' instead of 'about'.
"recover them from the void" or should I resend the entire series as: "[RESEND PATCH v2]" as see how it goes this time?
I guess the full resubmission would be better.
Should I apply Mark's ACKs for patches 2/6 and 3/6 in the about-to-be-resent series?
On Tue, Aug 16, 2022 at 12:08:12PM +0200, Cezary Rojewski wrote:
On 2022-08-16 11:59 AM, Takashi Iwai wrote:
"recover them from the void" or should I resend the entire series as: "[RESEND PATCH v2]" as see how it goes this time?
I guess the full resubmission would be better.
Note that if you do whatever upset the server again it'll happen again. If it was a blacklisting it does look like there's something going on there which might be random, I've had some issues with the kernel.org servers there that don't seem attached to the the actual sending server as much as you'd expect.
Should I apply Mark's ACKs for patches 2/6 and 3/6 in the about-to-be-resent series?
Yes, if there's tags always carry them formwards unless something changed to invalidate them.
On Tue, 16 Aug 2022 12:30:58 +0200, Mark Brown wrote:
On Tue, Aug 16, 2022 at 12:08:12PM +0200, Cezary Rojewski wrote:
On 2022-08-16 11:59 AM, Takashi Iwai wrote:
"recover them from the void" or should I resend the entire series as: "[RESEND PATCH v2]" as see how it goes this time?
I guess the full resubmission would be better.
Note that if you do whatever upset the server again it'll happen again.
If this happens, we need to investigate. alsa-devel ML is maintained outside vger by Jaroslav personally. Let's see...
Takashi
On 16. 08. 22 12:33, Takashi Iwai wrote:
On Tue, 16 Aug 2022 12:30:58 +0200, Mark Brown wrote:
On Tue, Aug 16, 2022 at 12:08:12PM +0200, Cezary Rojewski wrote:
On 2022-08-16 11:59 AM, Takashi Iwai wrote:
"recover them from the void" or should I resend the entire series as: "[RESEND PATCH v2]" as see how it goes this time?
I guess the full resubmission would be better.
Note that if you do whatever upset the server again it'll happen again.
If this happens, we need to investigate. alsa-devel ML is maintained outside vger by Jaroslav personally. Let's see...
It seems that it was a temporary issue with the DNS resolver. The resend is fine.
Jaroslav
participants (4)
-
Cezary Rojewski -
Jaroslav Kysela -
Mark Brown -
Takashi Iwai