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@bugseng.com>]

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)