linux-kernel - Re: [PATCH 1/1] irqchip: Add config MTK_SYSIRQ and MTK

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: <f47e8a1e70c982ecf6245db573630f51@kernel.org>
Date:   Thu, 18 Jun 2020 13:41:39 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Hanks Chen <hanks.chen@...iatek.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        wsd_upstream@...iatek.com, CC Hwang <cc.hwang@...iatek.com>,
        Loda Chou <loda.chou@...iatek.com>
Subject: Re: [PATCH 1/1] irqchip: Add config MTK_SYSIRQ and MTK_CIRQ

On 2020-06-18 13:31, Hanks Chen wrote:
> Mediatek sysirq and cirq drivers as-is were bound together to the 
> config
> of ARCH_MEDIATEK.  These two drivers should be able to be configured
> separately.  For example, on new Mediatek mobile chips such as 
> Dimensity
> 820, the sysirq driver is not used since the hardware module is 
> removed.
> 
> Add two new configs to sysirq and cirq drivers.
> - config MTK_SYSIRQ for the interrupt polarity controller driver: 
> sysirq
> - config MTK_CIRQ for the low-power interrupt driver: cirq
> 
> Signed-off-by: cc.hwang <cc.hwang@...iatek.com>
> Signed-off-by: Hanks Chen <hanks.chen@...iatek.com>
> ---
>  drivers/irqchip/Kconfig  |   12 ++++++++++++
>  drivers/irqchip/Makefile |    3 ++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 29fead2..cc9aa18 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -572,4 +572,16 @@ config LOONGSON_PCH_MSI
>  	help
>  	  Support for the Loongson PCH MSI Controller.
> 
> +config MTK_SYSIRQ
> +	tristate "Mediatek interrupt polarity controller"

How do you expect this to work as a module?

> +	help
> +	  Interrupt polarity controller driver to swap polarity for
> +	  interrupts for Mediatek mobile chips.
> +
> +config MTK_CIRQ
> +	bool "Mediatek low-power interrupt controller"
> +	help
> +	  Low-power interrupt controller driver to monitor IRQS
> +	  in the sleep mode for Mediatek mobile chips.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 133f9c4..30421d2 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -69,7 +69,8 @@ obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
>  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
>  obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
> -obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o irq-mtk-cirq.o
> +obj-$(CONFIG_MTK_SYSIRQ)		+= irq-mtk-sysirq.o
> +obj-$(CONFIG_MTK_CIRQ)			+= irq-mtk-cirq.o
>  obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
>  obj-$(CONFIG_RENESAS_H8300H_INTC)	+= irq-renesas-h8300h.o
>  obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o

In general, this approach doesn't look right. As it stands, this is just 
breaking existing configurations

Do you really expect users to know exactly which interrupt controllers 
their system is going to use? This seems like the wrong assumption. If 
you really want to save the handful of bytes these drivers take in your 
image, then add the relevant dependency information in Kconfig.

         M.
-- 
Jazz is not dead. It just smells funny...