linux-kernel - Re: [PATCH 1/2] irqchip: define EXYNOS_IRQ

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 9 Dec 2019 09:41:15 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Hyunki Koo <hyunki00.koo@...il.com>
Cc:     linux@...linux.org.uk, kgene@...nel.org, tglx@...utronix.de,
        jason@...edaemon.net, maz@...nel.org,
        linux-arm-kernel@...ts.infradead.org,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kernel-team@...roid.com, Hyunki Koo <hyunki00.koo@...sung.com>
Subject: Re: [PATCH 1/2] irqchip: define EXYNOS_IRQ_COMBINER

On Sun, Dec 08, 2019 at 07:24:49AM +0900, Hyunki Koo wrote:
> 
> On 19. 12. 7. 오후 10:37, Krzysztof Kozlowski wrote:
> > On Sat, 7 Dec 2019 at 14:28, Krzysztof Kozlowski <krzk@...nel.org> wrote:
> > > On Sat, Dec 07, 2019 at 10:00:48PM +0900, Hyunki Koo wrote:
> > > > From: Hyunki Koo <hyunki00.koo@...sung.com>
> > > > 
> > > > Not all exynos device have IRQ_COMBINER.
> > > > Thus add the config for EXYNOS_IRQ_COMBINER.
> > > > 
> > > > Signed-off-by: Hyunki Koo <hyunki00.koo@...sung.com>
> > > > ---
> > > >   drivers/irqchip/Kconfig  | 7 +++++++
> > > >   drivers/irqchip/Makefile | 2 +-
> > > >   2 files changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > Hi,
> > > 
> > > There is no changelog and versioning of this patch so I do not
> > > understand how it differs with previous. It's a resend? v2? It brings
> > > the confusion and looks like you're ignoring previous comments.
> > > 
> > > Looks the same and looks like breaking Exynos platforms in the same way.
> > > 
> > > If you not want to skip combiner on ARMv8, it makes sense, then please
> > > follow the approach we did for Pinctrl drivers (PINCTRL_EXYNOS_ARM and
> > > PINCTRL_EXYNOS_ARM64).
> > > 
> > > Best regards,
> > > Krzysztof
> > Ah, now I see the second patch. Still you break bisect which requires
> > specific ordering of patches or squashing them into one. Optionally
> > this could be default=y if ARCH_EXYNOS && ARM. I prefer just squashing
> > both into one patch in this case.
> 
> you mean squashing two files arch/arm/mach-exynos/Kconfig and
> arch/arm/mach-exynos/Kconfig into one patch
> 
> or squashing into only one file like blow?
> 
> +config EXYNOS_IRQ_COMBINER
> +       bool "Samsung Exynos IRQ combiner support"
> +       depends on (ARCH_EXYNOS  && ARM) || COMPILE_TEST
> +       default y
> 
> I prefer first one (squashing two files into one patch)

Squashing two patches into one.

> 
> > 
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index ba152954324b..3ed7b7f2ae26 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -499,4 +499,11 @@ config SIFIVE_PLIC
> > > > 
> > > >           If you don't know what to do here, say Y.
> > > > 
> > > > +config EXYNOS_IRQ_COMBINER
> > > > +     bool "Samsung Exynos IRQ combiner support"
> > Now point it to be visible. Only for COMPILE_TEST

Typo from my side. I wanted to say "No point" - this should not be
selectable by user.

> > 
> > > > +     depends on ARCH_EXYNOS
> > Since you make it a separate option, make it COMPILE_TEST.
> 
> Is this  good ?

Not entirely. The bool should be also with "if COMPILE TEST" so:

config EXYNOS_IRQ_COMBINER
	bool "Samsung Exynos IRQ combiner support" if COMPILE_TEST
	depends on (ARCH_EXYNOS && ARM) || COMPILE_TEST


Best regards,
Krzysztof