linux-kernel - Re: [PATCH V4] regulator: DA9211 : new regulator 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-next>] [day] [month] [year] [list]

Message-ID: <201407090828.s698SxPD017842@krsrvapps-01.diasemi.com>
Date:	Wed, 9 Jul 2014 17:28:59 +0900
From:	James Ban <james.ban.opensource@...semi.com>
To:	Mark Brown <broonie@...nel.org>
CC:	Liam Girdwood <lgirdwood@...il.com>,
	Support Opensource <support.opensource@...semi.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"David Dajun Chen" <david.chen@...semi.com>
Subject: Re: [PATCH V4] regulator: DA9211 : new regulator driver

> -----Original Message-----
> From: Mark Brown [mailto:broonie@...nel.org]
> Sent: Wednesday, July 09, 2014 4:57 PM
> To: Opensource [James Seong-Won Ban]
> Cc: Liam Girdwood; Support Opensource; LKML; David Dajun Chen
> Subject: Re: your mail
> 
> On Wed, Jul 09, 2014 at 10:03:32AM +0900, James Ban wrote:
> 
> > > > +	ret = regmap_read(chip->regmap, DA9211_REG_EVENT_B, &reg_val);
> > > > +	if (ret < 0)
> > > > +		goto error_i2c;
> 
> > > > +	if (reg_val & DA9211_E_OV_CURR_A) {
> 
> > > > +	if (reg_val & DA9211_E_OV_CURR_B) {
> 
> > > > +	return IRQ_HANDLED;
> 
> > > This is buggy - the driver should only return IRQ_HANDLED if it
> > > handled the interrupt somehow, otherwise it should return IRQ_NONE
> > > and let the interrupt core handle things.  This is especially
> > > important since the device appears to require that interrupts are
> > > explicitly acknoweldged so if something is flagged but not handled the
> interrupt will just sit constantly asserted.
> 
> > Basically all interrupts are masked when the chip wakes up.
> > Only two interrupts are unmasked at the start of driver like below.
> 
> I know that's the intention but the code should still be written robustly -
> something might go wrong somewhere which causes another interrupt to be
> enabled, or we might even gain support for shared threaded interrupts in the
> interrupt core and someone could then try to use that in a system.
How about below code for proper handle of interrupt?
static irqreturn_t da9211_irq_handler(int irq, void *data)
{
	struct da9211 *chip = data;
	int reg_val, ret;

	ret = regmap_read(chip->regmap, DA9211_REG_EVENT_B, &reg_val);
	if (ret < 0)
		goto error_i2c;

	if (reg_val & DA9211_E_OV_CURR_A) {
		regulator_notifier_call_chain(chip->rdev[0],
			REGULATOR_EVENT_OVER_CURRENT,
			rdev_get_drvdata(chip->rdev[0]));

		ret = regmap_write(chip->regmap, DA9211_REG_EVENT_B,
			DA9211_E_OV_CURR_A);
		if (ret < 0)
			goto error_i2c;

		return IRQ_HANDLED;
	}
	else if (reg_val & DA9211_E_OV_CURR_B) {
		regulator_notifier_call_chain(chip->rdev[1],
			REGULATOR_EVENT_OVER_CURRENT,
			rdev_get_drvdata(chip->rdev[1]));

		ret = regmap_write(chip->regmap, DA9211_REG_EVENT_B,
			DA9211_E_OV_CURR_B);
		if (ret < 0)
			goto error_i2c;

		return IRQ_HANDLED;
	}
	else
		return IRQ_NONE;

error_i2c:
	dev_err(chip->dev, "I2C error : %d\n", ret);
	return IRQ_NONE;
}
--
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/