[SRU] [L/Unstable] [PATCH 00/16] NULL pointer dereference on CS35L41 HDA AMP Edit

[Kai-Heng Feng]

On Wed, Aug 2, 2023 at 3:58 PM Stefan Bader <stefan.bader at canonical.com> wrote:
>
> On 01.08.23 10:36, Kai-Heng Feng wrote:
> > BugLink: https://bugs.launchpad.net/bugs/2029199
> >
> > [Impact]
> > NULL pointer dereference happens because the HDA driver is trying to use
> > CS35L41 HDA AMP before its probe routine is fully realized.
> >
> > [Fix]
> > Use device link to enforce proper probe order.
> > Since the fixing commit is part of a patch series, pull in the entire
> > series which has several other important fixes too.
> >
> > [Test]
> > Use dmesg to see if there's error. With the fix applied, no more kernel
> > splat can be found. Hence the system can perform suspend, reboot and
> > shutdown normally.
> >
> > [Where problems could occur]
> > Though the entire series isn't trivia, it's not a major overhaul either.
> > The entire changeset is limited to CS35L41 AMP, so the scope of
> > regression risk is constrained.
> > One possible risk factor is that it may require newer DSP firmware, and
> > we'll monitor it closely on such scenario.
> >
> > Lucas Tanure (3):
> >    ASoC: cs35l41: Refactor error release code
> >    ALSA: cs35l41: Add shared boost feature
> >    ASoC: dt-bindings: cirrus,cs35l41: Document CS35l41 shared boost
> >
> > Stefan Binding (13):
> >    ALSA: hda: cs35l41: Ensure firmware/tuning pairs are always loaded
> >    ALSA: hda: cs35l41: Enable Amp High Pass Filter
> >    ALSA: cs35l41: Use mbox command to enable speaker output for external
> >      boost
> >    ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
> >      delay
> >    ALSA: hda: cs35l41: Check mailbox status of pause command after
> >      firmware load
> >    ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before system
> >      suspending.
> >    ALSA: hda: cs35l41: Ensure we pass up any errors during system
> >      suspend.
> >    ALSA: hda: cs35l41: Move Play and Pause into separate functions
> >    ALSA: hda: hda_component: Add pre and post playback hooks to
> >      hda_component
> >    ALSA: hda: cs35l41: Use pre and post playback hooks
> >    ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
> >      separation
> >    ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
> >    ALSA: hda: cs35l41: Ensure amp is only unmuted during playback
> >
> >   .../bindings/sound/cirrus,cs35l41.yaml        |  10 +-
> >   include/sound/cs35l41.h                       |  14 +-
> >   sound/pci/hda/cs35l41_hda.c                   | 395 ++++++++++++------
> >   sound/pci/hda/hda_component.h                 |   2 +
> >   sound/pci/hda/patch_realtek.c                 |  10 +-
> >   sound/soc/codecs/cs35l41-lib.c                | 185 +++++++-
> >   sound/soc/codecs/cs35l41.c                    | 101 ++---
> >   sound/soc/codecs/cs35l41.h                    |   1 +
> >   8 files changed, 522 insertions(+), 196 deletions(-)
> >
>
> I am a bit hesitant with that for Lunar. As tempting it is to pull in
> complete patchsets it is not what we do for SRU and there is a reason to
> it. This set includes at least one new feature (shared boost) which adds
> risk of breaking. Review gets hard and testing as well. So imo SRU for
> Lunar should *only* fix a specific issue and for that try to only
> include the minimum dependencies.

I understand your concern.

However, from backporting perspective, if only fixing commits get
backported and the remaining series are omitted, it can make future
backport really hard, if the omitting part is necessary to said
backport.

And the frustration builds up really fast because it already happened
several times for me. So I really hope this SRU can get into Lunar.
The OEM kernel migration will definitely need this fix.

As for the "shared boost" thing, it's not used by HDA system, so the
only part that affect existing systems is the CS35L41_PLL_LOCK check
in ISR, though it doesn't mean it's regression free, but at least we
can spot what's wrong easily.

Kai-Heng

>
> --
> - Stefan
>