| Message ID | 20221213181855.81713-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/arm: smmuv3: mark arm_smmu_disable_pasid __maybe_unused | expand |
Hi Stewart, > On 13 Dec 2022, at 6:18 pm, Stewart Hildebrand <stewart.hildebrand@amd.com> 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@amd.com> Reviewed-by: Rahul Singh <rahul.singh@arm.com> Regards, Rahul
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@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(...) > { > struct pci_dev *pdev; > @@ -1405,6 +1406,7 @@ static inline int arm_smmu_enable_pasid(struct arm_smmu_master *master) > return 0; > } > > +__maybe_unused > static inline void arm_smmu_disable_pasid(struct arm_smmu_master *master) { } Same here. > #endif /* CONFIG_PCI_ATS */ > Cheers,
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@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): 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.
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@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. Cheers,
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@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?
Hi Stewart, > On 15 Dec 2022, at 3:10 pm, Stewart Hildebrand <stewart.hildebrand@amd.com> 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@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? Yes you can retain my R-b. Regards, Rahul
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) { struct pci_dev *pdev; @@ -1405,6 +1406,7 @@ static inline int arm_smmu_enable_pasid(struct arm_smmu_master *master) return 0; } +__maybe_unused static inline void arm_smmu_disable_pasid(struct arm_smmu_master *master) { } #endif /* CONFIG_PCI_ATS */
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@amd.com> --- v1->v2: Add __maybe_unused attribute instead of removing --- xen/drivers/passthrough/arm/smmu-v3.c | 2 ++ 1 file changed, 2 insertions(+)