| Message ID | 20220525131638.5512-1-vitalyr@opensource.cirrus.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ALSA: hda: cirrus: Add initial DSP support and firmware loading | expand |
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
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? > 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). Thanks, Charles
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
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: > > > 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... > 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. Thanks, Charles
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: > > > > 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... > > > > 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. thanks, Takashi
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. I guess we could look at adding locked versions of the add control functions as well if that might be preferred? Thanks, Charles
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...? > 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. thanks, Takashi
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. Thanks, Charles
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
On 30. 05. 22 13:07, Takashi Iwai wrote: > And yet moreover, we'll need to consider some way for protecting > against DoS-like behavior by frequent kctl changes. I agree, but only the driver knows details about the kctl operation time and resource constraints. So the driver should implement a rate or i/o limit for those controls. We may offer helper functions in the ALSA core for this job (if required). Jaroslav
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().
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
On Wed, 25 May 2022 14:16:21 +0100, 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. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [09/17] ASoC: cs35l41: Move cs35l41 exit hibernate function into shared code commit: 94e0bc317ad241c022a6bb311b3a28b4d51ea8b6 [10/17] ASoC: cs35l41: Do not print error when waking from hibernation commit: 97076475e2fdf471348b9ce73215cdbceeb4390f [11/17] ASoC: cs35l41: Add common cs35l41 enter hibernate function commit: e341efc308e5374ded6b471f9e1ec01450bcc93e All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
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. Stefan Binding (16): ALSA: hda: hda_cs_dsp_ctl: Add Library to support CS_DSP ALSA controls ALSA: hda: hda_cs_dsp_ctl: Add apis to write the controls directly ALSA: hda: cs35l41: Save codec object inside component struct ALSA: hda: cs35l41: Save Subsystem ID inside CS35L41 Driver ALSA: hda: cs35l41: Support reading subsystem id from ACPI ALSA: hda: cs35l41: Support multiple load paths for firmware ALSA: hda: cs35l41: Support Speaker ID for laptops ASoC: cs35l41: Move cs35l41 exit hibernate function into shared code ASoC: cs35l41: Do not print error when waking from hibernation ASoC: cs35l41: Add common cs35l41 enter hibernate function ALSA: hda: cs35l41: Support Hibernation during Suspend ALSA: hda: cs35l41: Read Speaker Calibration data from UEFI variables ALSA: hda: hda_cs_dsp_ctl: Add fw id strings ALSA: hda: cs35l41: Add defaulted values into dsp bypass config sequence ALSA: hda: cs35l41: Support Firmware switching and reloading ALSA: hda: cs35l41: Add module parameter to control firmware load Vitaly Rodionov (1): ALSA: hda: cs35l41: Add initial DSP support and firmware loading MAINTAINERS | 1 + include/sound/cs35l41.h | 7 + sound/pci/hda/Kconfig | 8 + sound/pci/hda/Makefile | 2 + sound/pci/hda/cs35l41_hda.c | 853 +++++++++++++++++++++++++++++++- sound/pci/hda/cs35l41_hda.h | 37 ++ sound/pci/hda/cs35l41_hda_i2c.c | 1 + sound/pci/hda/cs35l41_hda_spi.c | 1 + sound/pci/hda/hda_component.h | 4 + sound/pci/hda/hda_cs_dsp_ctl.c | 302 +++++++++++ sound/pci/hda/hda_cs_dsp_ctl.h | 40 ++ sound/pci/hda/patch_realtek.c | 27 +- sound/soc/codecs/cs35l41-lib.c | 82 ++- sound/soc/codecs/cs35l41.c | 71 +-- 14 files changed, 1362 insertions(+), 74 deletions(-) create mode 100644 sound/pci/hda/hda_cs_dsp_ctl.c create mode 100644 sound/pci/hda/hda_cs_dsp_ctl.h