linux-kernel - Re: [PATCH 09/16] irqchip: Add RDA8810PL interrupt 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <86r2fgrwy4.wl-marc.zyngier@arm.com>
Date:   Tue, 20 Nov 2018 08:10:27 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     olof@...om.net, arnd@...db.de, robh+dt@...nel.org,
        tglx@...utronix.de, jason@...edaemon.net,
        daniel.lezcano@...aro.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        amit.kucheria@...aro.org, linus.walleij@...aro.org,
        zhao_steven@....net,
        Andreas Färber 
        <afaerber@...e.de>
Subject: Re: [PATCH 09/16] irqchip: Add RDA8810PL interrupt driver

[dropping the @rdamicro.com addresses, as they bounce...]

On Tue, 20 Nov 2018 03:19:58 +0000,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote:
> 
> Hi Marc,
> 
> Thanks for the quick review!
> 
> On Mon, Nov 19, 2018 at 05:36:49PM +0000, Marc Zyngier wrote:
> > Manivannan,
> > 
> > On 19/11/2018 17:09, Manivannan Sadhasivam wrote:

> > > +static int rda_intc_set_type(struct irq_data *data, unsigned int flow_type)
> > > +{
> > > +	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))
> > > +		irq_set_handler(data->irq, handle_edge_irq);
> > > +	if (flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> > > +		irq_set_handler(data->irq, handle_level_irq);
> > 
> > So you don't need to set anything in your interrupt controller for this
> > to switch between level and edge? That'd be a first...
> >
> 
> Interrupt controller can only handle level triggered interrupts. Should
> I just remove irq_set_type callback itself?

No, keep it, but return -EINVAL on anything that doesn't match what
the controller actually supports.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +struct irq_domain *rda_irq_domain;
> > 
> > static?
> > 
> 
> Ack.
> 
> > > +
> > > +static void __exception_irq_entry rda_handle_irq(struct pt_regs *regs)
> > > +{
> > > +	u32 stat = readl(base + RDA_INTC_FINALSTATUS);
> > > +	u32 hwirq;
> > > +
> > > +	while (stat) {
> > > +		hwirq = __fls(stat);
> > > +		handle_domain_irq(rda_irq_domain, hwirq, regs);
> > > +		stat &= ~(1 << hwirq);
> > > +	}
> > > +}
> > > +
> > > +static struct irq_chip rda_irq_chip = {
> > > +	.name		= "rda-intc",
> > > +	.irq_ack	= rda_intc_mask_irq,
> > 
> > You're joking, right? What does it mean to implement both ack as mask
> > when you already have mask?
> > 
> 
> Right, but I just followed what other drivers were doing (irq-sa11x0). Will
> remove it.

As usual, seeing something in another driver doesn't mean it is
right. Also, StrongARM is an interesting piece of history, and taking
inspiration from it is mostly a bad idea.

[...]

> > > +static int __init rda8810_intc_init(struct device_node *node,
> > > +				    struct device_node *parent)
> > > +{
> > > +	base = of_io_request_and_map(node, 0, "rda-intc");
> > > +	if (!base)
> > > +		return -ENXIO;
> > > +	/*
> > > +	 * Mask, and invalid all interrupt sources
> > > +	 */
> > > +	writel(RDA_IRQ_MASK_ALL, base + RDA_INTC_MASK_CLR);
> > > +
> > > +	rda_irq_domain = irq_domain_create_linear(&node->fwnode, RDA_NR_IRQS,
> > > +						  &rda_irq_domain_ops, base);
> > > +	WARN_ON(!rda_irq_domain);
> > 
> > Just WARN_ON(), and carry on? Please implement some error handling.
> > 
> 
> Sure. Which one would you recommend? Panic or returning -ENXIO?

Don't leak the IO space, return -ENOMEM.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.