linux-kernel - Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver

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] [day] [month] [year] [list]

Date:   Wed, 23 Aug 2017 10:30:07 +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>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        devicetree@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2] irqchip: uniphier-aidet: add UniPhier AIDET irqchip driver

Hi Marc,

2017-08-22 17:20 GMT+09:00 Marc Zyngier <marc.zyngier@....com>:
> On 22/08/17 03:03, Masahiro Yamada wrote:
>> Hi Mark,
>>
>>
>> 2017-08-21 19:25 GMT+09:00 Marc Zyngier <marc.zyngier@....com>:
>>
>>>> +static struct irq_chip uniphier_aidet_irq_chip = {
>>>> +     .name = "AIDET",
>>>> +     .irq_mask = irq_chip_mask_parent,
>>>> +     .irq_unmask = irq_chip_unmask_parent,
>>>> +     .irq_eoi = irq_chip_eoi_parent,
>>>> +     .irq_set_type = uniphier_aidet_irq_set_type,
>>>
>>> Is this irqchip only used in a uniprocessor system? If not, how is the
>>> interrupt affinity managed without a irq_set_affinity callback?
>>>
>>
>>
>> After consideration, some questions popped up.
>>
>>
>>
>> We can set other hooks, for example, .irq_{enable,disable} if we like.
>>
>>          .irq_enable = irq_chip_enable_parent,
>>          .irq_disable = irq_chip_disable_parent,
>>
>>
>> I know the parent (GIC) implements unmask/mask instead of enable/disable,
>> but this is also out of the scope of this driver.
>>
>> I am not familiar with the difference between unmask/mask and enable/disable.
>> IIUC, the difference is that
>> if enable/disable hooks are missing, IRQs are masked lazily.
>
> That's a rather good thing. Disabling interrupts lazily is a net
> performance gain when you you have to repeatedly mask/unmask interrupts.
>
>> If a child irqchip implemented enable/disable,
>> IRQs would be masked immediately.  So, in irq-domain hierarchy,
>> a child irqchip need to have a good insight about its parent
>> which is be better, unmask/mask or enable/disable.
>
> Not necessarily. irq_chip_enable_parent will call unmask if enable is
> not implemented in the parent. But you'll loose the benefit of lazy
> masking of interrupts routed through this controller.

Right.
I will not add .irq_{enable/disable} to keep the benefit of lazy masking.




> There are many other things that are *much* worse, like the need to
> implement a irq_eoi callback even if the irqchip has no such concept.

Right. I noticed irq_eoi is mandatory when I tested my driver.

I notice handle_percpu_irq() and handle_percpu_devid_irq()
check the NULL pointer dereference like

        if (chip->irq_eoi)
                chip->irq_eoi(&desc->irq_data);

But, other flow handlers do not this.




>>> Nit: please use irq_domain_create_hierarchy.
>>
>> I'd like to know your intention about your commit
>> 2a5e9a072da6469a37d1f0b1577416f51223c280
>>
>> Is that mean, irq_domain_add_hierarchy will be deprecated
>> some time in the future?
>
> That's the intent, as we're moving towards a firmware-agnostic
> irq_domain layer. Think of it as a deprecated interface.
>
>> If I grep under drivers/irqchip/,
>> most drivers are currently using irq_domain_add_hierarchy(),
>> and this provides a shorter form for DT-based drivers.
> Yes, we have a lot of legacy, and I don't always catch new additions.

I see.

I hope all drivers will be converted and irq_domain_add_hierarchy() will be
deleted.

Having many variants with slight difference is confusing to driver developers.


-- 
Best Regards
Masahiro Yamada