Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 30 May 2022 11:18:43 +0200
Message-ID: <87czfvxtsc.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>,
        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: <20220530090846.GS38351@ediswmail.ad.cirrus.com>
References: <20220525131638.5512-1-vitalyr@opensource.cirrus.com>
        <871qwf0x8t.wl-tiwai@suse.de>
        <20220530090846.GS38351@ediswmail.ad.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 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:
> > 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.
> > 
> 
> This I am a little less sure how we handle. I mean arn't there
> already a few ways to do this? Both the existing ASoC wm_adsp
> stuff, and the topology stuff (used on all new Intel platforms,
> so very widely deployed) let you create controls by loading a
> firmware file. Also within ALSA itself can't user-space create
> user ALSA controls? Is there some rate limiting on that? How is
> this issue tackled there?

The creation of kctls via firmware loading would be OK, as the code
path can't be triggered so frequently.  Is it the case for this patch
set?  There was too little information about the implementation (and
more importantly, how to use the controls), so it's hard to judge...

And yes, a rate-limit could be implemented for the user controls.
Or, even the hard upper limit for number of additions/deletions per
process could be set, I suppose.  (Currently only the total number of
ctl elements are managed.)

> > 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.
> > 
> 
> The TLV stuff should be completely removed once my latest
> comments have been updated. I don't think we need this for the
> amps and I would also rather keep the usage to a minimum until
> one of us finally gets around to resolving the large control
> issues in a way that is more acceptable to everyone (likely
> with a new IOCTL).

It'll be great if the complexity could be reduced.


thanks,

Takashi