On 21.06.2024 11:50, Nicola Vetrini wrote:
> From: Alessandro Zucchelli <alessandro.zucche...@bugseng.com>
>
> 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.zucche...@bugseng.com>
> Signed-off-by: Nicola Vetrini <nicola.vetr...@bugseng.com>
> ---
> 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
