On Wed, 2007-07-18 at 15:46 -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2007 19:09:57 +0900
> Fernando Luis V__zquez Cao <fernando at oss.ntt.co.jp> wrote:
>
> > With the advent of kdump it is possible that device drivers receive
> > interrupts generated in the context of a previous kernel. Ideally
> > quiescing the underlying devices should suffice but not all drivers
> > do this, either because it is not possible or because they did not
> > contemplate this case. Thus drivers ought to be able to handle
> > interrupts coming in as soon as the interrupt handler is registered.
> >
> > Signed-off-by: Fernando Luis Vazquez Cao <fernando at oss.ntt.co.jp>
> > ---
> >
> > diff -urNp linux-2.6.22-orig/kernel/irq/manage.c linux-2.6.22/kernel/irq/manage.c
> > --- linux-2.6.22-orig/kernel/irq/manage.c 2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/kernel/irq/manage.c 2007-07-17 18:37:24.000000000 +0900
> > @@ -537,6 +537,29 @@ int request_irq(unsigned int irq, irq_ha
> >
> > select_smp_affinity(irq);
> >
> > +#if defined(CONFIG_DEBUG_PENDING_IRQ) || defined(CONFIG_DEBUG_SHIRQ)
> > +#ifndef CONFIG_DEBUG_PENDING_IRQ
> > + if (irqflags & IRQF_SHARED) {
> > + /*
> > + * It's a shared IRQ -- the driver ought to be prepared for it
> > + * to happen immediately, so let's make sure....
> > + * We do this before actually registering it, to make sure that
> > + * a 'real' IRQ doesn't run in parallel with our fake.
> > + */
> > +#endif /* !CONFIG_DEBUG_PENDING_IRQ */
> > + if (irqflags & IRQF_DISABLED) {
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > + handler(irq, dev_id);
> > + local_irq_restore(flags);
> > + } else
> > + handler(irq, dev_id);
> > +#ifndef CONFIG_DEBUG_PENDING_IRQ
> > + }
> > +#endif /* !CONFIG_DEBUG_PENDING_IRQ */
> > +#endif /* CONFIG_DEBUG_PENDING_IRQ || CONFIG_DEBUG_SHIRQ */
>
> Even if we were going to merge this functionality as-is, I'd ask for some
> sort of refactoring to fix up that ifdef maze.
I a absolutely agree. My first impulse was to get rid of all the cpp
kludge including the Kconfig setting CONFIG_DEBUG_SHIRQ, since, as you
pointed out, request_irq() is not really performance critical.
Unfortunately for the RFC I decided to be conservative and ended up with
an "ifdef maze". Thank you for the heads-up.
> But more substantial issues:
>
> - This is presented as a "debug" feature, but it isn't a debug feature at
> all - it is new functionality which is unrelated to kernel development.
Yup.
> Also, it is a "debug" feature which provides no debugging! At the very
> least, one would expect to see it emit a printk to tell people that we
> have some driver which needs fixing.
I am afraid that in some occasions the kernel may panic inside the
interrupt handler, but I agree that we need to print a meaningful
message for the general case (i.e. something goes wrong but we can
recover). I will do that.
> Also, this not-really-a-debug-feature is undesirably coupled with a
> real debugging feature: CONFIG_DEBUG_PENDING_IRQ.
In the new version of the patch I will remove both
CONFIG_DEBUG_PENDING_IRQ and CONFIG_DEBUG_SHIRQ. request_irq() and
setup_irq() are not fast paths and free_irq() much less so.
> - Does this new feature really need its own Kconfig setting? Why not enable
> it unconditionally? request_irq() isn't exactly performance-critical.
I guess the Kconfig setting is not needed. In fact, by enabling this
feature unconditionally we would have _everyone_ (unknowing) testing an
area which is a major pain point for kdump. I am not sure this is an
acceptable default for all systems though. Opinions welcome.
> - If poss, we really do want to find some way of emitting a warning when
> we detect such a device driver. Like, call the handler and if it
> returned IRQ_HANDLED, start shouting.
I will do that and submit updated patches.
Thank you for your feedback, Andrew!
