Re: [XEN PATCH v3 3/3] xen/mm: add declaration for first_valid_mfn

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2023 03:05, Stefano Stabellini wrote:
> On Wed, 13 Dec 2023, Jan Beulich wrote:
>> On 11.12.2023 10:14, Nicola Vetrini wrote:
>>> --- a/xen/arch/arm/include/asm/numa.h
>>> +++ b/xen/arch/arm/include/asm/numa.h
>>> @@ -2,8 +2,9 @@
>>>  #define __ARCH_ARM_NUMA_H
>>>  
>>>  #include <xen/mm.h>
>>
>> With this, ...
>>
>>> +#include <xen/types.h>
>>>  
>>> -typedef u8 nodeid_t;
>>> +typedef uint8_t nodeid_t;
>>>  
>>>  #ifndef CONFIG_NUMA
>>>  
>>> @@ -12,10 +13,9 @@ typedef u8 nodeid_t;
>>>  #define node_to_cpumask(node)   (cpu_online_map)
>>>  
>>>  /*
>>> - * TODO: make first_valid_mfn static when NUMA is supported on Arm, this
>>> - * is required because the dummy helpers are using it.
>>> + * TODO: define here first_valid_mfn as static when NUMA is supported on 
>>> Arm,
>>> + * this is required because the dummy helpers are using it.
>>>   */
>>> -extern mfn_t first_valid_mfn;
>>
>> ... and this declaration moved to xen/mm.h, how is it going to be possible
>> to do as the adjusted comment says? The compiler will choke on the static
>> after having seen the extern.
> 
> Nicola was just following a review comment by Julien. NUMA has been
> pending for a while and I wouldn't hold this patch back because of it.
> I suggest we go with Julien's request (this version of the patch).
> 
> If you feel strongly about it, please suggest an alternative.

Leaving a TODO comment which cannot actually be carried out is just wrong
imo. And I consider in unfair to ask me to suggest an alternative. The
(imo obvious) alternative is to drop this patch, if no proper change can
be proposed. There's nothing wrong with leaving a violation in place,
when that violation is far from causing any kind of harm. The more that
the place is already accompanied by a (suitable afaict) comment.

Jan