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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
> 
> In the file `xen/drivers/passthrough/arm/smmu-v3.c' there are a few 
> occurrences

here you use a different character to enclose the file path (` vs ‘) may I 
suggest to
use only (‘)?

> of nested '//' character sequences inside C-style comment blocks, which 
> violate
> Rule 3.1.
> 
> The patch aims to resolve those by replacing the nested comments with
> equivalent constructs that do not violate the rule.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>

You are missing the “---“ here, meaning that the lines below are part of the
commit message and I’m sure you don’t want that.

Also here, may I suggest to use this commit title instead?
“xen/arm: smmuv3: Fix violations of MISRA C:2012 Rule 3.1”

Apart from that, changes looks good to me:

Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>

Will be the maintainer/committer to decide if addressing these comment,
if accepted, on commit or if you need to send another version, in which
case you can retain my r-by provided that no other modifications are done.

> 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
> Changes in V3:
> - Revised the comment to make it clear the function the parallel control
> flows in the comment belong to.
> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index 720aa69ff2..cdbb505134 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()
> - * TLBI+SYNC atomic_inc(&nr_ats_masters);
> - * smp_mb(); [...]
> - * atomic_read(&nr_ats_masters); pci_enable_ats() // writel()
> + * --- unmap() ---                 --- arm_smmu_enable_ats() ---
> + * TLBI+SYNC                       atomic_inc(&nr_ats_masters);
> + * smp_mb();                       [...]
> + * atomic_read(&nr_ats_masters);   pci_enable_ats() (see 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.1
> 
>