Re: [RFC] Reorganizing HD-audio driver code?

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 2025-07-03 15:38, Takashi Iwai wrote:
> On Thu, 03 Jul 2025 15:29:56 +0200,
> Amadeusz Sławiński wrote:
>>
>>
>>
>> On 2025-07-02 16:53, Takashi Iwai wrote:
>>> On Sun, 29 Jun 2025 11:22:25 +0200,
>>> Takashi Iwai wrote:
>>>>
>>>> On Fri, 27 Jun 2025 15:22:20 +0200,
>>>> Richard Fitzgerald wrote:
>>>>>
>>>>> On 27/06/2025 1:04 pm, Takashi Iwai wrote:
>>>>>> Hi,
>>>>>>
>>>>>> HD-audio driver is known to be quite messy in both file structures and
>>>>>> its design, but until now I haven't touched its files paths so much
>>>>>> because I set a higher priority for the easiness of backport to stable
>>>>>> kernels.  But, you can't leave garbages forever, it's been already
>>>>>> high time for a large clean up.
>>>>>>
>>>>>> So I tried a quick code reorganization, and put the result in
>>>>>> test/hda-reorg branch of sound.git tree.
>>>>>>      https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=test/hda-reorg
>>>>>>
>>>>>> The basic idea is to move the code from sound/pci/hda/* into different
>>>>>> subdirectories in sound/hda/ per functionality, as most of the stuff
>>>>>> are independent from PCI, but rather HD-audio bus specific.
>>>>>>
>>>>>
>>>>> This all seems reasonable to me. I always thought it strange that there
>>>>> is a sound/hda directory but most of the HDA support isn't in that
>>>>> directory.
>>>>
>>>> Thanks for your review!
>>>>
>>>> It's a long story: at the time of ASoC Intel HD-audio support, we
>>>> thought to implement more ASoC-friendly way of HD-audio controller and
>>>> codec drivers, e.g. adapting DAPM and others.  So we began with the
>>>> factoring out the HD-audio basic core stuff to the common directory
>>>> sound/hda/*, while keeping the rest legacy stuff almost as is.  But
>>>> ASoC implementation didn't fly in the end from various reasons, and
>>>> the legacy HD-audio stuff was good enough for the actual use cases;
>>>> it's too bit to fail, after all.
>>>>
>>>>>> The Realtek codec is split further to smaller pieces (which was really
>>>>>> huge).
>>>>>
>>>>> Yes, that file was very confusing having support and quirks for so many
>>>>> Realtek parts all in one file.
>>>>>
>>>>>> The Cirrus and TI sub-codec drivers are moved to codecs/side-codecs
>>>>>> subdirectory:
>>>>>
>>>>> That's ok
>>>>>
>>>>>> They can be put to each own directory and drop the file name prefix,
>>>>>> if we want, too.  Let me know if Cirrus and TI people would like to
>>>>>> split to more subdirectories.
>>>>>
>>>>> I don't mind either way.
>>>>> The hda_component* files are common to the amps and the realtek driver
>>>>> so I wonder whether they belong in helpers. They are only utility
>>>>> wrappers around the kernel component-binding APIs.
>>>>
>>>> Right.  So far, just because it's basically only binding with
>>>> side-codecs, I put into side-codecs subdirectory.
>>>>
>>>>>> *HOWEVER* the biggest question is: whether it's worth?
>>>>>>
>>>>>> Essentially, this makes almost impossible to make a patch for stable
>>>>>> trees from the original commit as is; one has to translate the file
>>>>>> paths and adjust manually in each patch.
>>>>>
>>>>> Depends which file you are patching and how much it has changed.
>>>>> Git can figure out file renames (and changing directory is a rename).
>>>>
>>>> I'm afraid that the Realtek codec changes might be hard to track
>>>> automatically.  Maybe the Cirrus side-codecs stuff would work.
>>>>
>>>>>> Also, of course, if anyone is working on HD-audio stuff right now, the
>>>>>> work had to be adjusted to the new file path.  It'd be one-off action,
>>>>>> though.
>>>>>
>>>>> Speaking only for Cirrus, I don't think this makes much extra effort
>>>>> for us. Our amp drivers have only changed location, so that should be
>>>>> trivial. The other file we change is patch_realtek.c but that typically
>>>>> is only 1-line quirk entries.
>>>>
>>>> OK, thanks for confirmation!
>>>
>>> ... and now I worked further on this, resulting in larger set of
>>> changes.  It's not only the file location changes but also the
>>> HD-audio codec driver binding changes.  So I put more people to Cc for
>>> catch their cautions.
>>>
>>> To recap:
>>>
>>> - The all HD-audio driver code are moved under sound/hda.
>>>
>>>     % ls sound/hda
>>>     codecs/  common/  controllers/  core/  Kconfig  Makefile
>>>
>>>     * The former hda core code is found in sound/hda/core.
>>>     * The former snd-hda-codec code is found in sound/hda/common.
>>>     * The former snd-hda-intel, tegra and acpi are put in
>>>       sound/hda/controllers.
>>>     * The former patch_* and co are put to sound/hda/codecs.
>>>     * Realtek codec driver is split to several modules as
>>>       sound/hda/codecs/realtek/alc*.
>>>     * Cirrus codec driver is split to cs420x and cs421x, put under
>>>       sound/hda/codecs/cirrus together with cs8409.
>>>     * HDMI codec driver is split to several modules under
>>>       sound/hda/codecs/hdmi
>>>     * Cirrus and TI sub-codecs are put under
>>>       sound/hda/codecs/side-codecs
>>>
>>> - The HD-audio codec driver binding is changed from the embedded
>>>     hda_codec.patch_ops to hda_codec_driver.ops.
>>>     (As of now, hda_codec_driver.ops is a pointer, but it can be
>>>      embedded later, too.)
>>>
>>>     This change required some code to be modified without the dynamic
>>>     override of callbacks.
>>>
>>> - In future, we may convert the runtime PM handling to use the
>>>     standard pm_ops, too.  This was raised some time ago during the
>>>     discussion with Realtek devs.
>>>
>>> The current patches are found in test/hda-reorg branch of sound git
>>> tree:
>>>     https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=test/hda-reorg
>>>
>>> It's based on master branch for taking all changes of for-linus and
>>> for-next, hence the branch may be still frequently rebased.
>>>
>>> So, please let me know if you see any issues or suggestions for this
>>> conversion.  I have no concrete plan for merging, but if everything
>>> looks fine, it can be even on the next 6.17, too.
>>>
>>
>> I've built it and put it on one of our machines and hit KASAN during
>> HDMI initialization, when I go back to master branch it works, so it
>> is something on test/hda-reorg.
> 
> Yes, there was bug in my branch that I fixed in this morning.
> Could you try the latest test/hda-reorg branch, commit
> 6491d5b2e256a678b3a138500bf601c916d3913e
> ?
> 

Still broken :(

>> I've checked few commits and overall I suspect that commit
>> 16e74bb52bba0aa67d3447a731ee5eb3fbb2440a ("ALSA: hda/hdmi: Rewrite to
>> new probe method") broke it somehow.
>> Seems like codec->spec is NULL while intel_haswell_enable_all_pins()
>> is called, so later spec->vendor_nid ends up in NULL pointer
>> dereference.
> 
> Right, there was a place that needed the NULL check of the bound
> driver.
> 
>> As a side note while trying to narrow it, while checking out
>> 0d20e421cf14ab73246f4476ad4d25aadbf2b740 ("ALSA: hda/ca0132: Rewrite
>> to new probe method")
>> results in following during build:
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> patch_generic_hdmi from namespace SND_HDA_CODEC_HDMI, but does not
>> import it.
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> snd_hda_hdmi_acomp_master_bind from namespace SND_HDA_CODEC_HDMI, but
>> does not import it.
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> snd_hda_hdmi_acomp_master_unbind from namespace SND_HDA_CODEC_HDMI,
>> but does not import it.
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> snd_hda_hdmi_acomp_pin_eld_notify from namespace SND_HDA_CODEC_HDMI,
>> but does not import it.
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> snd_hda_hdmi_acomp_init from namespace SND_HDA_CODEC_HDMI, but does
>> not import it.
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> snd_hda_hdmi_setup_stream from namespace SND_HDA_CODEC_HDMI, but does
>> not import it.
>> ERROR: modpost: module snd-hda-codec-atihdmi uses symbol
>> snd_hda_hdmi_generic_init from namespace SND_HDA_CODEC_HDMI, but does
>> not import it.
> 
> Right, this one was also fixed in my latest branch.

That one is fixed.