RE: [PATCH v3 01/12] genirq: add IRQF_NO_AUTOEN for request_irq
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Friday, January 29, 2021 8:55 AM
> To: Song Bao Hua (Barry Song) ;
> dmitry.torok...@gmail.com; m...@kernel.org; gre...@linuxfoundation.org;
> linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: linux...@openeuler.org; Song Bao Hua (Barry Song)
>
> Subject: Re: [PATCH v3 01/12] genirq: add IRQF_NO_AUTOEN for request_irq
>
> Barry,
>
> On Fri, Jan 08 2021 at 11:39, Barry Song wrote:
> > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > index ab8567f32501..2b28314e2572 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(>irq_data, IRQD_NO_BALANCING);
> > }
> >
> > + if (new->flags & IRQF_NO_AUTOEN)
> > + irq_settings_set_noautoen(desc);
>
> If we move this to request time flags, then setting the noautoen bit on
> the irq descriptor is pretty pointless. See below.
>
> I rather get rid of the irq_settings magic for NOAUTOEN completely.
Thanks for your comment, Thomas.
Got this issue fixed in v4:
https://lore.kernel.org/lkml/20210128223538.20272-1-song.bao@hisilicon.com/
btw, for those drivers which are using the first pattern:
irq_set_status_flags(irq, IRQ_NOAUTOEN);
request_irq(dev, irq...);
Simply running "git grep IRQ_NOAUTOEN" will help figure where to fix.
For those drivers which are using the second pattern:
request_irq(dev, irq...);
disable_irq(irq);
I wrote a script as below:
#!/bin/bash
if [ $# != 1 -o ! -d $1 ] ; then
echo "USAGE: $0 dir"
exit 1;
fi
find $1 -iname "*.c" | while read i
do
if [ -d "$i" ]; then
break
fi
irq=`grep -n -A 10 -E
"request_irq|request_threaded_irq|request_any_context_irq" $i | grep
disable_irq`
if [ "$irq" != "" ]; then
echo "$i":"$irq"
fi
done
The script says there are more than 70 cases in 5.11-rc6.
We are going to fix all of them after this one settles down.
Thanks
Barry
>
> Thanks,
>
> tglx
> ---
> --- 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 0x0080
> #define IRQF_PROBE_SHARED0x0100
> @@ -74,6 +76,7 @@
> #define IRQF_NO_THREAD 0x0001
> #define IRQF_EARLY_RESUME0x0002
> #define IRQF_COND_SUSPEND0x0004
> +#define IRQF_NO_AUTOEN 0x0008
>
> #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND |
> IRQF_NO_THREAD)
>
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1693,7 +1693,8 @@ static int
> irqd_set(>irq_data, IRQD_NO_BALANCING);
> }
>
> - if (irq_settings_can_autoenable(desc)) {
> + if (!(new->flags & IRQF_NO_AUTOEN) &&
> + irq_settings_can_autoenable(desc)) {
> irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
> } else {
> /*
> @@ -2086,10 +2087,15 @@ int request_threaded_irq(unsigned int ir
>* which interrupt is which (messes up the interrupt freeing
>* logic etc).
>*
> + * Also 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.
> + *
>* 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_NO_AUTOEN)) ||
> (!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) ||
> ((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND)))
> return -EINVAL;
Re: [PATCH v3 01/12] genirq: add IRQF_NO_AUTOEN for request_irq
Barry,
On Fri, Jan 08 2021 at 11:39, Barry Song wrote:
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index ab8567f32501..2b28314e2572 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(>irq_data, IRQD_NO_BALANCING);
> }
>
> + if (new->flags & IRQF_NO_AUTOEN)
> + irq_settings_set_noautoen(desc);
If we move this to request time flags, then setting the noautoen bit on
the irq descriptor is pretty pointless. See below.
I rather get rid of the irq_settings magic for NOAUTOEN completely.
Thanks,
tglx
---
--- 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_SHARED0x0080
#define IRQF_PROBE_SHARED 0x0100
@@ -74,6 +76,7 @@
#define IRQF_NO_THREAD 0x0001
#define IRQF_EARLY_RESUME 0x0002
#define IRQF_COND_SUSPEND 0x0004
+#define IRQF_NO_AUTOEN 0x0008
#define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND |
IRQF_NO_THREAD)
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1693,7 +1693,8 @@ static int
irqd_set(>irq_data, IRQD_NO_BALANCING);
}
- if (irq_settings_can_autoenable(desc)) {
+ if (!(new->flags & IRQF_NO_AUTOEN) &&
+ irq_settings_can_autoenable(desc)) {
irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
} else {
/*
@@ -2086,10 +2087,15 @@ int request_threaded_irq(unsigned int ir
* which interrupt is which (messes up the interrupt freeing
* logic etc).
*
+* Also 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.
+*
* 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_NO_AUTOEN)) ||
(!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) ||
((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND)))
return -EINVAL;
[PATCH v3 01/12] genirq: add IRQF_NO_AUTOEN for request_irq
Many drivers 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.
With this patch, drivers can request_irq with IRQF_NO_AUTOEN flag.
They will need neither irq_set_status_flags() nor disable_irq().
Hundreds of drivers with this problem will be handled afterwards.
Cc: Dmitry Torokhov
Signed-off-by: Barry Song
---
include/linux/interrupt.h | 3 +++
kernel/irq/manage.c | 8
kernel/irq/settings.h | 10 ++
3 files changed, 21 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_SHARED0x0080
#define IRQF_PROBE_SHARED 0x0100
@@ -74,6 +76,7 @@
#define IRQF_NO_THREAD 0x0001
#define IRQF_EARLY_RESUME 0x0002
#define IRQF_COND_SUSPEND 0x0004
+#define IRQF_NO_AUTOEN 0x0008
#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..2b28314e2572 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(>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 {
@@ -2086,10 +2089,15 @@ int request_threaded_irq(unsigned int irq,
irq_handler_t handler,
* which interrupt is which (messes up the interrupt freeing
* logic etc).
*
+* Also 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.
+*
* 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_NO_AUTOEN)) ||
(!(irqflags & IRQF_SHARED) && (irqflags & IRQF_COND_SUSPEND)) ||
((irqflags & IRQF_NO_SUSPEND) && (irqflags & IRQF_COND_SUSPEND)))
return -EINVAL;
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
