* [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:34 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).