[RFC] Reorganizing HD-audio driver code?

[RFC] Reorganizing HD-audio driver code?

[Takashi Iwai]

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.

After the changes, the HD-audio root directory looks like:

  % ls sound/hda 
  codecs/  common/  controllers/  core/  Kconfig  Makefile

The current HD-audio core code is moved to sound/hda/core/*.
The common snd-hda-codec driver code is moved into
sound/hda/common/*.
The controller code (snd-hda-intel, snd-hda-tegra, snd-hda-acpi) are
moved into sound/hda/controllers/*.
And the codecs are moved to sound/hda/codecs/*.

The core code is almost same as now but I dropped the ugly hdac_
prefix from the filenames:

  % ls sound/hda/core
  array.c       device.c        i915.c              Kconfig   stream.c
  bus.c         ext/            intel-dsp-config.c  local.h   sysfs.c
  component.c   hda_bus_type.c  intel-nhlt.c        Makefile  trace.c
  controller.c  hdmi_chmap.c    intel-sdw-acpi.c    regmap.c  trace.h

  % ls sound/hda/core/ext
  bus.c  controller.c  Makefile  stream.c

The code of snd-hda-codec library module is found in common directory:

  % ls sound/hda/common
  auto_parser.c  controller.c        hda_controller.h  jack.c    sysfs.c
  beep.c         controller_trace.h  hda_jack.h        Kconfig
  bind.c         hda_auto_parser.h   hda_local.h       Makefile
  codec.c        hda_beep.h          hwdep.c           proc.c

Again, the hda_ prefix is dropped from most of files.  The headers
still contain hda_ prefix because otherwise it can be confusing at
including from others.

The controller driver code looks like:

  % ls sound/hda/controllers
  acpi.c  intel_trace.h  intel.c  intel.h  Kconfig  Makefile  tegra.c

And the codec driver code looks like:

  % ls sound/hda/codecs
  analog.c       cmedia.c         eld.c      Kconfig       side-codecs/
  ca0110.c       conexant.c       generic.c  Makefile      sigmatel.c
  ca0132.c       cs8409.c         generic.h  realtek/      via.c
  ca0132_regs.h  cs8409.h         hdmi.c     senarytech.c
  cirrus.c       cs8409-tables.c  helpers/   si3054.c

The Realtek codec is split further to smaller pieces (which was really
huge).

  % ls sound/hda/codecs/realtek
  alc260.c  alc268.c  alc662.c  alc861.c    alc880.c  Kconfig   realtek.c
  alc262.c  alc269.c  alc680.c  alc861vd.c  alc882.c  Makefile  realtek.h

Other drivers like Sigmatel/IDT driver might be worth to split, too.

The Cirrus and TI sub-codec drivers are moved to codecs/side-codecs
subdirectory:

  % ls sound/hda/codecs/side-codecs
  cirrus_scodec.c         cs35l41_hda_property.h  hda_component.h
  cirrus_scodec.h         cs35l41_hda_spi.c       Kconfig
  cirrus_scodec_test.c    cs35l56_hda.c           Makefile
  cs35l41_hda.c           cs35l56_hda.h           tas2781_hda.c
  cs35l41_hda.h           cs35l56_hda_i2c.c       tas2781_hda.h
  cs35l41_hda_i2c.c       cs35l56_hda_spi.c       tas2781_hda_i2c.c
  cs35l41_hda_property.c  hda_component.c         tas2781_hda_spi.c

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.

Finally, the helper code that is included from the codec driver is put
under codecs/helpers subdirectory (with drop of superfluous suffix):

  % ls sound/hda/codecs/helpers
  hp_x360.c  ideapad_hotkey_led.c  ideapad_s740.c  thinkpad.c

So far, so good; all looks better organized.
I planned for further driver code modernization, and this code
reorganization makes it easier, too.

*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.

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.

Any thoughts / opinions?


thanks,

Takashi

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

[Richard Fitzgerald]

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.

> 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.

> *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).

> 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.

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

[Takashi Iwai]

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!


Takashi

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

[Takashi Iwai]

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.


thanks,

Takashi

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

[Amadeusz Sławiński]

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.

[   27.659732] 
==================================================================
[   27.663843] BUG: KASAN: null-ptr-deref in 
intel_haswell_enable_all_pins+0x3a/0xc0 [snd_hda_codec_intelhdmi]
[   27.668036] Read of size 2 at addr 00000000000005c8 by task 
systemd-udevd/482

[   27.676096] CPU: 6 UID: 0 PID: 482 Comm: systemd-udevd Tainted: G S 
                6.16.0-rc3+ #72 PREEMPT(voluntary)
[   27.676119] Tainted: [S]=CPU_OUT_OF_SPEC
[   27.676124] Hardware name: Intel Corporation Kabylake Client 
platform/AmberLake Y 42 LPDDR3 RVP3, BIOS 
KBLSE2R1.R00.X151.P00.1902180320 02/18/2019
[   27.676134] Call Trace:
[   27.676141]  <TASK>
[   27.676148]  dump_stack_lvl+0x6c/0x90
[   27.676169]  print_report+0x3e4/0x630
[   27.676187]  ? intel_haswell_enable_all_pins+0x3a/0xc0 
[snd_hda_codec_intelhdmi]
[   27.676209]  ? kasan_addr_to_slab+0xd/0xa0
[   27.676222]  ? intel_haswell_enable_all_pins+0x3a/0xc0 
[snd_hda_codec_intelhdmi]
[   27.676244]  kasan_report+0xdb/0x110
[   27.676259]  ? intel_haswell_enable_all_pins+0x3a/0xc0 
[snd_hda_codec_intelhdmi]
[   27.676284]  __asan_load2+0x79/0xa0
[   27.676298]  intel_haswell_enable_all_pins+0x3a/0xc0 
[snd_hda_codec_intelhdmi]
[   27.676324]  haswell_set_power_state+0x50/0x60 [snd_hda_codec_intelhdmi]
[   27.676347]  hda_set_power_state+0xb8/0x190 [snd_hda_codec]
[   27.676498]  snd_hda_codec_device_new+0x18b/0x330 [snd_hda_codec]
[   27.676643]  ? mutex_unlock+0x7f/0xd0
[   27.676659]  ? __pfx_snd_hda_codec_device_new+0x10/0x10 [snd_hda_codec]
[   27.676809]  ? snd_hdac_ext_bus_link_get+0x4f/0x160 [snd_hda_ext_core]
[   27.676849]  hda_codec_probe+0x14e/0x980 [snd_soc_hda_codec]
[   27.676875]  snd_soc_component_probe+0x3d/0x80 [snd_soc_core]
[   27.677139]  soc_probe_component+0x32b/0x540 [snd_soc_core]
[   27.677386]  snd_soc_bind_card+0x8ed/0x1660 [snd_soc_core]
[   27.677633]  ? devm_snd_soc_bind_card+0x38/0xa0 [snd_soc_core]
[   27.677880]  ? __devres_alloc_node+0x87/0xb0
[   27.677901]  devm_snd_soc_bind_card+0x48/0xa0 [snd_soc_core]
[   27.678163]  snd_soc_register_card+0x24e/0x2d0 [snd_soc_core]
[   27.678411]  devm_snd_soc_register_deferrable_card+0x2d/0x40 
[snd_soc_core]
[   27.678667]  avs_hdaudio_probe+0x2fb/0x370 [snd_soc_avs_hdaudio]
[   27.678694]  platform_probe+0x6b/0x100
[   27.678711]  really_probe+0x15f/0x500
[   27.678729]  __driver_probe_device+0xda/0x1f0
[   27.678746]  driver_probe_device+0x4f/0x120
[   27.678763]  __driver_attach+0x14a/0x2b0
[   27.678779]  ? __pfx___driver_attach+0x10/0x10
[   27.678795]  bus_for_each_dev+0xee/0x160
[   27.678809]  ? __pfx_bus_for_each_dev+0x10/0x10
[   27.678824]  ? preempt_count_sub+0x18/0xc0
[   27.678843]  driver_attach+0x2b/0x40
[   27.678858]  bus_add_driver+0x1a3/0x330
[   27.678876]  driver_register+0xa3/0x1d0
[   27.678893]  __platform_driver_register+0x39/0x50
[   27.678908]  ? __pfx_avs_hdaudio_driver_init+0x10/0x10 
[snd_soc_avs_hdaudio]
[   27.678928]  avs_hdaudio_driver_init+0x1c/0xff0 [snd_soc_avs_hdaudio]
[   27.678948]  do_one_initcall+0x98/0x370
[   27.678961]  ? kasan_save_track+0x14/0x40
[   27.678974]  ? __pfx_do_one_initcall+0x10/0x10
[   27.678988]  ? __kasan_kmalloc+0xb3/0xc0
[   27.679001]  ? kasan_poison+0x3a/0x50
[   27.679012]  ? kasan_unpoison+0x28/0x60
[   27.679024]  ? kasan_poison+0x3a/0x50
[   27.679035]  ? __asan_register_globals+0x5e/0x80
[   27.679055]  do_init_module+0x1a5/0x4c0
[   27.679072]  ? __pfx_do_init_module+0x10/0x10
[   27.679092]  load_module+0x39cc/0x3ca0
[   27.679162]  ? __pfx_load_module+0x10/0x10
[   27.679187]  ? __pfx_ima_post_read_file+0x10/0x10
[   27.679210]  ? security_file_permission+0x131/0x150
[   27.679228]  ? rw_verify_area+0x86/0x230
[   27.679249]  ? __kasan_check_write+0x14/0x20
[   27.679261]  ? kernel_read_file+0x384/0x5f0
[   27.679278]  ? __pfx_kernel_read_file+0x10/0x10
[   27.679292]  ? vm_mmap_pgoff+0x263/0x340
[   27.679309]  init_module_from_file+0x100/0x170
[   27.679322]  ? init_module_from_file+0x100/0x170
[   27.679335]  ? __pfx_init_module_from_file+0x10/0x10
[   27.679351]  ? preempt_count_sub+0x18/0xc0
[   27.679370]  ? __kasan_check_write+0x14/0x20
[   27.679381]  ? _raw_spin_lock+0x87/0xe0
[   27.679398]  ? __pfx__raw_spin_lock+0x10/0x10
[   27.679414]  ? __pfx_cred_has_capability.isra.0+0x10/0x10
[   27.679436]  idempotent_init_module+0x1d4/0x4a0
[   27.679452]  ? __pfx_idempotent_init_module+0x10/0x10
[   27.679469]  ? __kasan_check_read+0x11/0x20
[   27.679485]  ? fdget+0x9d/0x1b0
[   27.679500]  __x64_sys_finit_module+0x89/0xf0
[   27.679515]  x64_sys_call+0x1a99/0x1f90
[   27.679530]  do_syscall_64+0x6b/0x1d0
[   27.679544]  ? debug_smp_processor_id+0x17/0x20
[   27.679557]  ? fpregs_assert_state_consistent+0x7f/0x90
[   27.679574]  ? do_syscall_64+0xad/0x1d0
[   27.679587]  ? debug_smp_processor_id+0x17/0x20
[   27.679600]  ? fpregs_assert_state_consistent+0x7f/0x90
[   27.679616]  ? do_syscall_64+0xad/0x1d0
[   27.679628]  ? debug_smp_processor_id+0x17/0x20
[   27.679641]  ? fpregs_assert_state_consistent+0x7f/0x90
[   27.679656]  ? irqentry_exit_to_user_mode+0x2e/0x180
[   27.679670]  ? clear_bhb_loop+0x30/0x80
[   27.679683]  ? clear_bhb_loop+0x30/0x80
[   27.679695]  ? clear_bhb_loop+0x30/0x80
[   27.679707]  ? clear_bhb_loop+0x30/0x80
[   27.679720]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   27.679733] RIP: 0033:0x7f18d031ea3d
[   27.679746] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 
48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 a3 0f 00 f7 d8 64 89 01 48
[   27.679760] RSP: 002b:00007ffc1655be88 EFLAGS: 00000246 ORIG_RAX: 
0000000000000139
[   27.679774] RAX: ffffffffffffffda RBX: 000055c8c070bba0 RCX: 
00007f18d031ea3d
[   27.679785] RDX: 0000000000000000 RSI: 00007f18d052d441 RDI: 
0000000000000006
[   27.679793] RBP: 0000000000020000 R08: 0000000000000000 R09: 
0000000000000002
[   27.679801] R10: 0000000000000006 R11: 0000000000000246 R12: 
00007f18d052d441
[   27.679810] R13: 000055c8c070c4a0 R14: 000055c8c0819480 R15: 
000055c8c080c410
[   27.679828]  </TASK>
[   27.679834] 
==================================================================


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.


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.

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

[Takashi Iwai]

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
?

> 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.


thanks,

Takashi

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

[Amadeusz Sławiński]

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.

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

[Richard Fitzgerald]

On 29/06/2025 10:22 am, 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.
>>>

I tried your branch on a Cirrus development platform. This has an
out-of-tree patch so was a test of how easily we can apply our backlog
of patches to the new tree. Good news, it was easy to apply the patch
(with only very minor conflicts) and audio works.

Test hardware is Aaeon UpXtreme (WhiskeyLake) + CS8409 + CS35L56 
side-codec amps.

[    5.155395] snd_hda_codec_cs8409 hdaudioC0D0: CS8409: picked fixup 
for codec SSID 1013:8409
[    5.155622] snd_hda_codec_cs8409 hdaudioC0D0: Found 4 CSC3556 on i2c 
(-%s:00-cs35l56-hda.%d)
[    5.159626] snd_hda_codec_cs8409 hdaudioC0D0: bound 
i2c-CSC3556:00-cs35l56-hda.0 (ops cs35l56_hda_comp_ops 
[snd_hda_scodec_cs35l56])
[    5.159955] snd_hda_codec_cs8409 hdaudioC0D0: bound 
i2c-CSC3556:00-cs35l56-hda.1 (ops cs35l56_hda_comp_ops 
[snd_hda_scodec_cs35l56])
[    5.160722] snd_hda_codec_cs8409 hdaudioC0D0: bound 
i2c-CSC3556:00-cs35l56-hda.2 (ops cs35l56_hda_comp_ops 
[snd_hda_scodec_cs35l56])
[    5.160857] snd_hda_codec_cs8409 hdaudioC0D0: bound 
i2c-CSC3556:00-cs35l56-hda.3 (ops cs35l56_hda_comp_ops 
[snd_hda_scodec_cs35l56])
[    5.161763] snd_hda_codec_cs8409 hdaudioC0D0: autoconfig for CS8409: 
line_outs=1 (0x2c/0x0/0x0/0x0/0x0) type:speaker
[    5.161769] snd_hda_codec_cs8409 hdaudioC0D0:    speaker_outs=0 
(0x0/0x0/0x0/0x0/0x0)
[    5.161773] snd_hda_codec_cs8409 hdaudioC0D0:    hp_outs=0 
(0x0/0x0/0x0/0x0/0x0)
[    5.161777] snd_hda_codec_cs8409 hdaudioC0D0:    mono: mono_out=0x0
[    5.161779] snd_hda_codec_cs8409 hdaudioC0D0:    inputs:
[    5.161782] snd_hda_codec_cs8409 hdaudioC0D0:      Mic=0x3c

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

[Takashi Iwai]

On Thu, 03 Jul 2025 15:57:48 +0200,
Amadeusz Sławiński wrote:
> 
> 
> 
> 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 :(

OK, thanks for confirmation.

I guess it's a power up code path that happens just before the codec
driver initialization.

Could you check whether the patch below covers it?


Takashi

--- a/sound/hda/codecs/hdmi/intelhdmi.c
+++ b/sound/hda/codecs/hdmi/intelhdmi.c
@@ -77,7 +77,7 @@ static void intel_haswell_fixup_enable_dp12(struct hda_codec *codec)
 static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 				unsigned int power_state)
 {
-	if (power_state == AC_PWRST_D0) {
+	if (codec->spec && power_state == AC_PWRST_D0) {
 		intel_haswell_enable_all_pins(codec, false);
 		intel_haswell_fixup_enable_dp12(codec);
 	}

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

[Takashi Iwai]

On Thu, 03 Jul 2025 16:04:29 +0200,
Richard Fitzgerald wrote:
> 
> On 29/06/2025 10:22 am, 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.
> >>> 
> 
> I tried your branch on a Cirrus development platform. This has an
> out-of-tree patch so was a test of how easily we can apply our backlog
> of patches to the new tree. Good news, it was easy to apply the patch
> (with only very minor conflicts) and audio works.

Good to hear!  Thanks for verification.


Takashi


> 
> Test hardware is Aaeon UpXtreme (WhiskeyLake) + CS8409 + CS35L56
> side-codec amps.
> 
> [    5.155395] snd_hda_codec_cs8409 hdaudioC0D0: CS8409: picked fixup
> for codec SSID 1013:8409
> [    5.155622] snd_hda_codec_cs8409 hdaudioC0D0: Found 4 CSC3556 on
> i2c (-%s:00-cs35l56-hda.%d)
> [    5.159626] snd_hda_codec_cs8409 hdaudioC0D0: bound
> i2c-CSC3556:00-cs35l56-hda.0 (ops cs35l56_hda_comp_ops
> [snd_hda_scodec_cs35l56])
> [    5.159955] snd_hda_codec_cs8409 hdaudioC0D0: bound
> i2c-CSC3556:00-cs35l56-hda.1 (ops cs35l56_hda_comp_ops
> [snd_hda_scodec_cs35l56])
> [    5.160722] snd_hda_codec_cs8409 hdaudioC0D0: bound
> i2c-CSC3556:00-cs35l56-hda.2 (ops cs35l56_hda_comp_ops
> [snd_hda_scodec_cs35l56])
> [    5.160857] snd_hda_codec_cs8409 hdaudioC0D0: bound
> i2c-CSC3556:00-cs35l56-hda.3 (ops cs35l56_hda_comp_ops
> [snd_hda_scodec_cs35l56])
> [    5.161763] snd_hda_codec_cs8409 hdaudioC0D0: autoconfig for
> CS8409: line_outs=1 (0x2c/0x0/0x0/0x0/0x0) type:speaker
> [    5.161769] snd_hda_codec_cs8409 hdaudioC0D0:    speaker_outs=0
> (0x0/0x0/0x0/0x0/0x0)
> [    5.161773] snd_hda_codec_cs8409 hdaudioC0D0:    hp_outs=0
> (0x0/0x0/0x0/0x0/0x0)
> [    5.161777] snd_hda_codec_cs8409 hdaudioC0D0:    mono: mono_out=0x0
> [    5.161779] snd_hda_codec_cs8409 hdaudioC0D0:    inputs:
> [    5.161782] snd_hda_codec_cs8409 hdaudioC0D0:      Mic=0x3c
> 

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

[Takashi Iwai]

On Thu, 03 Jul 2025 16:32:51 +0200,
Takashi Iwai wrote:
> 
> On Thu, 03 Jul 2025 15:57:48 +0200,
> Amadeusz Sławiński wrote:
> > 
> > 
> > 
> > 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 :(
> 
> OK, thanks for confirmation.
> 
> I guess it's a power up code path that happens just before the codec
> driver initialization.
> 
> Could you check whether the patch below covers it?

A possible alternative is like below.  This one might be safer.
Let me know if either of them can work for you.


thanks,

Takashi

--- a/sound/hda/common/codec.c
+++ b/sound/hda/common/codec.c
@@ -2774,8 +2774,9 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
 
 	/* repeat power states setting at most 10 times*/
 	for (count = 0; count < 10; count++) {
-		/* might be called before binding to driver, too */
-		if (driver && driver->ops && driver->ops->set_power_state)
+		/* might be called before or during binding to driver, too */
+		if (device_is_bound(&codec->core.dev) &&
+		    driver && driver->ops->set_power_state)
 			driver->ops->set_power_state(codec, fg, power_state);
 		else {
 			state = power_state;

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

[Amadeusz Sławiński]

On 2025-07-03 16:43, Takashi Iwai wrote:
> On Thu, 03 Jul 2025 16:32:51 +0200,
> Takashi Iwai wrote:
>>
>> On Thu, 03 Jul 2025 15:57:48 +0200,
>> Amadeusz Sławiński wrote:
>>>
>>>
>>>
>>> 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 :(
>>
>> OK, thanks for confirmation.
>>
>> I guess it's a power up code path that happens just before the codec
>> driver initialization.
>>
>> Could you check whether the patch below covers it?
This one works.

> A possible alternative is like below.  This one might be safer.
> Let me know if either of them can work for you.

This one doesn't.

I will run more tests on the working one tomorrow, unless you want to 
try something else.

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

[Takashi Iwai]

On Thu, 03 Jul 2025 16:52:10 +0200,
Amadeusz Sławiński wrote:
> 
> 
> 
> On 2025-07-03 16:43, Takashi Iwai wrote:
> > On Thu, 03 Jul 2025 16:32:51 +0200,
> > Takashi Iwai wrote:
> >> 
> >> On Thu, 03 Jul 2025 15:57:48 +0200,
> >> Amadeusz Sławiński wrote:
> >>> 
> >>> 
> >>> 
> >>> 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 :(
> >> 
> >> OK, thanks for confirmation.
> >> 
> >> I guess it's a power up code path that happens just before the codec
> >> driver initialization.
> >> 
> >> Could you check whether the patch below covers it?
> This one works.
> 
> > A possible alternative is like below.  This one might be safer.
> > Let me know if either of them can work for you.
> 
> This one doesn't.

Interesting.  So the device_is_bound() returns true if it's called
during the binding operation.

> I will run more tests on the working one tomorrow, unless you want to
> try something else.

Thanks!


Takashi

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

[Amadeusz Sławiński]

On 2025-07-03 17:12, Takashi Iwai wrote:
> On Thu, 03 Jul 2025 16:52:10 +0200,
> Amadeusz Sławiński wrote:
>>
>>
>>
>> On 2025-07-03 16:43, Takashi Iwai wrote:
>>> On Thu, 03 Jul 2025 16:32:51 +0200,
>>> Takashi Iwai wrote:
>>>>
>>>> On Thu, 03 Jul 2025 15:57:48 +0200,
>>>> Amadeusz Sławiński wrote:
>>>>>
>>>>>
>>>>>
>>>>> 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 :(
>>>>
>>>> OK, thanks for confirmation.
>>>>
>>>> I guess it's a power up code path that happens just before the codec
>>>> driver initialization.
>>>>
>>>> Could you check whether the patch below covers it?
>> This one works.
>>
>>> A possible alternative is like below.  This one might be safer.
>>> Let me know if either of them can work for you.
>>
>> This one doesn't.
> 
> Interesting.  So the device_is_bound() returns true if it's called
> during the binding operation.
> 
>> I will run more tests on the working one tomorrow, unless you want to
>> try something else.
> 
> Thanks!

Apart from unrelated issue in snd_soc_avs driver for which I just sent a 
fix [1]. Everything looks good to me.

[1] 
https://lore.kernel.org/linux-sound/20250708105009.1883627-1-amadeuszx.slawinski@linux.intel.com/T/#u

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

[Takashi Iwai]

On Tue, 08 Jul 2025 12:56:37 +0200,
Amadeusz Sławiński wrote:
> 
> 
> 
> On 2025-07-03 17:12, Takashi Iwai wrote:
> > On Thu, 03 Jul 2025 16:52:10 +0200,
> > Amadeusz Sławiński wrote:
> >> 
> >> 
> >> 
> >> On 2025-07-03 16:43, Takashi Iwai wrote:
> >>> On Thu, 03 Jul 2025 16:32:51 +0200,
> >>> Takashi Iwai wrote:
> >>>> 
> >>>> On Thu, 03 Jul 2025 15:57:48 +0200,
> >>>> Amadeusz Sławiński wrote:
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> 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 :(
> >>>> 
> >>>> OK, thanks for confirmation.
> >>>> 
> >>>> I guess it's a power up code path that happens just before the codec
> >>>> driver initialization.
> >>>> 
> >>>> Could you check whether the patch below covers it?
> >> This one works.
> >> 
> >>> A possible alternative is like below.  This one might be safer.
> >>> Let me know if either of them can work for you.
> >> 
> >> This one doesn't.
> > 
> > Interesting.  So the device_is_bound() returns true if it's called
> > during the binding operation.
> > 
> >> I will run more tests on the working one tomorrow, unless you want to
> >> try something else.
> > 
> > Thanks!
> 
> Apart from unrelated issue in snd_soc_avs driver for which I just sent
> a fix [1]. Everything looks good to me.
> 
> [1]
> https://lore.kernel.org/linux-sound/20250708105009.1883627-1-amadeuszx.slawinski@linux.intel.com/T/#u

Thanks for verification!

Then I'm going to submit the patches in this week, maybe after sending
a PR for 6.15-rc6.


Takashi