Fix for unintialized variables

PATCH New Raghavendra D Prabhu 2 participants 4 messages Apr 16, 2011 – Apr 18, 2011 LED

Share & tools

Raghavendra D Prabhu

author

2011-04-16 08:37 UTC #0 lore raw

PATCH 1/1 Fix for unintialized variables

Hi,
I have attached (as reply) two patches to one of the drivers in led subsytem. The return value was not being checked before proceeding leading to uninitialized values being passed in few places. In the second patch, I am a bit unclear of the logic in the function being patched. So modify it if required.
--------------------------
Raghavendra Prabhu
GPG Id : D72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977

Raghavendra D Prabhu

author

2011-04-16 08:43 UTC #1 lore raw

PATCH 1/2 FIX: engine_state is uninitialized

In case lp5521_read() returns EIO, engine_state is in an uninitialized state. This adds checks to fix that.

Signed-off-by Raghavendra D Prabhu <[email protected]>

---
 drivers/leds/leds-lp5521.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

drivers/leds/leds-lp5521.c +6 -4

index c0cff64..6c5eac0 100644
@@ -176,11 +176,13 @@ static int lp5521_set_engine_mode(struct lp5521_engine *engine, u8 mode)
 
 	ret = lp5521_read(client, LP5521_REG_OP_MODE, &engine_state);
 
+	if (ret == 0){
 	/* set mode only for this engine */
-	engine_state &= ~(engine->engine_mask);
-	mode &= engine->engine_mask;
-	engine_state |= mode;
-	ret |= lp5521_write(client, LP5521_REG_OP_MODE, engine_state);
+		engine_state &= ~(engine->engine_mask);
+		mode &= engine->engine_mask;
+		engine_state |= mode;
+		ret |= lp5521_write(client, LP5521_REG_OP_MODE, engine_state);
+	}
 
 	return ret;
 }

Signature · 6 lines

1.7.4.4

--------------------------
Raghavendra Prabhu
GPG Id : D72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977

Jonathan Neuschäfer

2011-04-18 11:21 UTC #2 lore raw

Re: [PATCH 1/2] FIX: engine_state is uninitialized

Quoted 28 lines · Raghavendra D Prabhu

In case lp5521_read() returns EIO, engine_state is in an uninitialized state. This adds checks to fix that.

Signed-off-by Raghavendra D Prabhu <[email protected]>

---
 drivers/leds/leds-lp5521.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

drivers/leds/leds-lp5521.c +6 -5

index c0cff64..6c5eac0 100644
@@ -176,11 +176,13 @@ static int lp5521_set_engine_mode(struct lp5521_engine *engine, u8 mode)
 	ret = lp5521_read(client, LP5521_REG_OP_MODE, &engine_state);
+	if (ret == 0){
 	/* set mode only for this engine */
-	engine_state &= ~(engine->engine_mask);
-	mode &= engine->engine_mask;
-	engine_state |= mode;
-	ret |= lp5521_write(client, LP5521_REG_OP_MODE, engine_state);
+		engine_state &= ~(engine->engine_mask);
+		mode &= engine->engine_mask;
+		engine_state |= mode;
+		ret |= lp5521_write(client, LP5521_REG_OP_MODE, engine_state);
+	}
 	return ret;
 }
--

Quoted 1 line

1.7.4.4

Usually something like

	if (ret < 0)
		goto out;

	/* some code here */

out:

	return ret;

is preferred to avoid unnecessary nesting/indentation. Anyway, thanks for the patch (although I'm not the maintainer, but just commenting).

Thanks,
Jonathan Neuschäfer

Raghavendra D Prabhu

author

2011-04-16 08:44 UTC #3 lore raw

PATCH 2/2 FIX: Check the return value before proceeding

In reply to Raghavendra D Prabhu

In case of failure from lp5521_read, an uninitialized mode is passed to lp5521_write. This adds checks to prevent that.

Signed-off-by Raghavendra D Prabhu <[email protected]>

---
 drivers/leds/leds-lp5521.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

drivers/leds/leds-lp5521.c +22 ~~-18~~

index 6c5eac0..f03a524 100644
@@ -176,7 +176,7 @@ static int lp5521_set_engine_mode(struct lp5521_engine *engine, u8 mode)
 
 	ret = lp5521_read(client, LP5521_REG_OP_MODE, &engine_state);
 
-	if (ret == 0){
+	if (!ret){
 	/* set mode only for this engine */
 		engine_state &= ~(engine->engine_mask);
 		mode &= engine->engine_mask;

@@ -192,6 +192,7 @@ static int lp5521_load_program(struct lp5521_engine *eng, const u8 *pattern)
 	struct lp5521_chip *chip = engine_to_lp5521(eng);
 	struct i2c_client *client = chip->client;
 	int ret;
+	int ret_read;
 	int addr;
 	u8 mode;

@@ -199,23 +200,26 @@ static int lp5521_load_program(struct lp5521_engine *eng, const u8 *pattern)
 	ret = lp5521_set_engine_mode(eng, LP5521_CMD_DIRECT);
 	/* Mode change requires min 500 us delay. 1 - 2 ms  with margin */
 	usleep_range(1000, 2000);
-	ret |= lp5521_read(client, LP5521_REG_OP_MODE, &mode);
-
-	/* For loading, all the engines to load mode */
-	lp5521_write(client, LP5521_REG_OP_MODE, LP5521_CMD_DIRECT);
-	/* Mode change requires min 500 us delay. 1 - 2 ms  with margin */
-	usleep_range(1000, 2000);
-	lp5521_write(client, LP5521_REG_OP_MODE, LP5521_CMD_LOAD);
-	/* Mode change requires min 500 us delay. 1 - 2 ms  with margin */
-	usleep_range(1000, 2000);
-
-	addr = LP5521_PROG_MEM_BASE + eng->prog_page * LP5521_PROG_MEM_SIZE;
-	i2c_smbus_write_i2c_block_data(client,
-				addr,
-				LP5521_PROG_MEM_SIZE,
-				pattern);
-
-	ret |= lp5521_write(client, LP5521_REG_OP_MODE, mode);
+	ret_read = lp5521_read(client, LP5521_REG_OP_MODE, &mode);
+	ret |= ret_read;
+
+	if (!ret_read){
+		/* For loading, all the engines to load mode */
+		lp5521_write(client, LP5521_REG_OP_MODE, LP5521_CMD_DIRECT);
+		/* Mode change requires min 500 us delay. 1 - 2 ms  with margin */
+		usleep_range(1000, 2000);
+		lp5521_write(client, LP5521_REG_OP_MODE, LP5521_CMD_LOAD);
+		/* Mode change requires min 500 us delay. 1 - 2 ms  with margin */
+		usleep_range(1000, 2000);
+
+		addr = LP5521_PROG_MEM_BASE + eng->prog_page * LP5521_PROG_MEM_SIZE;
+		i2c_smbus_write_i2c_block_data(client,
+					addr,
+					LP5521_PROG_MEM_SIZE,
+					pattern);
+
+		ret |= lp5521_write(client, LP5521_REG_OP_MODE, mode);
+	}
 	return ret;
 }

Signature · 6 lines

1.7.4.4

--------------------------
Raghavendra Prabhu
GPG Id : D72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977