On 21/02/2023 1:40 pm, Jan Beulich wrote:
> On 20.02.2023 20:47, Andrew Cooper wrote:
>> Despite its name, the irq{save,restore}() APIs are only intended to
>> conditionally disable and re-enable interrupts.
> Are they?Yes, absolutely. And as said before, the potentially dubious naming does not give us permission or the right to interpret the behaviour differently. > Maybe nowadays they indeed are, but I couldn't spot any wording > to this effect in Linux'es Documentation/ (and I don't think we have > anywhere such could be put). Yet I'd expect such a statement to be backed > by something. It is backed by the rude words the owners of this API had to say on discovering this particular use. Conditionally enabling with a construct like this is bogus everywhere. It is only ever safe to enable irqs if you know exactly why they were disabled previously, and that can never be the case with a construct like this. It only reason this one example doesn't explode is because its an init path not nested within an irq/irqsave lock or other critical region. > >> IO-APIC's timer_irq_works() violates this intention. As it is init code, >> switch to simple irq enable/disable(). > If all callers of the function obviously did have interrupts off, I might > agree. But the last of them sits _after_ local_irq_restore(), and all of > this is called from underneath smp_prepare_cpus() Which do you mean "the last of them" ? There is a large amount of genuinely dead logic here. ~Andrew
