[PATCH v2 0/6] ALSA: hda: Unify codec construction

[PATCH v2 0/6] ALSA: hda: Unify codec construction

[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/20220706120230.427296-7-cezary.rojewski@intel.com/
[2]: https://lore.kernel.org/alsa-devel/3c40df55-3aee-1e08-493b-7b30cd84dc00@linux.intel.com/
[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(-)

-- 
2.25.1

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

[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 <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)
-- 
2.25.1

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

[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 <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
-- 
2.25.1

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

[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 <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
  *
-- 
2.25.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/6] ASoC: Intel: Drop hdac_ext usage for codec device creation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

On Mon, Aug 15, 2022 at 07:42:24PM +0200, Cezary Rojewski wrote:
> To make snd_hda_codec_device_init() the only constructor for struct
> hda_codec instances remaining tasks are:

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/6] ALSA: hda: Unify codec construction

[Cezary Rojewski]

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

Re: [PATCH v2 0/6] ALSA: hda: Unify codec construction

[Takashi Iwai]

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

Re: [PATCH v2 0/6] ALSA: hda: Unify codec construction

[Cezary Rojewski]

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?

Re: [PATCH v2 0/6] ALSA: hda: Unify codec construction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/6] ALSA: hda: Unify codec construction

[Takashi Iwai]

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

Re: [PATCH v2 0/6] ALSA: hda: Unify codec construction

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.