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.hildebr...@amd.com>
>>>> ---
>>>> 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?
