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

[Julien Grall <julien@xxxxxxx>]

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> ---

>          *      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