Re: [XEN PATCH v2 2/3] xen/drivers/passthrough/arm/smmu-v3.c: fix violations of MISRA C:2012 Rule 3.1

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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@xxxxxxxxxxx
> > 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@xxxxxxxxxxx>
> > ---
> >   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)