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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 20 Jun 2024, Federico Serafini wrote:
> On 19/06/24 13: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?
> > 
> > >           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".
> 
> Yes, I also think this could be an opportunity to check the pattern
> but no one has yet been identified to do this.

I don't think I understand Julien's question and/or your answer.

Is the question whether someone has done an analysis to make sure this
patch covers all notifier patters in the xen codebase?

If so, I expect that you have done an analysis simply by basing this
patch on the 16.4 violations reported by ECLAIR?