On 03.02.2025 17:24, Oleksii Kurochko wrote:
> On 2/3/25 5:03 PM, Jan Beulich wrote:
>> On 03.02.2025 16:05, Oleksii Kurochko wrote:
>>> On 1/27/25 3:47 PM, Jan Beulich wrote:
>>>>> +static bool is_lowercase_extension_name(const char *str)
>>>>> +{
>>>>> + /*
>>>>> + * `str` could contain full riscv,isa string from device tree so one
>>>>> + * of the stop condionitions is checking for '_' as extensions are
>>>>> + * separated by '_'.
>>>>> + */
>>>>> + for ( unsigned int i = 0; (str[i] != '\0') && (str[i] != '_'); i++ )
>>>>> + if ( !islower(str[i]) )
>>>>> + return false;
>>>>> +
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> +static void __init match_isa_ext(const char *name, const char *name_end,
>>>>> + unsigned long *bitmap)
>>>>> +{
>>>>> + const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
>>>>> +
>>>>> + for ( unsigned int i = 0; i < riscv_isa_ext_count; i++ )
>>>>> + {
>>>>> + const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
>>>>> +
>>>>> + /*
>>>>> + * `name` (according to device tree binding) and
>>>>> + * `ext->name` (according to initialization of riscv_isa_ext[]
>>>>> + * elements) must be all in lowercase.
>>>>> + *
>>>>> + * Just to be sure that it is true, ASSERT() is added.
>>>>> + */
>>>>> + ASSERT(is_lowercase_extension_name(name) &&
>>>>> + is_lowercase_extension_name(ext->name));
>>>> More general remark: While asserting on ext->name is okay, for it being
>>>> our own data, asserting on data coming from the outside is generally not
>>>> correct. For now I'm not going to insist on this being changed, but
>>>> sooner or later it will want revisiting
>>> IIUC it would be better to leave
>>> ASSERT(is_lowercase_extension_name(ext->name)) in match_isa_ext()
>>> and put ASSERT(is_lowercase_extension_name(ext) in riscv_isa_parse_string()
>>> before match_isa_ext()
>>> is called:
>>> static int __init riscv_isa_parse_string(const char *isa,
>>> unsigned long *out_bitmap)
>>> {
>>> ...
>>> while ( *isa )
>>> {
>>> const char *ext = isa++;
>>> ...
>>> ASSERT(is_lowercase_extension_name(ext));
>>> match_isa_ext(ext, ext_end, out_bitmap);
>>> }
>>>
>>> Is my understanding correct?
>> That depends on the origin of the incoming "isa". Considering the function
>> wants to parse it, I'd expect it still comes from DT. In which case
>> asserting on it is wrong; anything may come from there, and nothing should
>> cause assertion failures. Recall that assertions are checks of _our own
>> internal state_ only.
>
> But based on the device tree binding
> (https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/riscv/extensions.yaml#L47
>
> <https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/riscv/extensions.yaml#L47>
> ),
> not anything should come from DT for the riscv,isa string; only lowercase
> letters are allowed.
> I am not sure if it makes sense to double-check if riscv,isa is correct, as
> my expectation (which I haven’t checked yet) is that the DTS will
> be validated during compilation.
>
> Does it make sense to double check what was put in DT's riscv,isa?
I think so. Just not by way of an assertion.
Jan
