Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2023 18:10, nicola.vetrini@xxxxxxxxxxx wrote:
> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> 
> The xen sources contain several violations of Rule 3.1 from MISRA C:2012,
> whose headline states:
> "The character sequences '/*' and '//' shall not be used within a comment".
> 
> Most of the violations are due to the presence of links to webpages within
> C-style comment blocks, such as:
> 
> xen/arch/arm/include/asm/smccc.h:37.1-41.3
> /*
>  * This file provides common defines for ARM SMC Calling Convention as
>  * specified in
>  * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>  */
> 
> In this case, we propose to deviate all of these occurrences with a
> project deviation to be captured by a tool configuration.

Here "propose" is appropriate in the description, as this is something
the patch does not do. Further down, however, you mean to describe what
the patch does, not what an eventual patch might do.

Somewhat similarly you don't want to use past tense in the title, as
that wants to describe what is being done, rather than describing a
patch that has already landed.

> There are, however, a few other violations that do not fall under this
> category, namely:
> 
> 1. in file "xen/arch/arm/include/asm/arm64/flushtlb.h" we propose to
> avoid the usage of a nested comment;
> 2. in file "xen/common/xmalloc_tlsf.c" we propose to substitute the
> commented-out if statement with a "#if 0 .. #endif;
> 3. in file "xen/include/xen/atomic.h" and
> "xen/drivers/passthrough/arm/smmu-v3.c" we propose to split the C-style
> comment containing the nested comment into two doxygen comments, clearly
> identifying the second as a code sample. This can then be captured with a
> project deviation by a tool configuration.

Finally I find the use of "we" somewhat suspicious. Who besides you is
it that you're talking about?

Also please don't forget to Cc relevant maintainers.

> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1045,15 +1045,18 @@ static int arm_smmu_atc_inv_domain(struct 
> arm_smmu_domain *smmu_domain,
>       /*
>        * Ensure that we've completed prior invalidation of the main TLBs
>        * before we read 'nr_ats_masters' in case of a concurrent call to
> -      * arm_smmu_enable_ats():
> +      * arm_smmu_enable_ats().
> +      */
> +     /**
> +      * Code sample: Ensures that we always see the incremented
> +      * 'nr_ats_masters' count if ATS was enabled at the PCI device before
> +      * completion of the TLBI.
>        *
>        *      // unmap()                      // arm_smmu_enable_ats()
>        *      TLBI+SYNC                       atomic_inc(&nr_ats_masters);
>        *      smp_mb();                       [...]
>        *      atomic_read(&nr_ats_masters);   pci_enable_ats() // 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.
>        */
>       smp_mb();
>       if (!atomic_read(&smmu_domain->nr_ats_masters))

I don't really know this code, but the use of inner comments looks
unnecessary to me here. The same could e.g. be achieved by

         *      unmap():                        arm_smmu_enable_ats():
         *      TLBI+SYNC                       atomic_inc(&nr_ats_masters);
         *      smp_mb();                       [...]
         *      atomic_read(&nr_ats_masters);   pci_enable_ats(); (i.e. 
writel())


and it would avoid the somewhat odd splitting of adjacent comments
(which then is at risk of someone later coming forward with a patch
re-combining them).

> --- a/xen/include/xen/atomic.h
> +++ b/xen/include/xen/atomic.h
> @@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);
>   * Returns the initial value in @v, hence succeeds when the return value
>   * matches that of @old.
>   *
> - * Sample (tries atomic increment of v until the operation succeeds):
> + */
> +/**
> + *
> + * Code sample: Tries atomic increment of v until the operation succeeds.
>   *
>   *  while(1)
>   *  {

Somewhat similarly here: I don't think the inner comment provides
much value, and could hence be dropped instead of splitting the comment.

Jan