On 11.08.2025 19:36, Dmytro Prokopchuk1 wrote: > --- a/xen/arch/arm/cpuerrata.c > +++ b/xen/arch/arm/cpuerrata.c > @@ -735,6 +735,11 @@ static int cpu_errata_callback(struct notifier_block > *nfb, > rc = enable_nonboot_cpu_caps(arm_errata); > break; > default: > + /* > + * Default case left empty as other notifier actions do not require > handling here. > + * The single CPU_STARTING action in the switch is sufficient > because this function > + * specifically handles enabling errata workarounds during CPU > startup. > + */ > break; > }
Here and below - please obey to the 80 char line length limit set forth by ./CODING_STYLE. As to what the comment says: There not being a need to clean up is tied to there not being any resources allocated during CPU_STARTING. Whether that's the case is left unclear. > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -436,6 +436,12 @@ static int cpu_gic_callback(struct notifier_block *nfb, > release_irq(gic_hw_ops->info->maintenance_irq, NULL); > break; > default: > + /* > + * Default case left empty as other notifier actions do not require > handling here. > + * The CPU_DYING action specifically handles releasing resources > acquired by > + * init_maintenance_interrupt(), when the CPU is being taken > offline. Other CPU > + * actions do not require GIC-specific handling. > + */ > break; > } I don't think this suffices. What about CPU_UP_CANCELED or CPU_RESUME_FAILED? It may well be that in that case init_maintenance_interrupt() would never have run, yet imo such needs stating explicitly. > --- a/xen/arch/arm/mmu/p2m.c > +++ b/xen/arch/arm/mmu/p2m.c > @@ -1828,6 +1828,12 @@ static int cpu_virt_paging_callback(struct > notifier_block *nfb, > setup_virt_paging_one(NULL); > break; > default: > + /* > + * Default case left empty as other notifier actions do not require > handling here. > + * The CPU_STARTING action is specifically handled because this > function sets up > + * virtual paging for CPUs, particularly for non-boot CPUs during > hotplug. Other > + * actions do not involve virtual paging setup. > + */ > break; > } See cpu_errata_callback() comment. > --- a/xen/arch/arm/time.c > +++ b/xen/arch/arm/time.c > @@ -382,6 +382,12 @@ static int cpu_time_callback(struct notifier_block *nfb, > deinit_timer_interrupt(); > break; > default: > + /* > + * Default case left empty as other notifier actions do not require > handling here. > + * The CPU_DYING action is specifically handled to revert actions > done in > + * init_timer_interrupt() and properly disable timer interrupts on > the CPU being > + * taken offline. Other actions do not involve timer > deinitialization. > + */ > break; > } See cpu_gic_callback() comment. > --- a/xen/common/kexec.c > +++ b/xen/common/kexec.c > @@ -549,6 +549,12 @@ static int cf_check cpu_callback( > kexec_init_cpu_notes(cpu); > break; > default: > + /* > + * Default case left empty as other notifier actions do not require > handling here. > + * The CPU_UP_PREPARE action is specifically handled to allocate > crash note buffers > + * for a newly onlined CPU. Other actions do not pertain to crash > note allocation > + * or memory preservation for kexec. > + */ > break; > } Yet when bringing down a CPU, these allocations are leaked. Looking at kexec_init_cpu_notes() that appears to be intentional, but again - such needs saying explicitly. Plus of course intentional leaks are always somewhat questionable, so extra justification may be on order. One further suggestion: Please consider splitting such patches at maintainer boundaries. For the patch to go in as is, you need both Arm and kexec approval. If you split, the part having got its approval could go in without need to wait for the other approval to trickle in. Jan
