linux-kernel - Re: [PATCH 3/8] irq-imgpdc: add ImgTec PDC 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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 24 Apr 2013 11:32:30 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	James Hogan <james.hogan@...tec.com>
cc:	linux-kernel@...r.kernel.org,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	devicetree-discuss@...ts.ozlabs.org, Rob Landley <rob@...dley.net>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 3/8] irq-imgpdc: add ImgTec PDC irqchip driver

On Wed, 24 Apr 2013, James Hogan wrote:
> Thanks for the review Thomas!
> 
> On 23/04/13 16:09, Thomas Gleixner wrote:
> > On Tue, 23 Apr 2013, James Hogan wrote:
 >> +	spinlock_t		lock;
> > 
> >   raw_spinlock_t please
> 
> Okay.
> 
> If I understand right, this would be because on RT, spinlock_t becomes a
> mutex and won't work correctly with irqs actually disabled for the irq
> callbacks below, is that right?

Yep.
 
> I'll look into this. kernel/irq/generic-chip.c was added after this
> driver was written.

Fair enough.
 
> >> +static void pdc_intc_perip_isr(unsigned int irq, struct irq_desc *desc)
> >> +{
> >> +	struct pdc_intc_priv *priv;
> >> +	unsigned int i, irq_no;
> >> +
> >> +	priv = (struct pdc_intc_priv *)irq_desc_get_handler_data(desc);
> >> +
> >> +	/* find the peripheral number */
> >> +	for (i = 0; i < priv->nr_perips; ++i)
> >> +		if (irq == priv->perip_irqs[i])
> >> +			goto found;
> > 
> > Whee. Aren't these numbers linear ?
> 
> Not necessarily as they're virtual irq numbers obtained via
> platform_get_irq(), which come individually from device tree. Even their
> hardware irq numbers aren't linear as they're not wired to their irqchip
> in the same order:
> > pdc: pdc@...2006000 {
> > 	interrupt-controller;
> > 	#interrupt-cells = <3>;
> > 
> > 	reg = <0x02006000 0x1000>;
> > 	compatible = "img,pdc-intc";
> > 
> > 	num-perips = <3>;
> > 	num-syswakes = <3>;
> > 
> > 	interrupts = <18 4 /* level */>, /* Syswakes */
> > 	             <30 4 /* level */>, /* Perip 0 (RTC) */
> > 	             <29 4 /* level */>, /* Perip 1 (IR) */
> > 	             <31 4 /* level */>; /* Perip 2 (WDT) */
> > };

Interesting. 

> >> +static int pdc_intc_remove(struct platform_device *pdev)
> >> +{
> >> +	struct pdc_intc_priv *priv = platform_get_drvdata(pdev);
> >> +
> >> +	irq_domain_remove(priv->domain);
> > 
> > And the rest of the resources is still there?
> 
> I was under the impression devm_kzalloc and devm_ioremap took care of
> that in both the probe error case and the remove case:
 
> >  * devm_kzalloc - Resource-managed kzalloc
> >  * Managed kzalloc.  Memory allocated with this function is
> >  * automatically freed on driver detach. 
> > 
> >  * devm_ioremap - Managed ioremap()
> >  * Managed ioremap().  Map is automatically unmapped on driver detach.
> 
> I may have misunderstood the whole point of their existence though?

No, that was just me missing the devm_ in front of
kzalloc/ioremap. So you're good.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/