RE: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: Tuesday, January 5, 2021 12:01 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>
> Cc: tglx@xxxxxxxxxxxxx; maz@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linuxarm@xxxxxxxxxxxxx
> Subject: Re: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq
> 
> On Tue, Jan 05, 2021 at 11:26:12AM +1300, Barry Song wrote:
> > This patch originated from the discussion with Dmitry in the below thread:
> >
> https://lore.kernel.org/linux-input/20210102042902.41664-1-song.bao.hua@hi
> silicon.com/
> > there are many drivers which don't want interrupts enabled automatically
> > due to request_irq().
> > So they are handling this issue by either way of the below two:
> > (1)
> > irq_set_status_flags(irq, IRQ_NOAUTOEN);
> > request_irq(dev, irq...);
> > (2)
> > request_irq(dev, irq...);
> > disable_irq(irq);
> >
> > The code in the second way is silly and unsafe. In the small time gap
> > between request_irq and disable_irq, interrupts can still come.
> > The code in the first way is safe though we might be able to do it in
> > the generic irq code.
> >
> > I guess Dmitry also prefers genirq handles this as he said
> > "What I would like to see is to allow passing something like IRQF_DISABLED
> > to request_irq() so that we would not need neither irq_set_status_flags()
> > nor disable_irq()" in the original email thread.
> 
> One of the reasons I dislike irq_set_status_flags() is that we have to
> call it before we actually granted our IRQ request...
> 
> >
> > If this one is accepted, hundreds of drivers with this problem will be
> > handled afterwards.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> > ---
> >  include/linux/interrupt.h |  3 +++
> >  kernel/irq/manage.c       |  3 +++
> >  kernel/irq/settings.h     | 10 ++++++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index bb8ff9083e7d..0f22d277078c 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -61,6 +61,8 @@
> >   *                interrupt handler after suspending interrupts. For system
> >   *                wakeup devices users need to implement wakeup detection in
> >   *                their interrupt handlers.
> > + * IRQF_NO_AUTOEN - Don't enable IRQ automatically when users request it.
> Users
> > + *                will enable it explicitly by enable_irq() later.
> >   */
> >  #define IRQF_SHARED		0x00000080
> >  #define IRQF_PROBE_SHARED	0x00000100
> > @@ -74,6 +76,7 @@
> >  #define IRQF_NO_THREAD		0x00010000
> >  #define IRQF_EARLY_RESUME	0x00020000
> >  #define IRQF_COND_SUSPEND	0x00040000
> > +#define IRQF_NO_AUTOEN		0x00080000
> >
> >  #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
> >
> > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > index ab8567f32501..364e8b47d9ba 100644
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -1693,6 +1693,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc,
> struct irqaction *new)
> >  			irqd_set(&desc->irq_data, IRQD_NO_BALANCING);
> >  		}
> >
> > +		if (new->flags & IRQF_NO_AUTOEN)
> > +			irq_settings_set_noautoen(desc);
> 
> Can we make sure we refuse this request if the caller also specified
> IRQF_SHARED?

Right now, there is a warning for IRQF_SHARED + NOAUTOEN:

		if (irq_settings_can_autoenable(desc)) {
			irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
		} else {
			/*
			 * Shared interrupts do not go well with disabling
			 * auto enable. The sharing interrupt might request
			 * it while it's still disabled and then wait for
			 * interrupts forever.
			 */
			WARN_ON_ONCE(new->flags & IRQF_SHARED);
			/* Undo nested disables: */
			desc->depth = 1;
		}

Of course, this could also be clearly rejected in the sanity-check
of request_threaded_irq() if we want to totally prohibit this
behavior:

int request_threaded_irq(unsigned int irq, irq_handler_t handler,
			 irq_handler_t thread_fn, unsigned long irqflags,
			 const char *devname, void *dev_id)
{
	struct irqaction *action;
	struct irq_desc *desc;
	int retval;

	if (irq == IRQ_NOTCONNECTED)
		return -ENOTCONN;

	/*
	 * Sanity-check: shared interrupts must pass in a real dev-ID,
	 * otherwise we'll have trouble later trying to figure out
	 * which interrupt is which (messes up the interrupt freeing
	 * logic etc).
	 *
	 * Also IRQF_COND_SUSPEND only makes sense for shared interrupts and
	 * it cannot be set along with IRQF_NO_SUSPEND.
	 */
	if (((irqflags & IRQF_SHARED) && !dev_id) ||
	    (!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) ||
	    ((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND)))
		return -EINVAL;


> 
> Thanks.

Thanks
Barry