linux-kernel - Re: [PATCH] genirq: add IRQF_NO_AUTOEN for request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X/OePw97VgXtRBxj@google.com>
Date:   Mon, 4 Jan 2021 15:01:19 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Barry Song <song.bao.hua@...ilicon.com>
Cc:     tglx@...utronix.de, maz@...nel.org, gregkh@...uxfoundation.org,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        linuxarm@...neuler.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@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@...il.com>
> Signed-off-by: Barry Song <song.bao.hua@...ilicon.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