Re: [PATCH v2] xen/arm: smmuv3: mark arm_smmu_disable_pasid __maybe_unused

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 12/15/22 06:34, Julien Grall wrote:
> Hi Stewart,
> 
> I was about to commit this patch when I noticed the placement of the
> attribute doesn't match what we are usually doing in Xen.
> 
> On 13/12/2022 18:18, Stewart Hildebrand wrote:
>> When building with clang 12 and CONFIG_ARM_SMMU_V3=y, we observe the
>> following build error:
>>
>> drivers/passthrough/arm/smmu-v3.c:1408:20: error: unused function 
>> 'arm_smmu_disable_pasid' [-Werror,-Wunused-function]
>> static inline void arm_smmu_disable_pasid(struct arm_smmu_master *master) { }
>>                     ^
>>
>> arm_smmu_disable_pasid is not currently called from anywhere in Xen, but
>> it is inside a section of code guarded by CONFIG_PCI_ATS, which may be
>> helpful in the future if the PASID feature is to be implemented. Add the
>> attribute __maybe_unused to the function.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> v1->v2:
>> Add __maybe_unused attribute instead of removing
>> ---
>>   xen/drivers/passthrough/arm/smmu-v3.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index 9c9f4630090e..0cdc862f96d1 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -1376,6 +1376,7 @@ static int arm_smmu_enable_pasid(struct 
>> arm_smmu_master *master)
>>       return 0;
>>   }
>>
>> +__maybe_unused
>>   static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> 
> The attribute should be placed after "void". I.e.:
> 
> static void __maybe_unused arm_smmu_disable_pasid(...)

I had initially tried placing it exactly where you suggest in the first draft 
of v2 of this patch. However, the line would then exceed 72 characters (actual 
81 characters):

static void __maybe_unused arm_smmu_disable_pasid(struct arm_smmu_master 
*master)

So I found myself juggling with how best to wrap it. How about a newline after 
the __maybe_unused attribute?

static void __maybe_unused
arm_smmu_disable_pasid(struct arm_smmu_master *master)

and similarly for the 2nd occurrence:

static inline void __maybe_unused
arm_smmu_disable_pasid(struct arm_smmu_master *master) { }

There is precedent for this style of wrapping in xen/common/sched/credit2.c.