Hi,
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
---
Krzysztof Kozlowski (14):
ASoC: SOF: debug: Constify local snd_sof_dsp_ops
ASoC: SOF: ipc3: Constify local snd_sof_dsp_ops
ASoC: SOF: pcm: Constify local snd_sof_dsp_ops
ASoC: SOF: Constify stored pointer to snd_sof_dsp_ops
ASoC: SOF: intel: pci-tng: Constify snd_sof_dsp_ops
ASoC: SOF: intel: hda: Constify snd_sof_dsp_ops
ASoC: SOF: amd: acp: Constify snd_sof_dsp_ops
ASoC: SOF: imx8: Constify snd_sof_dsp_ops
ASoC: SOF: imx8m: Constify snd_sof_dsp_ops
ASoC: SOF: imx8ulp: Constify snd_sof_dsp_ops
ASoC: SOF: intel: bdw: Constify snd_sof_dsp_ops
ASoC: SOF: intel: byt: Constify snd_sof_dsp_ops
ASoC: SOF: mediatek: mt8186: Constify snd_sof_dsp_ops
ASoC: SOF: mediatek: mt8195: Constify snd_sof_dsp_ops
include/sound/sof.h | 2 +-
sound/soc/sof/amd/acp-common.c | 2 +-
sound/soc/sof/amd/acp.h | 2 +-
sound/soc/sof/debug.c | 2 +-
sound/soc/sof/imx/imx8.c | 4 ++--
sound/soc/sof/imx/imx8m.c | 2 +-
sound/soc/sof/imx/imx8ulp.c | 2 +-
sound/soc/sof/intel/bdw.c | 2 +-
sound/soc/sof/intel/byt.c | 4 ++--
sound/soc/sof/intel/hda-common-ops.c | 2 +-
sound/soc/sof/intel/hda.h | 2 +-
sound/soc/sof/intel/pci-tng.c | 2 +-
sound/soc/sof/intel/shim.h | 2 +-
sound/soc/sof/ipc3-priv.h | 6 +++---
sound/soc/sof/mediatek/mt8186/mt8186.c | 2 +-
sound/soc/sof/mediatek/mt8195/mt8195.c | 2 +-
sound/soc/sof/pcm.c | 2 +-
17 files changed, 21 insertions(+), 21 deletions(-)
---
base-commit: 9aa527ea9d7e33323f6a5c793eb62b09b7f96d46
change-id: 20240414-n-const-ops-var-136f395b374b
Best regards,
--
Krzysztof Kozlowski <[email protected]>
Constify the pointer to 'struct snd_sof_dsp_ops' to annotate that
functioon does not modify pointed data.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
sound/soc/sof/debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
index cf282c8b18af..884d85770426 100644
--- a/sound/soc/sof/debug.c
+++ b/sound/soc/sof/debug.c
@@ -311,8 +311,8 @@ EXPORT_SYMBOL_GPL(snd_sof_dbg_memory_info_init);
int snd_sof_dbg_init(struct snd_sof_dev *sdev)
{
+ const struct snd_sof_dsp_ops *ops = sof_ops(sdev);
struct snd_sof_pdata *plat_data = sdev->pdata;
- struct snd_sof_dsp_ops *ops = sof_ops(sdev);
const struct snd_sof_debugfs_map *map;
struct dentry *fw_profile;
int i;
--
2.34.1
Constify the pointer to 'struct snd_sof_dsp_ops' to annotate that
functioon does not modify pointed data.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
sound/soc/sof/ipc3-priv.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sof/ipc3-priv.h b/sound/soc/sof/ipc3-priv.h
index 0bbca418e67e..9cd7794f3cb6 100644
--- a/sound/soc/sof/ipc3-priv.h
+++ b/sound/soc/sof/ipc3-priv.h
@@ -36,7 +36,7 @@ static inline int sof_dtrace_host_init(struct snd_sof_dev *sdev,
struct snd_dma_buffer *dmatb,
struct sof_ipc_dma_trace_params_ext *dtrace_params)
{
- struct snd_sof_dsp_ops *dsp_ops = sdev->pdata->desc->ops;
+ const struct snd_sof_dsp_ops *dsp_ops = sdev->pdata->desc->ops;
if (dsp_ops->trace_init)
return dsp_ops->trace_init(sdev, dmatb, dtrace_params);
@@ -46,7 +46,7 @@ static inline int sof_dtrace_host_init(struct snd_sof_dev *sdev,
static inline int sof_dtrace_host_release(struct snd_sof_dev *sdev)
{
- struct snd_sof_dsp_ops *dsp_ops = sdev->pdata->desc->ops;
+ const struct snd_sof_dsp_ops *dsp_ops = sdev->pdata->desc->ops;
if (dsp_ops->trace_release)
return dsp_ops->trace_release(sdev);
@@ -56,7 +56,7 @@ static inline int sof_dtrace_host_release(struct snd_sof_dev *sdev)
static inline int sof_dtrace_host_trigger(struct snd_sof_dev *sdev, int cmd)
{
- struct snd_sof_dsp_ops *dsp_ops = sdev->pdata->desc->ops;
+ const struct snd_sof_dsp_ops *dsp_ops = sdev->pdata->desc->ops;
if (dsp_ops->trace_trigger)
return dsp_ops->trace_trigger(sdev, cmd);
--
2.34.1
Constify the pointer to 'struct snd_sof_dsp_ops' to annotate that
functioon does not modify pointed data.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
sound/soc/sof/pcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 8fca4b0d63de..cb774e011e31 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -423,7 +423,7 @@ static int sof_pcm_open(struct snd_soc_component *component,
struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
struct snd_pcm_runtime *runtime = substream->runtime;
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
- struct snd_sof_dsp_ops *ops = sof_ops(sdev);
+ const struct snd_sof_dsp_ops *ops = sof_ops(sdev);
struct snd_sof_pcm *spcm;
struct snd_soc_tplg_stream_caps *caps;
int ret;
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/intel/pci-tng.c | 2 +-
sound/soc/sof/intel/shim.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/pci-tng.c b/sound/soc/sof/intel/pci-tng.c
index c90173003c2b..d8a36d5a29f7 100644
--- a/sound/soc/sof/intel/pci-tng.c
+++ b/sound/soc/sof/intel/pci-tng.c
@@ -132,7 +132,7 @@ static int tangier_pci_probe(struct snd_sof_dev *sdev)
return ret;
}
-struct snd_sof_dsp_ops sof_tng_ops = {
+const struct snd_sof_dsp_ops sof_tng_ops = {
/* device init */
.probe = tangier_pci_probe,
diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
index 9515d753c816..18ce3d33fa80 100644
--- a/sound/soc/sof/intel/shim.h
+++ b/sound/soc/sof/intel/shim.h
@@ -196,7 +196,7 @@ struct sof_intel_dsp_desc {
int (*cl_init)(struct snd_sof_dev *sdev, int stream_tag, bool imr_boot);
};
-extern struct snd_sof_dsp_ops sof_tng_ops;
+extern const struct snd_sof_dsp_ops sof_tng_ops;
extern const struct sof_intel_dsp_desc tng_chip_info;
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/intel/hda-common-ops.c | 2 +-
sound/soc/sof/intel/hda.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c
index d71bb66b9991..466769c68da5 100644
--- a/sound/soc/sof/intel/hda-common-ops.c
+++ b/sound/soc/sof/intel/hda-common-ops.c
@@ -14,7 +14,7 @@
#include "hda.h"
#include "../sof-audio.h"
-struct snd_sof_dsp_ops sof_hda_common_ops = {
+const struct snd_sof_dsp_ops sof_hda_common_ops = {
/* probe/remove/shutdown */
.probe_early = hda_dsp_probe_early,
.probe = hda_dsp_probe,
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index b59d1a572bce..7804c5b50c26 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -874,7 +874,7 @@ int hda_dsp_dais_suspend(struct snd_sof_dev *sdev);
/*
* Platform Specific HW abstraction Ops.
*/
-extern struct snd_sof_dsp_ops sof_hda_common_ops;
+extern const struct snd_sof_dsp_ops sof_hda_common_ops;
extern struct snd_sof_dsp_ops sof_skl_ops;
int sof_skl_ops_init(struct snd_sof_dev *sdev);
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/amd/acp-common.c | 2 +-
sound/soc/sof/amd/acp.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/amd/acp-common.c b/sound/soc/sof/amd/acp-common.c
index 0fc4e20ec673..b26fa471b431 100644
--- a/sound/soc/sof/amd/acp-common.c
+++ b/sound/soc/sof/amd/acp-common.c
@@ -193,7 +193,7 @@ struct snd_soc_acpi_mach *amd_sof_machine_select(struct snd_sof_dev *sdev)
}
/* AMD Common DSP ops */
-struct snd_sof_dsp_ops sof_acp_common_ops = {
+const struct snd_sof_dsp_ops sof_acp_common_ops = {
/* probe and remove */
.probe = amd_sof_acp_probe,
.remove = amd_sof_acp_remove,
diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h
index e229bb6b849d..87e79d500865 100644
--- a/sound/soc/sof/amd/acp.h
+++ b/sound/soc/sof/amd/acp.h
@@ -310,7 +310,7 @@ int acp_pcm_hw_params(struct snd_sof_dev *sdev, struct snd_pcm_substream *substr
snd_pcm_uframes_t acp_pcm_pointer(struct snd_sof_dev *sdev,
struct snd_pcm_substream *substream);
-extern struct snd_sof_dsp_ops sof_acp_common_ops;
+extern const struct snd_sof_dsp_ops sof_acp_common_ops;
extern struct snd_sof_dsp_ops sof_renoir_ops;
int sof_renoir_ops_init(struct snd_sof_dev *sdev);
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/imx/imx8.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/imx/imx8.c b/sound/soc/sof/imx/imx8.c
index d65e09a927f6..3021dc87ab5a 100644
--- a/sound/soc/sof/imx/imx8.c
+++ b/sound/soc/sof/imx/imx8.c
@@ -478,7 +478,7 @@ static int imx8_dsp_set_power_state(struct snd_sof_dev *sdev,
}
/* i.MX8 ops */
-static struct snd_sof_dsp_ops sof_imx8_ops = {
+static const struct snd_sof_dsp_ops sof_imx8_ops = {
/* probe and remove */
.probe = imx8_probe,
.remove = imx8_remove,
@@ -539,7 +539,7 @@ static struct snd_sof_dsp_ops sof_imx8_ops = {
};
/* i.MX8X ops */
-static struct snd_sof_dsp_ops sof_imx8x_ops = {
+static const struct snd_sof_dsp_ops sof_imx8x_ops = {
/* probe and remove */
.probe = imx8_probe,
.remove = imx8_remove,
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/imx/imx8m.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/imx/imx8m.c b/sound/soc/sof/imx/imx8m.c
index 3e0b88002b4f..4ed415f04345 100644
--- a/sound/soc/sof/imx/imx8m.c
+++ b/sound/soc/sof/imx/imx8m.c
@@ -411,7 +411,7 @@ static int imx8m_dsp_suspend(struct snd_sof_dev *sdev, unsigned int target_state
}
/* i.MX8 ops */
-static struct snd_sof_dsp_ops sof_imx8m_ops = {
+static const struct snd_sof_dsp_ops sof_imx8m_ops = {
/* probe and remove */
.probe = imx8m_probe,
.remove = imx8m_remove,
--
2.34.1
The core code does not modify the 'struct snd_sof_dsp_ops', stored in
'struct sof_dev_desc'. Make the pointer to 'struct snd_sof_dsp_ops'
const to annotate this, make code a bit safer and allow individual
drivers to also define this structure as const.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
include/sound/sof.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/sound/sof.h b/include/sound/sof.h
index 05213bb515a3..780c1a755917 100644
--- a/include/sound/sof.h
+++ b/include/sound/sof.h
@@ -166,7 +166,7 @@ struct sof_dev_desc {
/* default firmware name */
const char *default_fw_filename[SOF_IPC_TYPE_COUNT];
- struct snd_sof_dsp_ops *ops;
+ const struct snd_sof_dsp_ops *ops;
int (*ops_init)(struct snd_sof_dev *sdev);
void (*ops_free)(struct snd_sof_dev *sdev);
};
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/imx/imx8ulp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/imx/imx8ulp.c b/sound/soc/sof/imx/imx8ulp.c
index 296fbf11f6d0..8adfdd00413a 100644
--- a/sound/soc/sof/imx/imx8ulp.c
+++ b/sound/soc/sof/imx/imx8ulp.c
@@ -408,7 +408,7 @@ static int imx8ulp_dsp_set_power_state(struct snd_sof_dev *sdev,
}
/* i.MX8 ops */
-static struct snd_sof_dsp_ops sof_imx8ulp_ops = {
+static const struct snd_sof_dsp_ops sof_imx8ulp_ops = {
/* probe and remove */
.probe = imx8ulp_probe,
.remove = imx8ulp_remove,
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/intel/bdw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c
index e30ca086f3f8..2e8926cd00f6 100644
--- a/sound/soc/sof/intel/bdw.c
+++ b/sound/soc/sof/intel/bdw.c
@@ -567,7 +567,7 @@ static struct snd_soc_dai_driver bdw_dai[] = {
};
/* broadwell ops */
-static struct snd_sof_dsp_ops sof_bdw_ops = {
+static const struct snd_sof_dsp_ops sof_bdw_ops = {
/*Device init */
.probe = bdw_probe,
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/intel/byt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c
index 373527b206d7..14a065999a89 100644
--- a/sound/soc/sof/intel/byt.c
+++ b/sound/soc/sof/intel/byt.c
@@ -214,7 +214,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev)
}
/* baytrail ops */
-static struct snd_sof_dsp_ops sof_byt_ops = {
+static const struct snd_sof_dsp_ops sof_byt_ops = {
/* device init */
.probe = byt_acpi_probe,
.remove = byt_remove,
@@ -289,7 +289,7 @@ static const struct sof_intel_dsp_desc byt_chip_info = {
};
/* cherrytrail and braswell ops */
-static struct snd_sof_dsp_ops sof_cht_ops = {
+static const struct snd_sof_dsp_ops sof_cht_ops = {
/* device init */
.probe = byt_acpi_probe,
.remove = byt_remove,
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/mediatek/mt8186/mt8186.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/mediatek/mt8186/mt8186.c b/sound/soc/sof/mediatek/mt8186/mt8186.c
index 0d2d7d697de0..c63e0d2f4b96 100644
--- a/sound/soc/sof/mediatek/mt8186/mt8186.c
+++ b/sound/soc/sof/mediatek/mt8186/mt8186.c
@@ -481,7 +481,7 @@ static struct snd_soc_dai_driver mt8186_dai[] = {
};
/* mt8186 ops */
-static struct snd_sof_dsp_ops sof_mt8186_ops = {
+static const struct snd_sof_dsp_ops sof_mt8186_ops = {
/* probe and remove */
.probe = mt8186_dsp_probe,
.remove = mt8186_dsp_remove,
--
2.34.1
'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 <[email protected]>
---
sound/soc/sof/mediatek/mt8195/mt8195.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/mediatek/mt8195/mt8195.c b/sound/soc/sof/mediatek/mt8195/mt8195.c
index 8ee7ee246344..fc1c016104ae 100644
--- a/sound/soc/sof/mediatek/mt8195/mt8195.c
+++ b/sound/soc/sof/mediatek/mt8195/mt8195.c
@@ -505,7 +505,7 @@ static struct snd_soc_dai_driver mt8195_dai[] = {
};
/* mt8195 ops */
-static struct snd_sof_dsp_ops sof_mt8195_ops = {
+static const struct snd_sof_dsp_ops sof_mt8195_ops = {
/* probe and remove */
.probe = mt8195_dsp_probe,
.remove = mt8195_dsp_remove,
--
2.34.1
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 <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
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 <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> 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.
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.
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.
Best regards,
Krzysztof
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.
On 22/04/2024 17:52, Pierre-Louis Bossart wrote:
>
>
> 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.
What do you mean? Entire point of this patchset is for static and global
data. Which patches are you reviewing?
>
>> 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.
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.
>
> 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.
>
> 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.
Best regards,
Krzysztof
> 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;
On 22/04/2024 22:42, Pierre-Louis Bossart wrote:
>
>> 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?
>
>>> 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.
> 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.
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?
>
> <<< THE REST ISN'T.
>
> /* shutdown */
> sof_mtl_ops.shutdown = hda_dsp_shutdown;
Best regards,
Krzysztof
>>> 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.
On 22/04/2024 23:24, Pierre-Louis Bossart wrote:
>
>
>>>> 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.
I meant, drivers using such core library functions or directly calling
to the core, pass some private data structure with "struct foo_ops".
Sometimes pass directly "struct foo_ops". This is happening everywhere
and is common pattern.
And everywhere we try to make it const, where following conditions are met:
1. the driver allocates "struct foo_ops" statically,
2. the driver does not change it,
3. the core (or library) does not change it.
>
>>>>> 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.
The same as everywhere else. The distinction, that this is "library" and
in other cases often is "core framework" or "subsystem", does not
matter. Behaves the same.
>
>>> 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.
The "core" or the "library" either modifies the structure or not. That
is only that matters from the core point of view.
>
>> 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.
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.
Just like everywhere else.
I keep repeating the same arguments and keep repeating the same: please
bring me any argument why this should be different than all other 100
structs (or more, I count based on checkpatch). So far you did not bring
any argument for this, so I don't know how to provide any other
technical feedback.
According to my knowledge and easily visible here, this is exactly the
same as in all other cases. Drivers have some static or global struct
which can be moved to rodata, because nothing modifies it.
Best regards,
Krzysztof
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.
Can we discuss specific patches in this patchset? Which constifying of
static or global data is not correct, is misleading or is not beneficial
at all?
Best regards,
Krzysztof
> 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.
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 <[email protected]>
[1] https://github.com/thesofproject/linux/pull/4949
On Sun, Apr 14, 2024 at 08:47:26PM +0200, Krzysztof Kozlowski wrote:
> Constify the pointer to 'struct snd_sof_dsp_ops' to annotate that
> functioon does not modify pointed data.
This doesn't apply against current code, please check and resend.
On 26/04/2024 04:09, Mark Brown wrote:
> On Sun, Apr 14, 2024 at 08:47:26PM +0200, Krzysztof Kozlowski wrote:
>> Constify the pointer to 'struct snd_sof_dsp_ops' to annotate that
>> functioon does not modify pointed data.
>
> This doesn't apply against current code, please check and resend.
Maybe I am basing on wrong tree. I applied now on for-next of your
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/ and
b4 did not complain - no hunk rejects.
I will resend based on that tree, but I expect same result. Let me know
if I should use different tree.
Best regards,
Krzysztof
On 26/04/2024 09:58, Krzysztof Kozlowski wrote:
> On 26/04/2024 04:09, Mark Brown wrote:
>> On Sun, Apr 14, 2024 at 08:47:26PM +0200, Krzysztof Kozlowski wrote:
>>> Constify the pointer to 'struct snd_sof_dsp_ops' to annotate that
>>> functioon does not modify pointed data.
>>
>> This doesn't apply against current code, please check and resend.
>
> Maybe I am basing on wrong tree. I applied now on for-next of your
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/ and
> b4 did not complain - no hunk rejects.
>
> I will resend based on that tree, but I expect same result. Let me know
> if I should use different tree.
Never mind, my bad, the base commit was not public, but a commit from my
development tree, so b4 properly figured out any conflicts transparently
in my case.
Best regards,
Krzysztof
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