linux-kernel - Re: [PATCH v4 00/17] ALSA: hda: cirrus: Add initial DSP support and firmware loading

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87mtewyz64.wl-tiwai@suse.de>
Date:   Wed, 01 Jun 2022 21:13:55 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Vitaly Rodionov <vitalyr@...nsource.cirrus.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, Mark Brown <broonie@...nel.org>,
        <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 00/17] ALSA: hda: cirrus: Add initial DSP support and firmware loading

On Wed, 01 Jun 2022 18:43:31 +0200,
Richard Fitzgerald wrote:
> 
> On 30/05/2022 11:45, Takashi Iwai wrote:
> > On Mon, 30 May 2022 12:34:15 +0200,
> > Charles Keepax wrote:
> >> 
> >> On Mon, May 30, 2022 at 12:14:26PM +0200, Takashi Iwai wrote:
> >>> On Mon, 30 May 2022 11:36:39 +0200,
> >>> Charles Keepax wrote:
> >>>> On Mon, May 30, 2022 at 11:18:43AM +0200, Takashi Iwai wrote:
> >>>>> On Mon, 30 May 2022 11:08:46 +0200,
> >>>>> Charles Keepax wrote:
> >>>>>> On Fri, May 27, 2022 at 06:13:38PM +0200, Takashi Iwai wrote:
> >>>>>>> On Wed, 25 May 2022 15:16:21 +0200,
> >>>>>>> Vitaly Rodionov wrote:
> >>>> Yeah that should be what is happening here. Although it looks
> >>>> like this code might be removing all the controls if the firmware
> >>>> is unloaded. I will discuss that with the guys, we normal just
> >>>> disable the controls on the wm_adsp stuff.
> >>> 
> >>> OK, that sounds good.  Basically my concern came up from the code
> >>> snippet doing asynchronous addition/removal via work.  This showed
> >>> some yellow signal, as such a pattern doesn't appear in the normal
> >>> implementation.  If this is (still) really necessary, it has to be
> >>> clarified as an exception.
> >>> 
> >> 
> >> Hm... ok we will think about that. I think that part will
> >> probably still be necessary. Because there is an ALSA control
> >> that selects the firmware, then it is necesarry to defer creating
> >> the controls to some work, since you are already holding the
> >> lock.
> > 
> > Well, if an ALSA control can trigger the firmware loading, that's
> > already fragile.  A firmware loading is a heavy task, which should
> > happen only at probing and/or resuming in general.  Do we have other
> > drivers doing the f/w loading triggered by a kctl...?
> > 
> 
> On Wolfson/Cirrus codecs the firmware isn't to "make the chip work".
> The DSP is programmable to allow for additional audio processing
> algorithms. Which algorithm you need depends on the audio use case(s)
> you are running, and can change as you change use-case. Many of the
> codecs don't have enough DSP memory to hold all possible algorithms.
> Which is why the firmware load has always been triggered from ALSA
> controls in the ASoC code. It's not something that can be loaded
> once in probe().

But it's still a question whether such an easily triggerable interface
should be used for a heavy procedure like the DSP firmware load.
e.g. calling the standard request_firmware() API directly from a
control callback doesn't sound like a good idea at all.  The similar
argument is applied to the dynamic addition/removal of other controls
followed by a kctl callback.


thanks,

Takashi