Re: [PATCH v10] module: replace module_layout with module_memory

[Song Liu <songliubraving@meta.com>]

> On Feb 8, 2023, at 10:37 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> Le 09/02/2023 à 01:16, Song Liu a écrit :
>> 
>> 
>>> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>> 
>> [...]
>> 
>>>>  {
>>>>   unsigned long min = (unsigned long)base;
>>>>   unsigned long max = min + size;
>>>> 
>>>> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>>> 
>>> A #ifdef shouldn't be required. You can use IS_ENABLED() instead:
>>> 
>>> 
>>> 
>>>> + if (mod_mem_type_is_core_data(type)) {
>>> 
>>> if (IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
>>>    mod_mem_type_is_core_data(type))
>>> 
>>>> + if (min < tree->data_addr_min)
>>>> + tree->data_addr_min = min;
>>>> + if (max > tree->data_addr_max)
>>>> + tree->data_addr_max = max;
>>>> + return;
>>>> + }
>>>> +#endif
>> 
>> To use IS_ENABLED() here, we also need to keep data_addr_[min|max]
>> around. Do we really want them?
> 
> It is up to you. If you think it is not worth the effort, it's fine for me.
> 
> Allthough it could probably be easily fixed by doing (untested) :
> 
> struct mod_tree_root {
> #ifdef CONFIG_MODULES_TREE_LOOKUP
> struct latch_tree_root root;
> #endif
> #ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> unsigned long addr_min;
> unsigned long addr_max;
> unsigned long data_addr_min;
> unsigned long data_addr_max;
> #else
> union {
> unsigned long addr_min;
> unsigned long data_addr_min;
> };
> union {
> unsigned long addr_max;
> unsigned long data_addr_max;
> }:
> #endif
> };

I think using union here will be an overkill. Let's just keep it 
simple with #ifdef. 

Thanks,
Song