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(+)
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@bugseng.com>
---
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@bugseng.com/).
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. */
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;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3eaf670fd7..dc5408a456 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -463,6 +463,7 @@ static int cpu_gic_callback(struct notifier_block *nfb,
release_irq(gic_hw_ops->info->maintenance_irq, NULL);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index c60502444c..61ca6f5b87 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -127,6 +127,10 @@ static int cpu_callback(struct notifier_block *nfb, unsigned long action,
printk(XENLOG_ERR "Unable to allocate local IRQ for CPU%u\n",
cpu);
break;
+
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
index 1725cca649..bf7c66155d 100644
--- a/xen/arch/arm/mmu/p2m.c
+++ b/xen/arch/arm/mmu/p2m.c
@@ -1839,6 +1839,7 @@ static int cpu_virt_paging_callback(struct notifier_block *nfb,
setup_virt_paging_one(NULL);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/arm/percpu.c b/xen/arch/arm/percpu.c
index 87fe960330..81f91f05bb 100644
--- a/xen/arch/arm/percpu.c
+++ b/xen/arch/arm/percpu.c
@@ -66,6 +66,7 @@ static int cpu_percpu_callback(
free_percpu_area(cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 04e363088d..3d481e59f9 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -591,6 +591,7 @@ static int cpu_smpboot_callback(struct notifier_block *nfb,
remove_cpu_sibling_map(cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index e74d30d258..27cbfae874 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -382,6 +382,7 @@ static int cpu_time_callback(struct notifier_block *nfb,
deinit_timer_interrupt();
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 70b5aeb822..a33ff64ff2 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1194,6 +1194,7 @@ static void sanitize_its_base_reg(uint64_t *reg)
r |= GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
break;
default:
+ /* Notifier pattern. */
break;
}
@@ -1206,6 +1207,7 @@ static void sanitize_its_base_reg(uint64_t *reg)
r |= GIC_BASER_CACHE_RaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 57ac984790..f0af32e578 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -1661,6 +1661,10 @@ static int cf_check cpu_callback(
processor_powers[cpu] )
amd_cpuidle_init(processor_powers[cpu]);
break;
+
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 32c1b2756b..222b174bbb 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -722,6 +722,10 @@ static int cf_check cpu_callback(
if ( park_offline_cpus )
cpu_bank_free(cpu);
break;
+
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index dd812f4b8a..be82ea4631 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -949,6 +949,10 @@ static int cf_check cpu_callback(
cpu_mcheck_distribute_cmci();
cpu_mcabank_free(cpu);
break;
+
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index 371dd100c7..d271102f9f 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -238,6 +238,9 @@ static int cf_check update_clusterinfo(
}
FREE_CPUMASK_VAR(per_cpu(scratch_mask, cpu));
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(err);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8334ab1711..00c360cf24 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -123,6 +123,7 @@ static int cf_check cpu_callback(
alternative_vcall(hvm_funcs.cpu_dead, cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 9793fa2316..105efa5a71 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -434,6 +434,7 @@ static int cf_check cpu_nmi_callback(
kill_timer(&per_cpu(nmi_timer, cpu));
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c
index 3205eacea6..627b56b9f3 100644
--- a/xen/arch/x86/percpu.c
+++ b/xen/arch/x86/percpu.c
@@ -84,6 +84,9 @@ static int cf_check cpu_percpu_callback(
if ( park_offline_cpus )
free_percpu_area(cpu);
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 0b9631ac44..e76b129e6c 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -1661,6 +1661,9 @@ static int cf_check cpu_callback(
case CPU_DEAD:
psr_cpu_fini(cpu);
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 8aa621533f..5b9b196d58 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1134,6 +1134,9 @@ static int cf_check cpu_smpboot_callback(
case CPU_REMOVE:
cpu_smpboot_free(cpu, true);
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return notifier_from_errno(rc);
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 84fe8c3597..96883cdc70 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -549,6 +549,7 @@ static int cf_check cpu_callback(
kexec_init_cpu_notes(cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
return NOTIFY_DONE;
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 212a99acd8..0fe4097544 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -657,6 +657,7 @@ static int cf_check cpu_callback(
rcu_offline_cpu(&this_cpu(rcu_data), &rcu_ctrlblk, rdp);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d84b65f197..dffa1ef476 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2907,6 +2907,7 @@ static int cf_check cpu_schedule_callback(
cpu_schedule_down(cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index ad8f608462..c7117f4243 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -1073,6 +1073,7 @@ static int cf_check cpu_callback(
cpupool_cpu_remove_forced(cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 28c6e9d3ac..bf082478db 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -55,6 +55,7 @@ static int cf_check cpu_lockdebug_callback(struct notifier_block *nfb,
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
index 4c8d87a338..879b1f0d80 100644
--- a/xen/common/tasklet.c
+++ b/xen/common/tasklet.c
@@ -232,6 +232,7 @@ static int cf_check cpu_callback(
migrate_tasklets_from_cpu(cpu, &per_cpu(softirq_tasklet_list, cpu));
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/common/timer.c b/xen/common/timer.c
index a21798b76f..60e9a1493e 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -677,6 +677,7 @@ static int cf_check cpu_callback(
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 8659ad3aee..9584b55398 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -682,6 +682,7 @@ static int cf_check cpu_callback(
(void)cpufreq_del_cpu(cpu);
break;
default:
+ /* Notifier pattern. */
break;
}
diff --git a/xen/drivers/cpufreq/cpufreq_misc_governors.c b/xen/drivers/cpufreq/cpufreq_misc_governors.c
index 0327fad23b..464b267a17 100644
--- a/xen/drivers/cpufreq/cpufreq_misc_governors.c
+++ b/xen/drivers/cpufreq/cpufreq_misc_governors.c
@@ -101,6 +101,9 @@ static int cf_check cpufreq_userspace_cpu_callback(
case CPU_UP_PREPARE:
per_cpu(cpu_set_freq, cpu) = userspace_cmdline_freq;
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return NOTIFY_DONE;
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index d3627e4af7..e5b6be4794 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -1122,6 +1122,9 @@ static int cf_check cpu_callback(
*/
ASSERT(list_empty(&per_cpu(dpci_list, cpu)));
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return NOTIFY_DONE;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index cc0062b027..f0c84eeb85 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -749,6 +749,9 @@ static int cf_check cpu_callback(
if ( !page_list_empty(list) )
tasklet_schedule(tasklet);
break;
+ default:
+ /* Notifier pattern. */
+ break;
}
return NOTIFY_DONE;
--
2.34.1Hi 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@bugseng.com> > --- > 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@bugseng.com/). > > 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
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@bugseng.com> >> --- >> 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@bugseng.com/). >> >> 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. -- Federico Serafini, M.Sc. Software Engineer, BUGSENG (http://bugseng.com)
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@bugseng.com> > > > --- > > > 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@bugseng.com/). > > > > > > 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?
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@bugseng.com> >>>> --- >>>> 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@bugseng.com/). >>>> >>>> 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)
On Fri, 21 Jun 2024, Federico Serafini wrote: > 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@bugseng.com> > > > > > --- > > > > > 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@bugseng.com/). > > > > > > > > > > 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. That's really difficult to do, even for the maintainers of the code in question. And by not taking this patch we are exposing ourselves to more safety risks because we cannot make R16.4 blocking. > > 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). I think this patch is a step in the right direction. It doesn't prevent anyone in the community from making expert evaluations on whether the pattern is implemented correctly. Honestly, I don't see another way to make progress on this, except for maybe deviating project-wide "struct notifier_block". But that's conceptually the same thing as this patch. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi Stefano, On 21/06/2024 23:34, Stefano Stabellini wrote: >>>> 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. > > That's really difficult to do, even for the maintainers of the code in > question. Sure it will require some work, but as any violation. However, I thought the whole point of MISRA is to improve the safety and our code base in general? AFAIU, we already have some doubt that some notifiers are correct. So to me it seems wrong to add a comment because while this silence MISRA, this doesn't solve it in the true spirit. > > And by not taking this patch we are exposing ourselves to more safety > risks because we cannot make R16.4 blocking. > > >>> 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). > > I think this patch is a step in the right direction. It doesn't prevent > anyone in the community from making expert evaluations on whether the > pattern is implemented correctly. I am not sure I am reading this correctly. To me you are saying that for your MISRA, adding the default case is fine even if we believe some notifiers are incorrect. Did I understand right? If so it does seems odd because this is really not solving the violation. You are just putting a smoke screen in front in the hope that there are no big issue in the code... > > Honestly, I don't see another way to make progress on this, except for > maybe deviating project-wide "struct notifier_block". But that's > conceptually the same thing as this patch. I still don't quite understand why you are so eager to hide violation just that you can progress faster on other bits. I personally cannot put my name on a patch where the goal is to add a comment that no-one verified that it was actually true. (i.e. all the cases we care are handled). In particular in the Arm code, because IIRC we already identified issues in the past in the notifier. I think it wouild be worth discussing the approach again in the next MISRA meeting. Cheers, > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> -- Julien Grall
On Sat, 22 Jun 2024, Julien Grall wrote: > On 21/06/2024 23:34, Stefano Stabellini wrote: > > > > > 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. > > > > That's really difficult to do, even for the maintainers of the code in > > question. > Sure it will require some work, but as any violation. However, I thought the > whole point of MISRA is to improve the safety and our code base in general? > > AFAIU, we already have some doubt that some notifiers are correct. So to me it > seems wrong to add a comment because while this silence MISRA, this doesn't > solve it in the true spirit. > > > > > And by not taking this patch we are exposing ourselves to more safety > > risks because we cannot make R16.4 blocking. > > > > > > > > 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). > > > > I think this patch is a step in the right direction. It doesn't prevent > > anyone in the community from making expert evaluations on whether the > > pattern is implemented correctly. > > I am not sure I am reading this correctly. To me you are saying that for your > MISRA, adding the default case is fine even if we believe some notifiers are > incorrect. Did I understand right? > > If so it does seems odd because this is really not solving the violation. You > are just putting a smoke screen in front in the hope that there are no big > issue in the code... > > > > > Honestly, I don't see another way to make progress on this, except for > > maybe deviating project-wide "struct notifier_block". But that's > > conceptually the same thing as this patch. > > I still don't quite understand why you are so eager to hide violation just > that you can progress faster on other bits. > > I personally cannot put my name on a patch where the goal is to add a comment > that no-one verified that it was actually true. (i.e. all the cases we care > are handled). In particular in the Arm code, because IIRC we already > identified issues in the past in the notifier. > > I think it wouild be worth discussing the approach again in the next MISRA > meeting. Yes, good idea, we can discuss tomorrow. I'll write down my thinking about 16.4 here in the meantime to address your question and hopefully we can align on the approach to take tomorrow. As you might remember I supported 16.4 and I was keen on having it in rules.rst because I believe that this rules makes the code better. 16.4 is about ensuring that every switch that is supposed to handle all possible parameters, actually handle all possible parameters. It is also about having a default label so that we can handle unexpected parameters especially in case of integers parameters. (In case of enums we can check at compile time that we handle all possible parameters even without a default label.) In Xen, some switches are expected to handle all possible parameters, but some of them are not. Specifically the "notifier pattern" switches are by design not expected to handle all possible parameters. Of course it can be that some of them have to, but the design of the "notifier pattern" is that you can handle only the subset you care about, and you don't need to care about all of the possible parameters. ECLAIR or gcc cannot help us there. Most of these instances don't want to handle all parameters. We have to remove the violations from the scan, either by deviation or by adding a default label. Otherwise the notifier pattern stops us from making more progress. There are lots of other switches that are not part of the notifier pattern and are required to handle all possible parameters. I would like to make sure we enable the checks for these other switches where ECLAIR and gcc can actually help immediately. I do realize that some of the notifier pattern switches might want to handle all parameters but Bugseng or anyone else looking for simple improvements are not in the position to tell which ones they are. We need to wait for a maintainer or expert in the specific code to spot them. It is not a good idea to cause a delay in handling all the remaining 16.4 more interesting switches (which is also easy) to better handle the notifier pattern (which is hard). The notifier pattern can be looked at separately later by the relevant maintainer / interested community members by sending case-by-case improvements. They cannot be mechanically resolved. My understanding is that with this patch series committed we would be close to zero violations for 16.4.
On 25.06.2024 02:14, Stefano Stabellini wrote: > I do realize that some of the notifier pattern switches might want to > handle all parameters but Bugseng or anyone else looking for simple > improvements are not in the position to tell which ones they are. We > need to wait for a maintainer or expert in the specific code to spot > them. It is not a good idea to cause a delay in handling all the > remaining 16.4 more interesting switches (which is also easy) to better > handle the notifier pattern (which is hard). > > The notifier pattern can be looked at separately later by the relevant > maintainer / interested community members by sending case-by-case > improvements. They cannot be mechanically resolved. My understanding is > that with this patch series committed we would be close to zero > violations for 16.4. In fact yielding a bogus result, suggesting the tree is in a better state than it really is. Putting myself in an assessor's position, I might be considering such close to lying at me. IOW the fact that some violations cannot be mechanically resolved shouldn't lead to us mechanically papering over them. Jan
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@bugseng.com> >> --- >> 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@bugseng.com/). >> >> 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
On 19.06.2024 13:21, Julien Grall wrote: > > > 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@bugseng.com> >>> --- >>> 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@bugseng.com/). >>> >>> 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. Except that there it is only an example (and such an example could even be replaced at any time). Already on the previous version I had asked that some explanation be added as to what this means and under what circumstances it is legitimate to add (kind of related to a later part of the earlier reply of yours). Jan
© 2016 - 2024 Red Hat, Inc.