- Sound-open-firmware - mailman.alsa-project.org

Re: [PATCH v2 00/14] ASoC: Constify local snd_sof_dsp_ops
by Mark Brown 01 May '24

01 May '24

On Fri, 26 Apr 2024 11:03:47 +0200, Krzysztof Kozlowski wrote: > Changes in v2 > ============= > - Only rebase on for-next of broonie/sound.git > - Add tags. > - Link to v1: https://lore.kernel.org/r/20240414-n-const-ops-var-v1-0-8f53ee5d981c@kernel… > > Description > =========== > The core code does not modify the 'struct snd_sof_dsp_ops' passed via > pointer in various places, so this can be made pointer to const in few > places. This in turn allows few drivers to have the local (usually > static) 'struct snd_sof_dsp_ops' as const which increased code safety, > as it is now part of rodata. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [01/14] ASoC: SOF: debug: Constify local snd_sof_dsp_ops commit: ffca099bbff1978bc9c97b076f0d35b4fe6ddf27 [02/14] ASoC: SOF: ipc3: Constify local snd_sof_dsp_ops commit: ee5acc1e035ec5ed5d9f0f63fda9d627220090c2 [03/14] ASoC: SOF: pcm: Constify local snd_sof_dsp_ops commit: a0db037df9630edad76153c7937c6f5ca04ed44f [04/14] ASoC: SOF: Constify stored pointer to snd_sof_dsp_ops commit: 8bbc692d1abce5bc949dea9acba85fc686601c04 [05/14] ASoC: SOF: intel: pci-tng: Constify snd_sof_dsp_ops commit: 8f2b0d55abc44676b076128903a5dc730aab23c6 [06/14] ASoC: SOF: intel: hda: Constify snd_sof_dsp_ops commit: 6032eefc2c478243a511352dda04495c9a9fec6a [07/14] ASoC: SOF: amd: acp: Constify snd_sof_dsp_ops commit: 04f2f516be09d5493d764e0020a771c46b5470d8 [08/14] ASoC: SOF: imx8: Constify snd_sof_dsp_ops commit: ab85c44973298b69eb32795de11ce4d426705532 [09/14] ASoC: SOF: imx8m: Constify snd_sof_dsp_ops commit: 66d49ab5fb51bb8d1b4c2c9c8fa0fbe8e4c8ca1c [10/14] ASoC: SOF: imx8ulp: Constify snd_sof_dsp_ops commit: 232e0da9fa778233358586617bd22173bcac6bcc [11/14] ASoC: SOF: intel: bdw: Constify snd_sof_dsp_ops commit: 936cc56044a87ae7fbd0e4098a7daefa0f2f4e8e [12/14] ASoC: SOF: intel: byt: Constify snd_sof_dsp_ops commit: 48d5f1800d0cbda0212c5a58177918c419a24f8a [13/14] ASoC: SOF: mediatek: mt8186: Constify snd_sof_dsp_ops commit: fe80673f59da01776a1402e4b508a66fca43a24d [14/14] ASoC: SOF: mediatek: mt8195: Constify snd_sof_dsp_ops commit: 8b6d678fede700db6466d73f11fcbad496fa515e 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

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Mark Brown 01 May '24

01 May '24

On Sun, 14 Apr 2024 20:47:25 +0200, Krzysztof Kozlowski wrote: > The core code does not modify the 'struct snd_sof_dsp_ops' passed via > pointer in various places, so this can be made pointer to const. > > All further patches depend on the first four patches. > > Best regards, > Krzysztof > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [01/14] ASoC: SOF: debug: Constify local snd_sof_dsp_ops commit: ffca099bbff1978bc9c97b076f0d35b4fe6ddf27 [02/14] ASoC: SOF: ipc3: Constify local snd_sof_dsp_ops commit: ee5acc1e035ec5ed5d9f0f63fda9d627220090c2 [03/14] ASoC: SOF: pcm: Constify local snd_sof_dsp_ops commit: a0db037df9630edad76153c7937c6f5ca04ed44f [04/14] ASoC: SOF: Constify stored pointer to snd_sof_dsp_ops commit: 8bbc692d1abce5bc949dea9acba85fc686601c04 [05/14] ASoC: SOF: intel: pci-tng: Constify snd_sof_dsp_ops commit: 8f2b0d55abc44676b076128903a5dc730aab23c6 [06/14] ASoC: SOF: intel: hda: Constify snd_sof_dsp_ops commit: 6032eefc2c478243a511352dda04495c9a9fec6a [07/14] ASoC: SOF: amd: acp: Constify snd_sof_dsp_ops commit: 04f2f516be09d5493d764e0020a771c46b5470d8 [08/14] ASoC: SOF: imx8: Constify snd_sof_dsp_ops commit: ab85c44973298b69eb32795de11ce4d426705532 [09/14] ASoC: SOF: imx8m: Constify snd_sof_dsp_ops commit: 66d49ab5fb51bb8d1b4c2c9c8fa0fbe8e4c8ca1c [10/14] ASoC: SOF: imx8ulp: Constify snd_sof_dsp_ops commit: 232e0da9fa778233358586617bd22173bcac6bcc [11/14] ASoC: SOF: intel: bdw: Constify snd_sof_dsp_ops commit: 936cc56044a87ae7fbd0e4098a7daefa0f2f4e8e [12/14] ASoC: SOF: intel: byt: Constify snd_sof_dsp_ops commit: 48d5f1800d0cbda0212c5a58177918c419a24f8a [13/14] ASoC: SOF: mediatek: mt8186: Constify snd_sof_dsp_ops commit: fe80673f59da01776a1402e4b508a66fca43a24d [14/14] ASoC: SOF: mediatek: mt8195: Constify snd_sof_dsp_ops commit: 8b6d678fede700db6466d73f11fcbad496fa515e 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

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Pierre-Louis Bossart 25 Apr '24

