linux-kernel - RE: [PATCH 1/1 v1] leds: Add LED driver for lm3556 chip

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] [day] [month] [year] [list]

Date:	Tue, 22 May 2012 10:58:33 +0000
From:	"Jeong, Daniel" <Daniel.Jeong@...com>
To:	Johan Hovold <jhovold@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>
CC:	Richard Purdie <rpurdie@...ys.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Geon Si Jeong <gshark.jeong@...il.com>
Subject: RE: [PATCH 1/1 v1] leds: Add LED driver for lm3556 chip

Hi Andrew and Johan. 

Please drop it from current tree.
It will be replaced to "regmap"  and patched soon. 

Regards,
Daniel Jeong

-----Original Message-----
From: Johan Hovold [mailto:jhovold@...il.com] 
Sent: Tuesday, May 22, 2012 7:48 PM
To: Andrew Morton
Cc: Richard Purdie; Jeong, Daniel; linux-kernel@...r.kernel.org; Wolfram Sang; Geon Si Jeong
Subject: Re: [PATCH 1/1 v1] leds: Add LED driver for lm3556 chip

On Tue, May 15, 2012 at 08:43:30PM +0200, Johan Hovold wrote:
> On Mon, Apr 09, 2012 at 11:00:55AM +0900, Geon Si Jeong wrote:
> > It is a simple driver for LM3556 Chip(Texas Instruments)
> > LM3556 :
> > The LM3556 is a 4 MHz fixed-frequency synchronous boost converter 
> > plus 1.5A constant current driver for a high-current white LED.
> > Datasheet: www.national.com/ds/LM/LM3556.pdf
> > 
> > Tested on OMAP4430
> > 
> > Signed-off-by: Geon Si Jeong <gshark.jeong@...il.com>
> 
> [...]
> 
> > +/* i2c access*/
> > +static int lm3556_read_reg(struct i2c_client *client, u8 reg, u8 * 
> > +val) {
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "i2c reading fail at 0x%02x error %d\n",
> > +			reg, ret);
> > +		return ret;
> > +	}
> > +	*val = ret & 0xff;
> > +	return ret;
> > +}
> > +
> > +static int lm3556_write_reg(struct i2c_client *client, u8 reg, u8 
> > +val) {
> > +	int ret = 0;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > +
> > +	if (ret < 0)
> > +		dev_err(&client->dev, "i2c writting fail at 0x%02x\n", reg);
> > +	return ret;
> > +}
> > +
> > +static int lm3556_write_bits(struct i2c_client *client,
> > +			     u8 reg, u8 val, u8 mask, u8 shift) {
> > +	int ret;
> > +	u8 reg_val;
> > +	struct lm3556_chip_data *chip = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&chip->lock);
> > +	ret = lm3556_read_reg(client, reg, &reg_val);
> > +	if (ret < 0)
> > +		goto out;
> > +	reg_val &= (~(mask << shift));
> > +	reg_val |= ((val & mask) << shift);
> > +	ret = lm3556_write_reg(client, reg, reg_val);
> > +out:
> > +	mutex_unlock(&chip->lock);
> > +	return ret;
> > +}
> 
> The register io-locking above is broken. You need to protect both 
> write_reg and write_bits using the mutex (but you must restructure 
> your code because write_bits currently calls write_reg). This is 
> required to avoid register corruption due to concurrent write_reg and 
> write_bits.
> 
> As has been suggested elsewhere, you could consider using regmap.

Andrew, just wanted to remind you that this driver is still in your tree. Do you want to drop it or should someone else fix the locking?

Johan
--
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/