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

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

[PATCH AUTOSEL 6.6 01/12] ASoC: SOF: Intel: hda-dsp: Skip IMR boot on ACE platforms in case of S3 suspend
by Sasha Levin 15 Apr '24

15 Apr '24

From: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> [ Upstream commit c61115b37ff964d63191dbf4a058f481daabdf57 ] SoCs with ACE architecture are tailored to use s2idle instead deep (S3) suspend state and the IMR content is lost when the system is forced to enter even to S3. When waking up from S3 state the IMR boot will fail as the content is lost. Set the skip_imr_boot flag to make sure that we don't try IMR in this case. Signed-off-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Rander Wang <rander.wang(a)intel.com> Reviewed-by: Liam Girdwood <liam.r.girdwood(a)intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> Link: https://msgid.link/r/20240322112504.4192-1-peter.ujfalusi@linux.intel.com Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/intel/hda-dsp.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 44f39a520bb39..e80a2a5ec56a1 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -681,17 +681,27 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend) struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; const struct sof_intel_dsp_desc *chip = hda->desc; struct hdac_bus *bus = sof_to_bus(sdev); + bool imr_lost = false; int ret, j; /* - * The memory used for IMR boot loses its content in deeper than S3 state - * We must not try IMR boot on next power up (as it will fail). - * + * The memory used for IMR boot loses its content in deeper than S3 + * state on CAVS platforms. + * On ACE platforms due to the system architecture the IMR content is + * lost at S3 state already, they are tailored for s2idle use. + * We must not try IMR boot on next power up in these cases as it will + * fail. + */ + if (sdev->system_suspend_target > SOF_SUSPEND_S3 || + (chip->hw_ip_version >= SOF_INTEL_ACE_1_0 && + sdev->system_suspend_target == SOF_SUSPEND_S3)) + imr_lost = true; + + /* * In case of firmware crash or boot failure set the skip_imr_boot to true * as well in order to try to re-load the firmware to do a 'cold' boot. */ - if (sdev->system_suspend_target > SOF_SUSPEND_S3 || - sdev->fw_state == SOF_FW_CRASHED || + if (imr_lost || sdev->fw_state == SOF_FW_CRASHED || sdev->fw_state == SOF_FW_BOOT_FAILED) hda->skip_imr_boot = true; -- 2.43.0

1 0

[PATCH AUTOSEL 6.8 01/15] ASoC: SOF: Intel: hda-dsp: Skip IMR boot on ACE platforms in case of S3 suspend
by Sasha Levin 15 Apr '24

15 Apr '24

From: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> [ Upstream commit c61115b37ff964d63191dbf4a058f481daabdf57 ] SoCs with ACE architecture are tailored to use s2idle instead deep (S3) suspend state and the IMR content is lost when the system is forced to enter even to S3. When waking up from S3 state the IMR boot will fail as the content is lost. Set the skip_imr_boot flag to make sure that we don't try IMR in this case. Signed-off-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Rander Wang <rander.wang(a)intel.com> Reviewed-by: Liam Girdwood <liam.r.girdwood(a)intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> Link: https://msgid.link/r/20240322112504.4192-1-peter.ujfalusi@linux.intel.com Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/intel/hda-dsp.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 2445ae7f6b2e9..1506982a56c30 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -681,17 +681,27 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend) struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; const struct sof_intel_dsp_desc *chip = hda->desc; struct hdac_bus *bus = sof_to_bus(sdev); + bool imr_lost = false; int ret, j; /* - * The memory used for IMR boot loses its content in deeper than S3 state - * We must not try IMR boot on next power up (as it will fail). - * + * The memory used for IMR boot loses its content in deeper than S3 + * state on CAVS platforms. + * On ACE platforms due to the system architecture the IMR content is + * lost at S3 state already, they are tailored for s2idle use. + * We must not try IMR boot on next power up in these cases as it will + * fail. + */ + if (sdev->system_suspend_target > SOF_SUSPEND_S3 || + (chip->hw_ip_version >= SOF_INTEL_ACE_1_0 && + sdev->system_suspend_target == SOF_SUSPEND_S3)) + imr_lost = true; + + /* * In case of firmware crash or boot failure set the skip_imr_boot to true * as well in order to try to re-load the firmware to do a 'cold' boot. */ - if (sdev->system_suspend_target > SOF_SUSPEND_S3 || - sdev->fw_state == SOF_FW_CRASHED || + if (imr_lost || sdev->fw_state == SOF_FW_CRASHED || sdev->fw_state == SOF_FW_BOOT_FAILED) hda->skip_imr_boot = true; -- 2.43.0

1 0

Re: [PATCH] ASoC: SOF: Clean up sof_ipc_flood_dfs_write()
by Mark Brown 04 Apr '24

04 Apr '24

On Thu, 04 Apr 2024 10:34:53 +0300, Dan Carpenter wrote: > This function doesn't support partial writes so using > simple_write_to_buffer() doesn't really make sense. It's better to > just use copy_from_user(). > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: Clean up sof_ipc_flood_dfs_write() commit: 2cfa0eea2675ce569dc80d2c0d2d644a452b77b3 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] ASoC: SOF: Disable pointless writes to debugfs file
by Mark Brown 04 Apr '24

04 Apr '24

On Thu, 04 Apr 2024 10:38:45 +0300, Dan Carpenter wrote: > The permissions on this debugfs file are 0444 so it can't be written to. > And writing to the file hasn't done anything since commit 6e9548cdb30e > ("ASoC: SOF: Convert the generic IPC flood test into SOF client"). > Delete the write function. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: Disable pointless writes to debugfs file commit: 84ae7d9cfa5f6ec284efccedcb7baf7c075774d4 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] ASoC: SOF: amd: fix for false dsp interrupts
by Mark Brown 04 Apr '24

04 Apr '24

On Thu, 04 Apr 2024 09:47:13 +0530, Vijendar Mukunda wrote: > Before ACP firmware loading, DSP interrupts are not expected. > Sometimes after reboot, it's observed that before ACP firmware is loaded > false DSP interrupt is reported. > Registering the interrupt handler before acp initialization causing false > interrupts sometimes on reboot as ACP reset is not applied. > Correct the sequence by invoking acp initialization sequence prior to > registering interrupt handler. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: amd: fix for false dsp interrupts commit: b9846a386734e73a1414950ebfd50f04919f5e24 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