Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 27 May 2022 18:13:38 +0200
Message-ID: <871qwf0x8t.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        Mark Brown <broonie@kernel.org>, <alsa-devel@alsa-project.org>,
        <patches@opensource.cirrus.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 00/17] ALSA: hda: cirrus: Add initial DSP support and firmware loading
In-Reply-To: <20220525131638.5512-1-vitalyr@opensource.cirrus.com>
References: <20220525131638.5512-1-vitalyr@opensource.cirrus.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 25 May 2022 15:16:21 +0200,
Vitaly Rodionov wrote:
> 
> The CS35L41 Amplifier contains a DSP, capable of running firmware.
> The firmware can run algorithms such as Speaker Protection, to ensure
> that playback at high gains do not harm the speakers.
> Adding support for CS35L41 firmware into the CS35L41 HDA driver also
> allows us to support several extra features, such as hiberation 
> and interrupts.
> 
> The chain adds support in stages:
> - General fixes to improve generalization and code re-use inside
>   the CS35L41 HDA driver.
> - Add support for interrupts into the driver, which is required
>   for complete support of the firmware.
> - Refactor ASoC CS35L41 code which deals with firmware to allow
>   for code re-use inside the CS35L41 HDA driver.
> - Add support for loading firmware and tuning files from file system,
>   and creating alsa controls to control it.
> - Support firmware load paths for different hardware systems.
> - Support suspend/resume in the driver when using firmware. The firmware
>   supports hibernation, which allows the CS35L41 to drop into a low
>   power mode during suspend.
> - Support the ability to unload firmware, swap and reload the firmware.
>   This is to allow different firmware to run during calibration.
> 
> The intended use-case is to load the firmware once on boot, and the driver
> autmatically tries to load the firmware after it binds to the HDA driver.
> This behaviour can be switched off using a kconfig, if desired.

The idea to add / delete controls by the control element change
doesn't sound good; as already mentioned in my reply to the previous
patch set, the change of the control elements can be triggered too
easily by any normal users who have the access to the sound devices.
It means thousands of additions and removals per second could be
attacked by any user.

Moreover, the new controls with TLV controls don't look following the
standard TLV syntax (type-length-value).  My previous questions about
the TLV usages are still unanswered, so I'm not sure what impact this
would have, though.  At least, alsactl behavior must be checked
beforehand. If this is really device-specific (non-)TLV usages, it has
to be clearly documented.

I don't mean fully against such a TLV usage, *IFF* the same pattern
has been already used in ASoC side.  In that case, we may need to
introduce some PCM info flag to indicate a non-standard TLV usage (but
it's a bit different story).

OTOH, the too easily triggered control addition/removal is likely
no-go, as this could be a cause of DoS-like attacks.  If we must to in
this direction, it has to be verified and clarified.


thanks,

Takashi