Re: [PATCH] xen/arm: ffa: fix build with clang

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 1/8/25 11:34, Bertrand Marquis wrote:
> Hi,
> 
>> On 8 Jan 2025, at 16:45, Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx> 
>> wrote:
>>
>> Hi,
>>
>> On Wed Jan 8, 2025 at 3:23 PM GMT, Stewart Hildebrand wrote:
>>> Clang 16 reports:
>>>
>>> In file included from arch/arm/tee/ffa.c:72:
>>> arch/arm/tee/ffa_private.h:329:17: error: 'used' attribute ignored on a 
>>> non-definition declaration [-Werror,-Wignored-attributes]
>>> extern uint32_t __ro_after_init ffa_fw_version;
>>>                ^
>>>
>>> Remove the attribute from the header to resolve this. The attribute in
>>> ffa.c is the one the matters anyway.
>>>
>>> Fixes: 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support")
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>>> ---
>>> It appears the variable ffa_fw_version is only used in ffa.c. Was there
>>> any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa:
>>> Fine granular call support")? If not, perhaps we ought to make it static
>>> again and remove the declaration in the header.
>>
>> The only reason I can think of is wanting to have it in the symbol table of 
>> the
>> ELF file for some reason (livepatching?). But that's far fetched at best.
>>
>>> ---
>>> xen/arch/arm/tee/ffa_private.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h
>>> index d441c0ca5598..05368d5a88d3 100644
>>> --- a/xen/arch/arm/tee/ffa_private.h
>>> +++ b/xen/arch/arm/tee/ffa_private.h
>>> @@ -326,7 +326,7 @@ extern void *ffa_rx;
>>> extern void *ffa_tx;
>>> extern spinlock_t ffa_rx_buffer_lock;
>>> extern spinlock_t ffa_tx_buffer_lock;
>>> -extern uint32_t __ro_after_init ffa_fw_version;
>>> +extern uint32_t ffa_fw_version;
>>> extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE);
>>>
>>> bool ffa_shm_domain_destroy(struct domain *d);
>>>
>>> base-commit: 70f5a875becc9444a959830b10a361982c31a366
>>
>> IMO, it makes sense to make it static and remove this line altogether as you
>> suggest. If it needs to be exported later on it can be adjusted as needed.
>>
> 
> Yes in fact it was made global as there was a use case at some point but this 
> is not
> the case anymore so in fact we can completely remove this from the header and 
> make
> it static for now.
> 
> @stewart: are you happy to push a patch doing this instead ?

Yes