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
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(-)
diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index c0cff64..6c5eac0 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -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;
}
--
1.7.4.4
--------------------------
Raghavendra Prabhu
GPG Id : D72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
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(-)
diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index 6c5eac0..f03a524 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -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;
}
--
1.7.4.4
--------------------------
Raghavendra Prabhu
GPG Id : D72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
On Sat, Apr 16, 2011 at 02:13:13PM +0530, Raghavendra D Prabhu wrote:
> 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(-)
>
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index c0cff64..6c5eac0 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -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;
> }
> --
> 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