linux-kernel - Re: [PATCH v5 04/14] spmi: pmic-arb: convert to v2 irq interfaces to support hierarchical IRQ chips

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]

Date:   Fri, 18 Jan 2019 13:12:38 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Brian Masney <masneyb@...tation.org>
Cc:     linus.walleij@...aro.org, sboyd@...nel.org,
        bjorn.andersson@...aro.org, andy.gross@...aro.org,
        shawnguo@...nel.org, dianders@...omium.org,
        linux-gpio@...r.kernel.org, nicolas.dechesne@...aro.org,
        niklas.cassel@...aro.org, david.brown@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, thierry.reding@...il.com,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v5 04/14] spmi: pmic-arb: convert to v2 irq interfaces to
 support hierarchical IRQ chips

On 18/01/2019 12:27, Brian Masney wrote:
> Hi Marc,
> 
> On Thu, Jan 17, 2019 at 11:22:59AM +0000, Marc Zyngier wrote:
>>> -static int qpnpint_irq_domain_map(struct irq_domain *d,
>>> -				  unsigned int virq,
>>> -				  irq_hw_number_t hwirq)
>>> -{
>>> -	struct spmi_pmic_arb *pmic_arb = d->host_data;
>>>  
>>> +static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb,
>>> +				   struct irq_domain *domain, unsigned int virq,
>>> +				   irq_hw_number_t hwirq)
>>> +{
>>>  	dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
>>>  
>>> -	irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
>>> -	irq_set_chip_data(virq, d->host_data);
>>> -	irq_set_noprobe(virq);
>>> +	irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb,
>>> +			    handle_level_irq, NULL, NULL);
>>
>> I understand you haven't changed the existing semantic here by always
>> setting the handler to handle_level_irq. But is that guaranteed to
>> always be the case? See below.
>>
>>> +}
>>> +
>>> +static int qpnpint_irq_domain_alloc(struct irq_domain *domain,
>>> +				    unsigned int virq, unsigned int nr_irqs,
>>> +				    void *data)
>>> +{
>>> +	struct spmi_pmic_arb *pmic_arb = domain->host_data;
>>> +	struct irq_fwspec *fwspec = data;
>>> +	irq_hw_number_t hwirq;
>>> +	unsigned int type;
>>> +	int ret, i;
>>> +
>>> +	ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type);
>>
>> Here, you extract the trigger from DT.
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	for (i = 0; i < nr_irqs; i++)
>>> +		qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i);
>>
>> Shouldn't you propagate it into the mapping function so that the handler
>> can be selected accordingly? Or does the interrupt controller convert
>> edge signals to level somehow?
> 
> qpnpint_irq_set_type() calls irq_set_handler_locked() to set the hander
> to be either handle_edge_irq() or handle_level_irq(). So the handler is
> initially setup incorrectly in some cases, but then setup correctly (via
> __irq_set_trigger) when __setup_irq() is called by
> request_threaded_irq().
> 
> It looks like that this will cause problems with shared IRQs to work as
> expected.
> 
> I can rework this code and get this fixed.

It'd definitely be worth it, thanks.

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