Re: [PATCH 00/14] ASoC: Constify local snd_sof_dsp_ops

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

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