linux-kernel - Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

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: <02e2a5f9-8ecc-76b1-a3cc-c95b215e8fe1@prevas.dk>
Date:   Fri, 23 Feb 2018 13:16:08 +0100
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Marc Zyngier <marc.zyngier@....com>,
        Lina Iyer <ilina@...eaurora.org>, <tglx@...utronix.de>,
        <jason@...edaemon.net>
CC:     <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <sboyd@...eaurora.org>, <rnayak@...eaurora.org>,
        <asathyak@...eaurora.org>
Subject: Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt
 controller for QCOM SoCs

On 2018-02-02 15:58, Marc Zyngier wrote:
> Hi Lina,
> 
> On 02/02/18 14:21, Lina Iyer wrote:
>> From : Archana Sathyakumar <asathyak@...eaurora.org>
>>
>> +
>> +static int qcom_pdc_translate(struct irq_domain *d,
>> +	struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type)
>> +{
>> +	if (is_of_node(fwspec->fwnode)) {
>> +		if (fwspec->param_count < 3)
>> +			return -EINVAL;
> 
> Why 3? Reading the DT binding, this is indeed set to 3 without any
> reason. I'd suggest this becomes 2, encoding the pin number and the
> trigger information, as the leading 0 is quite useless. Yes, I know
> other examples in the kernel are using this 0, and that was a
> consequence of retrofitting the omitted interrupt controllers (back in
> the days of the stupid gic_arch_extn...). Don't do that.
> 

Hi Marc

I'm about to send out a new revision of the ls-extirq patchset, and
thanks to you pointing me to these patches, I've read the comments on
the various revisions of this series and tried to take those into
account. But the above confused me, because in response to my first RFC
(https://patchwork.kernel.org/patch/10102643/) we have

On 2017-12-08 17:09, Marc Zyngier wrote:
> On 08/12/17 15:11, Alexander Stein wrote:
>> Hi Rasmus,
>>
>>> +
>>> +Required properties:
>>> +- compatible: should be "fsl,ls1021a-extirq"
>>> +- interrupt-controller: Identifies the node as an interrupt controller
>>> +- #interrupt-cells: Use the same format as specified by GIC in
arm,gic.txt.
>>
>> Do you really need 3 interrupt-cells here? As you've written below
you don't
>> support PPI anyway the 1st flag might be dropped then. So support
just 2 cells:
>> * IRQ number (IRQ0 - IRQ5)
>> * IRQ flags
>
> The convention for irqchip stacked on top of a GIC is to keep the
> interrupt specifier the same. It makes the maintenance if the DT much
> easier, and doesn't hurt at all.

Personally, I'd actually prefer the simpler interrupt specifiers without
a redundant 0. Maybe I'm just missing some difference between this case
and the ls-extirq one?

Thanks,
Rasmus