linux-kernel - Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers

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: <CAK7LNARDB3fF8EBdRKV-Yv+nTrE4MzdOL3=gQrt6spRomtw+2Q@mail.gmail.com>
Date:   Wed, 26 Jul 2017 22:14:08 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers

2017-07-26 19:37 GMT+09:00 Marc Zyngier <marc.zyngier@....com>:
> On 26/07/17 11:18, Masahiro Yamada wrote:
>> Hi Marc,
>>
>>
>> 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@....com>:
>>> On 26/07/17 05:03, Masahiro Yamada wrote:
>>>> Some irqchip drivers have a Kconfig prompt.  When we run menuconfig
>>>> or friends, those drivers are directly listed in the "Device Drivers"
>>>> menu level.  This does not look nice.  Create a sub-system level menu.
>>>>
>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
>>>> ---
>>>>
>>>>  drivers/irqchip/Kconfig | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>> index f1fd5f44d1d4..7b66313a2952 100644
>>>> --- a/drivers/irqchip/Kconfig
>>>> +++ b/drivers/irqchip/Kconfig
>>>> @@ -1,3 +1,5 @@
>>>> +menu "IRQ chip support"
>>>> +
>>>>  config IRQCHIP
>>>>       def_bool y
>>>>       depends on OF_IRQ
>>>> @@ -306,3 +308,5 @@ config QCOM_IRQ_COMBINER
>>>>       help
>>>>         Say yes here to add support for the IRQ combiner devices embedded
>>>>         in Qualcomm Technologies chips.
>>>> +
>>>> +endmenu
>>>>
>>>
>>> I'm very reluctant to introduce this. IMHO, interrupt controllers are
>>> way too low level a thing to let them be selected by the user. They
>>> really should be selected by the platform that needs them
>>
>> This is true for the root irqchip.
>> Not necessarily true for child irqchips.
>
> I dispute that argument. We've been able to make this work so far
> *without* exposing yet another menu maze to the user. What has changed?


The irqchip maintainers applied drivers
with user-configurable Kconfig entries.




>>
>>
>>> Do you have any example in mind where having a user-selectable interrupt
>>> controller actually makes sense on its own?
>>
>> Yes.
>>
>> I see some user-selectable drivers in drivers/irqchip/Kconfig
>> and I'd like to add one more for my SoCs.
>>
>>
>> This patch:
>> https://github.com/uniphier/linux/commit/f39efdf0ce34f77ae9e324d9ec6c7f486f43a0ed
>>
>> This is really optional, so
>> I intentionally implemented it as a platform driver
>> instead of IRQCHIP_DECLARE().
>
> I really cannot see how this could be optional. It means that you could
> end-up in a situation where the drivers for the devices being this
> irqchip could have been compiled in, but not their interrupt controller.
> How useful is that?

In my case, the assumed irq consumer is GPIO.

If the irq consumer is probed before the irqchip,
it will be tried later by -EPROBE_DEFER.

If the irqchip is not compiled at all, right, the irq consumer will not work.
One possible (and general) solution is to specify "depends on" correctly
between the provider and the consumer.



>> Looks like irq-ts4800.c, irq-keystone.c are modules as well.
>
> They are directly selected by their respective defconfig.


Are you sure?

As far as I see, they are not selected by anyone.


$ git grep 'TS4800_IRQ\|KEYSTONE_IRQ'
arch/arm/configs/keystone_defconfig:CONFIG_KEYSTONE_IRQ=y
arch/arm/configs/multi_v7_defconfig:CONFIG_KEYSTONE_IRQ=y
drivers/irqchip/Kconfig:config TS4800_IRQ
drivers/irqchip/Kconfig:config KEYSTONE_IRQ
drivers/irqchip/Makefile:obj-$(CONFIG_TS4800_IRQ)               += irq-ts4800.o
drivers/irqchip/Makefile:obj-$(CONFIG_KEYSTONE_IRQ)             +=
irq-keystone.o



defconfig just provides a default value.

Users are allowed to disable the option from menuconfig.




> On arm64,
> which is what I expect you driver targets, you should simply select it
> in your platform entry.

OK, assuming your clain is correct,
we have 5 suspicious entries in drivers/irqchip/Kconfig.


config JCORE_AIC
        bool "J-Core integrated AIC" if COMPILE_TEST

config TS4800_IRQ
        tristate "TS-4800 IRQ controller"

config KEYSTONE_IRQ
        tristate "Keystone 2 IRQ controller IP"

config EZNPS_GIC
        bool "NPS400 Global Interrupt Manager (GIM)"

config QCOM_IRQ_COMBINER
        bool "QCOM IRQ combiner support"



The prompt strings make the entries visible in menuconfig.
So, they should be removed.
The prompts are pointless if the options are supposed by selected by others.

Also, tristate is pointless.
If they are supposed to be selected by platforms,
they have no chance to be a module.
They should be turned into bool (without prompt)

Is this what you mean?



-- 
Best Regards
Masahiro Yamada