Re: [XEN PATCH v2] xen: add explicit comment to identify notifier patterns

[Julien Grall <julien@xxxxxxx>]

Hi Federico,

On 19/06/2024 10:29, Federico Serafini wrote:
> MISRA C Rule 16.4 states that every `switch' statement shall have a
> `default' label" and a statement or a comment prior to the
> terminating break statement.
>
> This patch addresses some violations of the rule related to the
> "notifier pattern": a frequently-used pattern whereby only a few values
> are handled by the switch statement and nothing should be done for
> others (nothing to do in the default case).
>
> Note that for function mwait_idle_cpu_init() in
> xen/arch/x86/cpu/mwait-idle.c the /* Notifier pattern. */ comment is
> not added: differently from the other functions covered in this patch,
> the default label has a return statement that does not violates Rule 16.4.
>
> No functional change.
>
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> ---
> Changes in v2:
> as Jan pointed out, in the v1 some patterns were not explicitly identified
> (https://lore.kernel.org/xen-devel/cad05a5c-e2d8-4e5d-af05-30ae6f959184@xxxxxxxxxxx/).
>
> This version adds the /* Notifier pattern. */ to all the pattern present in
> the Xen codebase except for mwait_idle_cpu_init().
> ---
>   xen/arch/arm/cpuerrata.c                     | 1 +
>   xen/arch/arm/gic-v3-lpi.c                    | 4 ++++
>   xen/arch/arm/gic.c                           | 1 +
>   xen/arch/arm/irq.c                           | 4 ++++
>   xen/arch/arm/mmu/p2m.c                       | 1 +
>   xen/arch/arm/percpu.c                        | 1 +
>   xen/arch/arm/smpboot.c                       | 1 +
>   xen/arch/arm/time.c                          | 1 +
>   xen/arch/arm/vgic-v3-its.c                   | 2 ++
>   xen/arch/x86/acpi/cpu_idle.c                 | 4 ++++
>   xen/arch/x86/cpu/mcheck/mce.c                | 4 ++++
>   xen/arch/x86/cpu/mcheck/mce_intel.c          | 4 ++++
>   xen/arch/x86/genapic/x2apic.c                | 3 +++
>   xen/arch/x86/hvm/hvm.c                       | 1 +
>   xen/arch/x86/nmi.c                           | 1 +
>   xen/arch/x86/percpu.c                        | 3 +++
>   xen/arch/x86/psr.c                           | 3 +++
>   xen/arch/x86/smpboot.c                       | 3 +++
>   xen/common/kexec.c                           | 1 +
>   xen/common/rcupdate.c                        | 1 +
>   xen/common/sched/core.c                      | 1 +
>   xen/common/sched/cpupool.c                   | 1 +
>   xen/common/spinlock.c                        | 1 +
>   xen/common/tasklet.c                         | 1 +
>   xen/common/timer.c                           | 1 +
>   xen/drivers/cpufreq/cpufreq.c                | 1 +
>   xen/drivers/cpufreq/cpufreq_misc_governors.c | 3 +++
>   xen/drivers/passthrough/x86/hvm.c            | 3 +++
>   xen/drivers/passthrough/x86/iommu.c          | 3 +++
>   29 files changed, 59 insertions(+)
>
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 2b7101ea25..69c30aecd8 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -730,6 +730,7 @@ static int cpu_errata_callback(struct notifier_block *nfb,
>           rc = enable_nonboot_cpu_caps(arm_errata);
>           break;
>       default:
> +        /* Notifier pattern. */
Without looking at the commit message (which may not be trivial when 
committed), it is not clear to me what this is supposed to mean. Will 
there be a longer explanation in the MISRA doc? Should this be a SAF-* 
comment?
>           break;
>       }
>   
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index eb0a5535e4..4c2bd35403 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -389,6 +389,10 @@ static int cpu_callback(struct notifier_block *nfb, 
> unsigned long action,
>               printk(XENLOG_ERR "Unable to allocate the pendtable for CPU%lu\n",
>                      cpu);
>           break;
> +
> +    default:
> +        /* Notifier pattern. */
> +        break;
Skimming through v1, it was pointed out that gic-v3-lpi may miss some cases.

Let me start with that I understand this patch is technically not 
changing anything. However, it gives us an opportunity to check the 
notifier pattern.
Has anyone done any proper investigation? If so, what was the outcome? 
If not, have we identified someone to do it?
The same question will apply for place where you add "default".

Cheers,

--
Julien Grall