In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few occurrences
of nested '//' character sequences inside C-style comment blocks, which violate
Rule 3.1. The patch aims to resolve those by removing the nested comments.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com
Changes:
- Resending the patch with the right maintainers in CC.
Changes in V2:
- Split the patch into a series and reworked the fix.
- Apply the fix to the arm32 `flushtlb.h' file, for consistency
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/drivers/passthrough/arm/smmu-v3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 720aa69ff2..f410863e10 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1047,10 +1047,10 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
* before we read 'nr_ats_masters' in case of a concurrent call to
* arm_smmu_enable_ats():
*
- * // unmap() // arm_smmu_enable_ats()
+ * unmap() arm_smmu_enable_ats()
* TLBI+SYNC atomic_inc(&nr_ats_masters);
* smp_mb(); [...]
- * atomic_read(&nr_ats_masters); pci_enable_ats() // writel()
+ * atomic_read(&nr_ats_masters); pci_enable_ats() (i.e. writel())
*
* Ensures that we always see the incremented 'nr_ats_masters' count if
* ATS was enabled at the PCI device before completion of the TLBI.
--
2.34.1Hi, On 19/06/2023 10:56, Nicola Vetrini wrote: > In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few occurrences > of nested '//' character sequences inside C-style comment blocks, which violate > Rule 3.1. The patch aims to resolve those by removing the nested comments. I think it is important to understand/explain what was the intention of the // before removing them because, IMHO, the new approach doesn't convey the same information. Before... > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com > Changes: > - Resending the patch with the right maintainers in CC. > Changes in V2: > - Split the patch into a series and reworked the fix. > - Apply the fix to the arm32 `flushtlb.h' file, for consistency > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/drivers/passthrough/arm/smmu-v3.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c > index 720aa69ff2..f410863e10 100644 > --- a/xen/drivers/passthrough/arm/smmu-v3.c > +++ b/xen/drivers/passthrough/arm/smmu-v3.c > @@ -1047,10 +1047,10 @@ static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, > * before we read 'nr_ats_masters' in case of a concurrent call to > * arm_smmu_enable_ats(): > * > - * // unmap() // arm_smmu_enable_ats() > + * unmap() arm_smmu_enable_ats() ... with the // it would be clearer that the code below belongs to each function. But now, it leads to think there are a call to 'unmap' which it then followed by TLBI+SYNC. In this case, I would propose to use --- <function> --- > * TLBI+SYNC atomic_inc(&nr_ats_masters); > * smp_mb(); [...] > - * atomic_read(&nr_ats_masters); pci_enable_ats() // writel() > + * atomic_read(&nr_ats_masters); pci_enable_ats() (i.e. writel()) NIT: I think 'see' would be better than 'i.e.' because I read it as pci_enable_ats() is a simple writel(). > * > * Ensures that we always see the incremented 'nr_ats_masters' count if > * ATS was enabled at the PCI device before completion of the TLBI. Cheers, -- Julien Grall
On 19/06/23 12:10, Julien Grall wrote: > Hi, > > On 19/06/2023 10:56, Nicola Vetrini wrote: >> In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few >> occurrences >> of nested '//' character sequences inside C-style comment blocks, >> which violate >> Rule 3.1. The patch aims to resolve those by removing the nested >> comments. > > I think it is important to understand/explain what was the intention of > the // before removing them because, IMHO, the new approach doesn't > convey the same information. Before... > >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com >> Changes: >> - Resending the patch with the right maintainers in CC. >> Changes in V2: >> - Split the patch into a series and reworked the fix. >> - Apply the fix to the arm32 `flushtlb.h' file, for consistency >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/drivers/passthrough/arm/smmu-v3.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c >> b/xen/drivers/passthrough/arm/smmu-v3.c >> index 720aa69ff2..f410863e10 100644 >> --- a/xen/drivers/passthrough/arm/smmu-v3.c >> +++ b/xen/drivers/passthrough/arm/smmu-v3.c >> @@ -1047,10 +1047,10 @@ static int arm_smmu_atc_inv_domain(struct >> arm_smmu_domain *smmu_domain, >> * before we read 'nr_ats_masters' in case of a concurrent call to >> * arm_smmu_enable_ats(): >> * >> - * // unmap() // arm_smmu_enable_ats() >> + * unmap() arm_smmu_enable_ats() > > ... with the // it would be clearer that the code below belongs to each > function. But now, it leads to think there are a call to 'unmap' which > it then followed by TLBI+SYNC. > > In this case, I would propose to use --- <function> --- It seems a good suggestion to me. > >> * TLBI+SYNC atomic_inc(&nr_ats_masters); >> * smp_mb(); [...] >> - * atomic_read(&nr_ats_masters); pci_enable_ats() // writel() >> + * atomic_read(&nr_ats_masters); pci_enable_ats() (i.e. >> writel()) > > NIT: I think 'see' would be better than 'i.e.' because I read it as > pci_enable_ats() is a simple writel(). Ok. > >> * >> * Ensures that we always see the incremented 'nr_ats_masters' >> count if >> * ATS was enabled at the PCI device before completion of the TLBI. > > Cheers, > Regards, -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
© 2016 - 2026 Red Hat, Inc.