linux-kernel - Re: [Qualcomm PM8921 MFD v2 2/6] mfd: pm8xxx: Add irq support

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: <alpine.LFD.2.00.1103101126100.2787@localhost6.localdomain6>
Date:	Thu, 10 Mar 2011 11:32:17 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Abhijeet Dharmapurikar <adharmap@...eaurora.org>
cc:	davidb@...eaurora.org, "David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	Daniel Walker <dwalker@...o99.com>,
	David Collins <collinsd@...eaurora.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Joe Perches <joe@...ches.com>,
	Russell King <linux@....linux.org.uk>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Stepan Moskovchenko <stepanm@...eaurora.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Linus Walleij <linux.walleij@...rricsson.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Qualcomm PM8921 MFD v2 2/6] mfd: pm8xxx: Add irq support

On Tue, 8 Mar 2011, Abhijeet Dharmapurikar wrote:
> Thomas Gleixner wrote:
> > On Mon, 7 Mar 2011, adharmap@...eaurora.org wrote:
> > > +	if (pdata->irq_pdata) {
> > 
> > So if pdata->irq_pdata == NULL you return success. Is that correct ?
> 
> Yes. The board configuration may choose not to use pmic interrupts.

Ok.
 
> > Also please return early on (!pdata->irq_pdata) and avoid that extra
> > indent level for the real code path.
> 
> I did not do that because there are other subdevices that I will be adding in
> the later patches. I cannot return early. well I will change it for this
> patch.

Maybe splitting out the various init subsections into different
functions which are called from here might be a good thing.
 
> > > +static void pm8xxx_irq_ack(struct irq_data *d)
> > > +{
> > > +	const struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> > > +	unsigned int pmirq = d->irq - chip->irq_base;
> > > +	u8	block, config;
> > > +
> > > +	block = pmirq / 8;
> > > +
> > > +	config = PM_IRQF_WRITE | chip->config[pmirq] | PM_IRQF_CLR;
> > > +	/* Keep the mask */
> > > +	if (!(chip->irqs_allowed[block] & (1 << (pmirq % 8))))
> > > +		config |= PM_IRQF_MASK_FE | PM_IRQF_MASK_RE;
> > 
> > What's the point of this exercise? ack is called before mask and it
> 
> The register design is such that we cannot only clear the interrupt. One has
> to write to the trigger bits while clearing it. Now trigger bits define
> whether the interrupt is masked or unmasked. If unmasked they define whether
> the interrupt rising/falling/level high/level low triggered.
> So the code remembers which interrupts are masked and for them it clears and
> rewrite the masked status in trigger bits. For unmasked ones it clears and
> writes to the trigger bits essentially configuring them same way as it was
> before. That is why the if satement to check interrupt was masked earlier,
> chip->irqs_allowed[] maintains which interrupt are unmasked.
> 
> > ack is called before mask and it
> > should never be called when the interrupt is masked.
> 
> I didnt quite understand this comment. handle_level_irq calls mask_ack which
> masks the interrupt and then acks it. In this case the ack is called after the

Indeed, sorry. So the right way to deal with that is to provide a
mask_ack() callback which does it in the correct order for your
HW. That way you avoid all the local state storage.

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/