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

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

On 21/06/24 03:13, Stefano Stabellini wrote:
> 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?
I think Jan and Julien's concerns are about the fact that my patch
takes for granted that all the switch statements are doing the right
thing: someone should investigate the notifier patterns to confirm that
their are handling the different cases correctly.

> If so, I expect that you have done an analysis simply by basing this
> patch on the 16.4 violations reported by ECLAIR?
The previous version of the patch was based only on the reports of
ECLAIR but Jan said "you left out some patterns, why?".

So, this version of the patch adds the comment for all the notifier
patterns I found using git grep "struct notifier_block \*"
(a superset of the ones reported by ECLAIR because some of them are in
files excluded from the analysis or deviated).

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)