All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq
@ 2021-01-04 22:26 Barry Song
  2021-01-04 23:01 ` Dmitry Torokhov
  0 siblings, 1 reply; 3+ messages in thread
From: Barry Song @ 2021-01-04 22:26 UTC (permalink / raw)
  To: tglx, maz, gregkh, linux-input, linux-kernel
  Cc: linuxarm, Barry Song, Dmitry Torokhov

This patch originated from the discussion with Dmitry in the below thread:
https://lore.kernel.org/linux-input/20210102042902.41664-1-song.bao.hua@hisilicon.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.

If this one is accepted, hundreds of drivers with this problem will be
handled afterwards.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 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);
+
 		if (irq_settings_can_autoenable(desc)) {
 			irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
 		} else {
diff --git a/kernel/irq/settings.h b/kernel/irq/settings.h
index 403378b9947b..a28958a9c548 100644
--- a/kernel/irq/settings.h
+++ b/kernel/irq/settings.h
@@ -145,6 +145,16 @@ static inline bool irq_settings_can_move_pcntxt(struct irq_desc *desc)
 	return desc->status_use_accessors & _IRQ_MOVE_PCNTXT;
 }
 
+static inline void irq_settings_clr_noautoen(struct irq_desc *desc)
+{
+	desc->status_use_accessors &= ~_IRQ_NOAUTOEN;
+}
+
+static inline void irq_settings_set_noautoen(struct irq_desc *desc)
+{
+	desc->status_use_accessors |= _IRQ_NOAUTOEN;
+}
+
 static inline bool irq_settings_can_autoenable(struct irq_desc *desc)
 {
 	return !(desc->status_use_accessors & _IRQ_NOAUTOEN);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq
  2021-01-04 22:26 [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq Barry Song
@ 2021-01-04 23:01 ` Dmitry Torokhov
  2021-01-05  1:33   ` Song Bao Hua (Barry Song)
  0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2021-01-04 23:01 UTC (permalink / raw)
  To: Barry Song; +Cc: tglx, maz, gregkh, linux-input, linux-kernel, linuxarm

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@hisilicon.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@gmail.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> ---
>  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?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq
  2021-01-04 23:01 ` Dmitry Torokhov
@ 2021-01-05  1:33   ` Song Bao Hua (Barry Song)
  0 siblings, 0 replies; 3+ messages in thread
From: Song Bao Hua (Barry Song) @ 2021-01-05  1:33 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: tglx, maz, gregkh, linux-input, linux-kernel, linuxarm



> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Tuesday, January 5, 2021 12:01 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: tglx@linutronix.de; maz@kernel.org; gregkh@linuxfoundation.org;
> linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> 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@gmail.com>
> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> > ---
> >  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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-01-05  1:33 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 22:26 [PATCH] genirq: add IRQF_NO_AUTOEN for request_irq Barry Song
2021-01-04 23:01 ` Dmitry Torokhov
2021-01-05  1:33   ` Song Bao Hua (Barry Song)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.