Messages in this thread Patch in this message | | | From | Valentin Schneider <> | Subject | Re: [PATCH v2 2/6] genirq: Allow an interrupt to be marked as 'raw' | Date | Thu, 26 Nov 2020 18:18:33 +0000 |
| |
Hi Marc,
On 24/11/20 14:14, Marc Zyngier wrote: > @@ -680,14 +679,22 @@ int __handle_domain_irq(struct irq_domain *domain, unsigned int hwirq, > * Some hardware gives randomly wrong interrupts. Rather > * than crashing, do something sensible. > */ > - if (unlikely(!irq || irq >= nr_irqs)) { > + if (unlikely(!irq || irq >= nr_irqs || !(desc = irq_to_desc(irq)))) { > ack_bad_irq(irq); > ret = -EINVAL; > + goto out; > + } > + > + if (IS_ENABLED(CONFIG_ARCH_WANTS_IRQ_RAW) && > + unlikely(irq_settings_is_raw(desc))) { > + generic_handle_irq_desc(desc);
If I got the RCU bits right from what Thomas mentioned in
https://lore.kernel.org/r/87ft5q18qs.fsf@nanos.tec.linutronix.de https://lore.kernel.org/r/87lfewnmdz.fsf@nanos.tec.linutronix.de
then we're still missing something to notify RCU in the case the IRQ hits the idle task. All I see on our entry path is
trace_hardirqs_off(); ... irq_handler() handle_domain_irq(); ... trace_hardirqs_on();
so we do currently rely on handle_domain_irq()'s irq_enter() + irq_exit() for that. rcu_irq_enter() says CONFIG_RCU_EQS_DEBUG=y can detect missing bits, but I don't get any warnings with your series on my Juno.
Now, irq_enter() gives us:
rcu_irq_enter(); irq_enter_rcu() raise_softirq faffery; __irq_enter() irqtime accounting; preempt count + lockdep; } __irq_enter_raw()
Looking at irqentry_enter() + DEFINE_IDTENTRY_SYSVEC_SIMPLE(), I *think* we would be fine with just:
rcu_irq_enter(); __irq_enter_raw();
generic_handle_irq_desc()
__irq_exit_raw(); rcu_irq_exit();
I tested that and it didn't explode (though I haven't managed to make CONFIG_RCU_EQS_DEBUG squeal). Also please note RCU isn't my forte, so the above may contain traces of bullcrap.
> } else { > - generic_handle_irq(irq); > + irq_enter(); > + generic_handle_irq_desc(desc); > + irq_exit(); > } > > - irq_exit(); > +out: > set_irq_regs(old_regs); > return ret; > } [...] > @@ -180,3 +182,16 @@ static inline bool irq_settings_is_hidden(struct irq_desc *desc) > { > return desc->status_use_accessors & _IRQ_HIDDEN; > } > + > +static inline bool irq_settings_is_raw(struct irq_desc *desc) > +{ > + if (IS_ENABLED(CONFIG_ARCH_WANTS_IRQ_RAW)) > + return desc->status_use_accessors & _IRQ_RAW; > + > + /* > + * Using IRQ_RAW on architectures that don't expect it is > + * likely to be wrong. > + */ > + WARN_ON_ONCE(1);
Per __handle_domain_irq()'s short-circuit evaluation, this is only entered when the above config is enabled. Perhaps a better place to check for this would be in __irq_settings_clr_and_set().
> + return false; > +}
| |