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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

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