Re: [PATCH v7] arm: Add Kconfig entry to select CONFIG_DTB_FILE

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 16.03.2021 15:54, Julien Grall wrote:
> Hi Michal,
> 
> On 15/03/2021 09:23, Michal Orzel wrote:
>> Currently in order to link existing DTB into Xen image
>> we need to either specify option CONFIG_DTB_FILE on the
>> command line or manually add it into .config.
>> Add Kconfig entry: CONFIG_DTB_FILE
>> to be able to provide the path to DTB we want to embed
>> into Xen image. If no path provided - the dtb will not
>> be embedded.
>>
>> Remove the line: AFLAGS-y += -DCONFIG_DTB_FILE=\"$(CONFIG_DTB_FILE)\"
>> as it is not needed since Kconfig will define it in a header
>> with all the other config options.
>>
>> Make a change in the linker script from:
>> _sdtb = .;
>> to:
>> PROVIDE(_sdtb = .);
>> to avoid creation of _sdtb if there is no reference to it.
> 
> This means that if someone is using #ifdef CONFIG_DTB_FILE rather than 
> .ifnes, _sdtb will get defined.

Do we really want to overengineer something that simple?
Why would someone use #ifdef CONFIG_DTB_FILE?
In my opinion the rule of thumb is that you don't use #ifdef on configs of 
string type.
Using #ifdef CONFIG_DTB_FILE means that someone modifying the code did not even 
spend 1 minute checking the Kconfig.

If you do not agree, I can modify the code so _sdtb will be created in dtb.S.
In that case I would like Jan Beulich to give his opinion before I will send v8.
> 
> The difference between two is quite subttle and can be easily missed during 
> review.
> 
> How about defining _sdtb in dtb.S? With that approach, we would get a 
> compiler error if someone protect _sdtb with #ifdef rather than .ifnes.
> 
> Cheers,
> 

Cheers,
Michal