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 13:07:55 +0200
Message-ID: <87wne3wa5w.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: <20220530105329.GV38351@ediswmail.ad.cirrus.com>
References: <20220525131638.5512-1-vitalyr@opensource.cirrus.com>
        <871qwf0x8t.wl-tiwai@suse.de>
        <20220530090846.GS38351@ediswmail.ad.cirrus.com>
        <87czfvxtsc.wl-tiwai@suse.de>
        <20220530093639.GT38351@ediswmail.ad.cirrus.com>
        <87a6azxr7h.wl-tiwai@suse.de>
        <20220530103415.GU38351@ediswmail.ad.cirrus.com>
        <871qwbxpsb.wl-tiwai@suse.de>
        <20220530105329.GV38351@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 12:53:29 +0200,
Charles Keepax wrote:
> 
> On Mon, May 30, 2022 at 12:45:08PM +0200, Takashi Iwai wrote:
> > On Mon, 30 May 2022 12:34:15 +0200,
> > Charles Keepax wrote:
> > 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...?
> > 
> > > I guess we could look at adding locked versions of the add
> > > control functions as well if that might be preferred?
> > 
> > If the patterns of additional kctls (specific for firmware?) are
> > fixed, we may create all such kctls beforehand and let them inactive
> > unless the corresponding firmware is really loaded, too.
> > 
> 
> I am afraid we do, basically all the Wolfson/Cirrus audio devices
> allow you to select the firmware through a kctl. The patterns of
> controls are specific to the firmwares, so we can't really create
> them ahead of time. One could maybe look at changing when the
> firmwares are loaded, such as attempting to load all possible
> firmwares on boot or something but its a fairly sizable change
> that isn't without some side effects.

The call of request_firmawre() itself can be pretty lengthy (e.g. it
may hold until user-space process uploads the firmware if the fallback
mode is enabled), and it implies that the request_firmware() call
doesn't fit well as the operation to be done in a kctl put callback.
So, even if we accept the firmware loading behavior via kctl as-is,
the whole procedure should be async in work instead; namely, not only
kctl creation/deletion but both request_firmware() + post-process
should be done in the work.

And yet moreover, we'll need to consider some way for protecting
against DoS-like behavior by frequent kctl changes.


thanks,

Takashi