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

[Julien Grall <julien@xxxxxxx>]

On 18/03/2021 07:21, Michal Orzel wrote:
> 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?
Interesting, I would argue that using PROVIDE is over-engineered (and 
error-prone) when you likely can define _sdtb in .S.
> 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.
I'd like to point out that your first approach didn't replace #ifdef. 
This was fully reviewed and nearly committed. When I pointed out the 
issue, I had to argue that this was broken.
This is not quite the same as adding #ifdef but it shows that such 
mistake can be easily slipped in master.
Cheers,

--
Julien Grall