25 Apr '24

On 4/23/24 11:06, Krzysztof Kozlowski wrote: > On 23/04/2024 17:57, Pierre-Louis Bossart wrote: >> >>> Are we talking about basic C now? Of course it does not mean that and I >>> already explained what is the goal of this - the static or global memory >>> in the driver can be moved to rodata. >> >> the dsp_ops used by the Intel drivers cannot be moved to rodata in all >> cases. the baseline is constant, all extensions for TGL+ are dynamically >> allocated and modified. > > Yes and these drivers will not benefit. Did I changed them here? I think > I didn't, because then it would not compile. So we do agree that's there's no benefit on any Intel platform released in the last 4 years... The CI tests [1] don't show any regression, nothing was broken on our devices so I guess there's no further objection. Tested-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> [1] https://github.com/thesofproject/linux/pull/4949

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Pierre-Louis Bossart 23 Apr '24

23 Apr '24

> Are we talking about basic C now? Of course it does not mean that and I > already explained what is the goal of this - the static or global memory > in the driver can be moved to rodata. the dsp_ops used by the Intel drivers cannot be moved to rodata in all cases. the baseline is constant, all extensions for TGL+ are dynamically allocated and modified.

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Pierre-Louis Bossart 22 Apr '24

22 Apr '24

>>> There are multiple reasons and benefits for const, like compiler >>> optimization, code readability (meaning) up to security improvements, >>> e.g. by some GCC plugins or marking rodata as really non-writeable, so >>> closing some ways of exploits. There are many opportunities here, even >>> if they are not yet enabled. >> >> Possibly, but the SOF core does not know if the structure it uses is >> rodata or not. Using the 'const' identifier would be misleading. > > How so? If core does not modify structure, it should take it via ops, > just like 100 other widely known structures (see checkpatch). Why is > this different? I don't understand "it should take it via ops" We are already fetching the structure with private_data. >>>> that's a different interpretation to the 'software' view you're >>>> describing. "this structure will not modified by this function" is not >>>> the same thing as "this structure CANNOT be modified". >>> >>> Yes, but can we please discuss specific patchset then? Patches which >>> change pointers to const have one "interpretation". Patches which modify >>> static or global data have another. >> >> Just look at sound/soc/sof/intel/mtl.c... The core will sometimes use a > > That's a driver (or specific implementation), not core. You are making an assumption on what the SOF core is. The core is used by ACPI or PCI drivers as a library. The structures are all allocated in ACPI/PCI drivers and passed to the core library. >> constant structure and sometimes not, depending on the PCI ID reported >> by hardware. This was intentional to override common defaults and make >> the differences limited in scope between hardware generations. > > >> >> int sof_mtl_ops_init(struct snd_sof_dev *sdev) >> { >> struct sof_ipc4_fw_data *ipc4_data; >> >> /* common defaults */ >> memcpy(&sof_mtl_ops, &sof_hda_common_ops, sizeof(struct >> snd_sof_dsp_ops)); <<<< THE BASELINE IS CONSTANT > > Yes, I saw it and such users are not changed. They won't receive any > safety. But all others are getting safer. Maybe there's a misunderstanding on what the 'SOF core' is. This is just a helper library that are used by the PCI drivers. The core has zero knowledge on anything really. > I really do not understand what is the problem here. In entire Linux all > of such changes are welcomed with open arms. So what is different here? Adding 'const' at the SOF core level does not mean that we can treat structures as rodata. It only means that the structure is not modified by the core library. Not the same thing.

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Pierre-Louis Bossart 22 Apr '24

22 Apr '24

> There are multiple reasons and benefits for const, like compiler > optimization, code readability (meaning) up to security improvements, > e.g. by some GCC plugins or marking rodata as really non-writeable, so > closing some ways of exploits. There are many opportunities here, even > if they are not yet enabled. Possibly, but the SOF core does not know if the structure it uses is rodata or not. Using the 'const' identifier would be misleading. >> that's a different interpretation to the 'software' view you're >> describing. "this structure will not modified by this function" is not >> the same thing as "this structure CANNOT be modified". > > Yes, but can we please discuss specific patchset then? Patches which > change pointers to const have one "interpretation". Patches which modify > static or global data have another. Just look at sound/soc/sof/intel/mtl.c... The core will sometimes use a constant structure and sometimes not, depending on the PCI ID reported by hardware. This was intentional to override common defaults and make the differences limited in scope between hardware generations. int sof_mtl_ops_init(struct snd_sof_dev *sdev) { struct sof_ipc4_fw_data *ipc4_data; /* common defaults */ memcpy(&sof_mtl_ops, &sof_hda_common_ops, sizeof(struct snd_sof_dsp_ops)); <<<< THE BASELINE IS CONSTANT <<< THE REST ISN'T. /* shutdown */ sof_mtl_ops.shutdown = hda_dsp_shutdown; /* doorbell */ sof_mtl_ops.irq_thread = mtl_ipc_irq_thread; /* ipc */ sof_mtl_ops.send_msg = mtl_ipc_send_msg; sof_mtl_ops.get_mailbox_offset = mtl_dsp_ipc_get_mailbox_offset; sof_mtl_ops.get_window_offset = mtl_dsp_ipc_get_window_offset; /* debug */ sof_mtl_ops.debug_map = mtl_dsp_debugfs; sof_mtl_ops.debug_map_count = ARRAY_SIZE(mtl_dsp_debugfs); sof_mtl_ops.dbg_dump = mtl_dsp_dump; sof_mtl_ops.ipc_dump = mtl_ipc_dump;

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Pierre-Louis Bossart 22 Apr '24

22 Apr '24

On 4/22/24 00:43, Krzysztof Kozlowski wrote: > On 15/04/2024 16:19, Pierre-Louis Bossart wrote: >> >>> The core code does not modify the 'struct snd_sof_dsp_ops' passed via >>> pointer in various places, so this can be made pointer to const. >> >> The structure itself is NOT always const - the initialization itself >> does have platform-specific changes, so what do we really gain from all >> this? > > In the context of these patches, the structure is *always* const. In > other drivers, it is not, but they are not really relevant here. > >> >> some commit messages say the code is "a bit safer", but I personally >> find the 'const' more confusing since the information that the structure >> can be modified during initialization is lost. > > Functions which take some data and do not modify it are easier to read > if the pointed data is marked as const. Then it is obvious that > functions for example is re-entrant. Or that it does not affect the > state of other devices/core structures. > > Additionally, the static data is safer when is const, because it cannot > be used in some attacks. agree, but here you are marking as 'const' non-static data. > I really do not understand which information you lost here? Core does > not change the ops, so the data should be passed as const as often as > possible. If anyone wants to write a driver which does not use static > ops, but somehow dynamically allocated and changed, nothing stops him. > This patch did not make it less readable/doable. > > The point is that these ops do not differ from other ops or some other > driver-passed structures, which we have around 100 already in checkpatch. I am so old that I remember times where we had to put things in ROM. That's what 'const' means to me: a dedicated memory space for immutable values. that's a different interpretation to the 'software' view you're describing. "this structure will not modified by this function" is not the same thing as "this structure CANNOT be modified". I am not going to lay on the tracks, if Mark wants to apply the patches that's fine. I just wanted to highlight that the reason we did not use 'const' was that the data is dynamically allocated/modified and not constant at all.

1 0

Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops
by Pierre-Louis Bossart 15 Apr '24

15 Apr '24

> The core code does not modify the 'struct snd_sof_dsp_ops' passed via > pointer in various places, so this can be made pointer to const. The structure itself is NOT always const - the initialization itself does have platform-specific changes, so what do we really gain from all this? some commit messages say the code is "a bit safer", but I personally find the 'const' more confusing since the information that the structure can be modified during initialization is lost.

1 0

Re: [PATCH 13/14] ASoC: SOF: mediatek: mt8186: Constify snd_sof_dsp_ops
by AngeloGioacchino Del Regno 15 Apr '24

15 Apr '24

Il 14/04/24 20:47, Krzysztof Kozlowski ha scritto: > 'struct snd_sof_dsp_ops' is not modified by core code, so it can be made > const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski <krzk(a)kernel.org> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

1 0

Re: [PATCH 14/14] ASoC: SOF: mediatek: mt8195: Constify snd_sof_dsp_ops
by AngeloGioacchino Del Regno 15 Apr '24

15 Apr '24

Il 14/04/24 20:47, Krzysztof Kozlowski ha scritto: > 'struct snd_sof_dsp_ops' is not modified by core code, so it can be made > const for increased code safety. > > Signed-off-by: Krzysztof Kozlowski <krzk(a)kernel.org> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

1 0