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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall