On Mon, Oct 27, 2025 at 12:53:34PM +0100, Jan Beulich wrote: > On 27.10.2025 12:33, Roger Pau Monné wrote: > > On Mon, Oct 27, 2025 at 11:23:58AM +0100, Jan Beulich wrote: > >> On 24.10.2025 15:24, Roger Pau Monné wrote: > >>> On Thu, Oct 23, 2025 at 05:50:17PM +0200, Jan Beulich wrote: > >>>> @@ -343,6 +347,12 @@ static int __init hpet_setup_msi_irq(str > >>>> u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx)); > >>>> irq_desc_t *desc = irq_to_desc(ch->msi.irq); > >>>> > >>>> + clear_irq_vector(ch->msi.irq); > >>>> + ret = bind_irq_vector(ch->msi.irq, HPET_BROADCAST_VECTOR, > >>>> &cpu_online_map); > >>> > >>> By passing cpu_online_map here, it leads to _bind_irq_vector() doing: > >>> > >>> cpumask_copy(desc->arch.cpu_mask, &cpu_online_map); > >>> > >>> Which strictly speaking is wrong. However this is just a cosmetic > >>> issue until the irq is used for the first time, at which point it will > >>> be assigned to a concrete CPU. > >>> > >>> You could do: > >>> > >>> cpumask_clear(desc->arch.cpu_mask); > >>> cpumask_set_cpu(cpumask_any(&cpu_online_map), desc->arch.cpu_mask); > >>> > >>> (Or equivalent) > >>> > >>> To assign the interrupt to a concrete CPU and reflex it on the > >>> cpu_mask after the bind_irq_vector() call, but I can live with it > >>> being like this. I have patches to adjust _bind_irq_vector() myself, > >>> which I hope I will be able to post soon. > >> > >> Hmm, I wrongly memorized hpet_broadcast_init() as being pre-SMP-init only. > >> It has three call sites: > >> - mwait_idle_init(), called from cpuidle_presmp_init(), > >> - amd_cpuidle_init(), calling in only when invoked the very first time, > >> which is again from cpuidle_presmp_init(), > >> - _disable_pit_irq(), called from the regular initcall disable_pit_irq(). > >> I.e. for the latter you're right that the CPU mask is too broad (in only a > >> cosmetic way though). Would be you okay if I used cpumask_of(0) in place > >> of &cpu_online_map? > > > > Using cpumask_of(0) would be OK, as the per-cpu vector_irq array will > > be updated ahead of assigning the interrupt to a CPU, and hence it > > doesn't need to be done for all possible online CPUs in > > _bind_irq_vector(). > > > > In the context here it would be more accurate to provide an empty CPU > > mask, as the interrupt is not yet targeting any CPU. Using CPU 0 > > would be a placeholder, which seems fine for the purpose. > > Putting an empty mask there, while indeed logically correct, would (I fear) > again put us at risk with other code making various assumptions. I'll go > with cpumask_of(0).
Yeah, that's what I fear also. It's in principle possible for the cpu_mask to be empty, but since this targets 4.21 I think it's too risky. Using cpumask_of(0) is fine. Please keep my RB. Thanks, Roger.
