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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stewart,

> On 15 Dec 2022, at 3:10 pm, Stewart Hildebrand <stewart.hildebrand@xxxxxxx> 
> wrote:
> 
> On 12/15/22 09:51, Julien Grall wrote:
>> Hi Stewart,
>> 
>> On 15/12/2022 14:11, Stewart Hildebrand wrote:
>>> 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):
>> 
>> This doesn't change the problem here but the limit is 80 characters per
>> line rather than 72.
>> 
>>> 
>>> 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.
>> 
>> Ah! I didn't realize the line would have been too long. In this case,
>> the newline after __maybe_unused is the way to go.
> 
> Ok, I will send a v3 with this change.
> 
> Rahul - may I retain your R-b tag in v3?

Yes you can retain my R-b.

Regards,
Rahul