Re: [RFC XEN PATCH] x86/mctelem: address violations of MISRA C: 2012 Rule 5.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.06.2024 11:50, Nicola Vetrini wrote:
> From: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
> 
> This addresses violations of MISRA C:2012 Rule 5.3 which states as
> following: An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope. In this case the shadowing is between
> local variables "mctctl" and the file-scope static struct variable with the
> same name.
> 
> No functional change.
> 
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
> RFC because I'm not 100% sure the semantics of the code is preserved.
> I think so, and it passes gitlab pipelines [1], but there may be some missing
> information.

Details as to your concerns would help. I see no issue, not even a concern.

> --- a/xen/arch/x86/cpu/mcheck/mctelem.c
> +++ b/xen/arch/x86/cpu/mcheck/mctelem.c
> @@ -168,14 +168,14 @@ static void mctelem_xchg_head(struct mctelem_ent 
> **headp,
>  void mctelem_defer(mctelem_cookie_t cookie, bool lmce)
>  {
>       struct mctelem_ent *tep = COOKIE2MCTE(cookie);
> -     struct mc_telem_cpu_ctl *mctctl = &this_cpu(mctctl);
> +     struct mc_telem_cpu_ctl *mctctl_cpu = &this_cpu(mctctl);

When possible (i.e. without loss of meaning) I'd generally prefer names to
be shortened. Wouldn't just "ctl" work here?

> -     ASSERT(mctctl->pending == NULL || mctctl->lmce_pending == NULL);
> +     ASSERT(mctctl_cpu->pending == NULL || mctctl_cpu->lmce_pending == NULL);
>  
> -     if (mctctl->pending)
> -             mctelem_xchg_head(&mctctl->pending, &tep->mcte_next, tep);
> +     if (mctctl_cpu->pending)
> +             mctelem_xchg_head(&mctctl_cpu->pending, &tep->mcte_next, tep);
>       else if (lmce)
> -             mctelem_xchg_head(&mctctl->lmce_pending, &tep->mcte_next, tep);
> +             mctelem_xchg_head(&mctctl_cpu->lmce_pending, &tep->mcte_next, 
> tep);
>       else {
>               /*
>                * LMCE is supported on Skylake-server and later CPUs, on
> @@ -186,10 +186,10 @@ void mctelem_defer(mctelem_cookie_t cookie, bool lmce)
>                * moment. As a result, the following two exchanges together
>                * can be treated as atomic.
>                */

In the middle of this comment the variable is also mentioned, and hence
also wants adjusting (twice).

> -             if (mctctl->lmce_pending)
> -                     mctelem_xchg_head(&mctctl->lmce_pending,
> -                                       &mctctl->pending, NULL);
> -             mctelem_xchg_head(&mctctl->pending, &tep->mcte_next, tep);
> +             if (mctctl_cpu->lmce_pending)
> +                     mctelem_xchg_head(&mctctl_cpu->lmce_pending,
> +                                       &mctctl_cpu->pending, NULL);
> +             mctelem_xchg_head(&mctctl_cpu->pending, &tep->mcte_next, tep);
>       }
>  }
>  
> @@ -213,7 +213,7 @@ void mctelem_process_deferred(unsigned int cpu,
>  {
>       struct mctelem_ent *tep;
>       struct mctelem_ent *head, *prev;
> -     struct mc_telem_cpu_ctl *mctctl = &per_cpu(mctctl, cpu);
> +     struct mc_telem_cpu_ctl *mctctl_cpu = &per_cpu(mctctl, cpu);
>       int ret;
>  
>       /*
> @@ -232,7 +232,7 @@ void mctelem_process_deferred(unsigned int cpu,
>        * Any MC# occurring after the following atomic exchange will be
>        * handled by another round of MCE softirq.
>        */
> -     mctelem_xchg_head(lmce ? &mctctl->lmce_pending : &mctctl->pending,
> +     mctelem_xchg_head(lmce ? &mctctl_cpu->lmce_pending : 
> &mctctl_cpu->pending,
>                         &this_cpu(mctctl.processing), NULL);

By shortening the variable name here you'd also avoid going past line
length limits.

Jan