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

[Julien Grall <julien@xxxxxxx>]

On 19/06/2024 12:17, Julien Grall wrote:
> 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?
Please ignore this comment. Just found it in the rules.rst.

> >           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