All of lore.kernel.org
 help / color / mirror / Atom feed
* Broken locking in leds-lp5523.c
@ 2014-01-07 20:42 ` Pavel Machek
  0 siblings, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2014-01-07 20:42 UTC (permalink / raw)
  To: cooloney, rpurdie, linux-leds, milo.kim; +Cc: pali.rohar, sre, sre, kernel list

Hi!

There's some locking weirdness, and few missing comments in lp5523
driver.

Now, this is untested patch from my reverse-engineering. I hope I
understood things right...

In particular, there's unbalanced unlock in
lp5523_update_program_memory, and lp5523_update_program_memory needs
to be protected by the lock.

Comments? Does someone maintain this?

Thanks,
								Pavel
Signed-off-by: Pavel Machek <pavel@ucw.cz>


diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index c00f55e4..e8b83e9 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -34,7 +34,15 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP5523_PROGRAM_LENGTH		32
+#define LP5523_PROGRAM_LENGTH		32	/* bytes */
+/* Memory is used like this:
+   0x00 engine 1 program
+   0x10 engine 2 program
+   0x20 engine 3 program
+   0x30 engine 1 muxing info
+   0x40 engine 2 muxing info
+   0x50 engine 3 muxing info
+   ...and offsets are hard-coded all around :-( */
 #define LP5523_MAX_LEDS			9
 
 /* Registers */
@@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	/* one pattern per engine setting LED MUX start and stop addresses */
 	static const u8 pattern[][LP5523_PROGRAM_LENGTH] =  {
 		{ 0x9c, 0x30, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
+		/* 9c30 -- mux_map_start(0x30)
+		   9cb0 -- mux_ld_end(0x50)
+		   9d80 -- mux_sel???(0x00)
+		   d800 -- invalid?? */
 		{ 0x9c, 0x40, 0x9c, 0xc0, 0x9d, 0x80, 0xd8, 0x00, 0},
 		{ 0x9c, 0x50, 0x9c, 0xd0, 0x9d, 0x80, 0xd8, 0x00, 0},
 	};
 
-	/* hardcode 32 bytes of memory for each engine from program memory */
+	/* hardcode LP5523_PROGRAM_LENGTH bytes of memory for each
+	   engine from program memory */
 	ret = lp55xx_write(chip, LP5523_REG_CH1_PROG_START, 0x00);
 	if (ret)
 		return ret;
 
-	ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, 0x10);
+	ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
 	if (ret)
 		return ret;
 
-	ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
+	ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
 	if (ret)
 		return ret;
 
@@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
 
 	for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
 		ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
-		if (ret) {
-			mutex_unlock(&chip->lock);
+		if (ret)
 			return -EINVAL;
-		}
 	}
 
 	return size;
@@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
 {
 	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
 	struct lp55xx_chip *chip = led->chip;
+	ssize_t res;
 
 	mutex_lock(&chip->lock);
 
 	chip->engine_idx = nr;
 	lp5523_load_engine_and_select_page(chip);
 
+	res = lp5523_update_program_memory(chip, buf, len);
 	mutex_unlock(&chip->lock);
 
-	return lp5523_update_program_memory(chip, buf, len);
+	return res;
 }
 store_load(1)
 store_load(2)
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index 8d55a780..ae5c6fe 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
 	}
 
 	/*
-	 * Program momery sequence
+	 * Program memory sequence
 	 *  1) set engine mode to "LOAD"
 	 *  2) write firmware data into program memory
 	 */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Broken locking in leds-lp5523.c
@ 2014-01-07 20:42 ` Pavel Machek
  0 siblings, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2014-01-07 20:42 UTC (permalink / raw)
  To: cooloney, rpurdie, linux-leds, linux-kernel, milo.kim
  Cc: pali.rohar, sre, sre, kernel list

Hi!

There's some locking weirdness, and few missing comments in lp5523
driver.

Now, this is untested patch from my reverse-engineering. I hope I
understood things right...

In particular, there's unbalanced unlock in
lp5523_update_program_memory, and lp5523_update_program_memory needs
to be protected by the lock.

Comments? Does someone maintain this?

Thanks,
								Pavel
Signed-off-by: Pavel Machek <pavel@ucw.cz>


diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index c00f55e4..e8b83e9 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -34,7 +34,15 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP5523_PROGRAM_LENGTH		32
+#define LP5523_PROGRAM_LENGTH		32	/* bytes */
+/* Memory is used like this:
+   0x00 engine 1 program
+   0x10 engine 2 program
+   0x20 engine 3 program
+   0x30 engine 1 muxing info
+   0x40 engine 2 muxing info
+   0x50 engine 3 muxing info
+   ...and offsets are hard-coded all around :-( */
 #define LP5523_MAX_LEDS			9
 
 /* Registers */
@@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
 	/* one pattern per engine setting LED MUX start and stop addresses */
 	static const u8 pattern[][LP5523_PROGRAM_LENGTH] =  {
 		{ 0x9c, 0x30, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
+		/* 9c30 -- mux_map_start(0x30)
+		   9cb0 -- mux_ld_end(0x50)
+		   9d80 -- mux_sel???(0x00)
+		   d800 -- invalid?? */
 		{ 0x9c, 0x40, 0x9c, 0xc0, 0x9d, 0x80, 0xd8, 0x00, 0},
 		{ 0x9c, 0x50, 0x9c, 0xd0, 0x9d, 0x80, 0xd8, 0x00, 0},
 	};
 
-	/* hardcode 32 bytes of memory for each engine from program memory */
+	/* hardcode LP5523_PROGRAM_LENGTH bytes of memory for each
+	   engine from program memory */
 	ret = lp55xx_write(chip, LP5523_REG_CH1_PROG_START, 0x00);
 	if (ret)
 		return ret;
 
-	ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, 0x10);
+	ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
 	if (ret)
 		return ret;
 
-	ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
+	ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
 	if (ret)
 		return ret;
 
@@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
 
 	for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
 		ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
-		if (ret) {
-			mutex_unlock(&chip->lock);
+		if (ret)
 			return -EINVAL;
-		}
 	}
 
 	return size;
@@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
 {
 	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
 	struct lp55xx_chip *chip = led->chip;
+	ssize_t res;
 
 	mutex_lock(&chip->lock);
 
 	chip->engine_idx = nr;
 	lp5523_load_engine_and_select_page(chip);
 
+	res = lp5523_update_program_memory(chip, buf, len);
 	mutex_unlock(&chip->lock);
 
-	return lp5523_update_program_memory(chip, buf, len);
+	return res;
 }
 store_load(1)
 store_load(2)
diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
index 8d55a780..ae5c6fe 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
 	}
 
 	/*
-	 * Program momery sequence
+	 * Program memory sequence
 	 *  1) set engine mode to "LOAD"
 	 *  2) write firmware data into program memory
 	 */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: Broken locking in leds-lp5523.c
  2014-01-07 20:42 ` Pavel Machek
  (?)
@ 2014-01-07 23:57 ` Bryan Wu
  2014-01-09 23:08   ` Pavel Machek
  -1 siblings, 1 reply; 10+ messages in thread
From: Bryan Wu @ 2014-01-07 23:57 UTC (permalink / raw)
  To: Pavel Machek, milo kim
  Cc: rpurdie, Linux LED Subsystem, lkml, Pali Rohár,
	Sebastian Reichel, Sebastian Reichel

On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> There's some locking weirdness, and few missing comments in lp5523
> driver.
>
> Now, this is untested patch from my reverse-engineering. I hope I
> understood things right...
>
> In particular, there's unbalanced unlock in
> lp5523_update_program_memory, and lp5523_update_program_memory needs
> to be protected by the lock.
>
> Comments? Does someone maintain this?
>

I think Milo Kim is maintaining this driver for the LP55xx chip family.

Thanks,
-Bryan

> Thanks,
>                                                                 Pavel
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index c00f55e4..e8b83e9 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -34,7 +34,15 @@
>
>  #include "leds-lp55xx-common.h"
>
> -#define LP5523_PROGRAM_LENGTH          32
> +#define LP5523_PROGRAM_LENGTH          32      /* bytes */
> +/* Memory is used like this:
> +   0x00 engine 1 program
> +   0x10 engine 2 program
> +   0x20 engine 3 program
> +   0x30 engine 1 muxing info
> +   0x40 engine 2 muxing info
> +   0x50 engine 3 muxing info
> +   ...and offsets are hard-coded all around :-( */
>  #define LP5523_MAX_LEDS                        9
>
>  /* Registers */
> @@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
>         /* one pattern per engine setting LED MUX start and stop addresses */
>         static const u8 pattern[][LP5523_PROGRAM_LENGTH] =  {
>                 { 0x9c, 0x30, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
> +               /* 9c30 -- mux_map_start(0x30)
> +                  9cb0 -- mux_ld_end(0x50)
> +                  9d80 -- mux_sel???(0x00)
> +                  d800 -- invalid?? */
>                 { 0x9c, 0x40, 0x9c, 0xc0, 0x9d, 0x80, 0xd8, 0x00, 0},
>                 { 0x9c, 0x50, 0x9c, 0xd0, 0x9d, 0x80, 0xd8, 0x00, 0},
>         };
>
> -       /* hardcode 32 bytes of memory for each engine from program memory */
> +       /* hardcode LP5523_PROGRAM_LENGTH bytes of memory for each
> +          engine from program memory */
>         ret = lp55xx_write(chip, LP5523_REG_CH1_PROG_START, 0x00);
>         if (ret)
>                 return ret;
>
> -       ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, 0x10);
> +       ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
>         if (ret)
>                 return ret;
>
> -       ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
> +       ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
>         if (ret)
>                 return ret;
>
> @@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
>
>         for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
>                 ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
> -               if (ret) {
> -                       mutex_unlock(&chip->lock);
> +               if (ret)
>                         return -EINVAL;
> -               }
>         }
>
>         return size;
> @@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
>  {
>         struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
>         struct lp55xx_chip *chip = led->chip;
> +       ssize_t res;
>
>         mutex_lock(&chip->lock);
>
>         chip->engine_idx = nr;
>         lp5523_load_engine_and_select_page(chip);
>
> +       res = lp5523_update_program_memory(chip, buf, len);
>         mutex_unlock(&chip->lock);
>
> -       return lp5523_update_program_memory(chip, buf, len);
> +       return res;
>  }
>  store_load(1)
>  store_load(2)
> diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
> index 8d55a780..ae5c6fe 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
>         }
>
>         /*
> -        * Program momery sequence
> +        * Program memory sequence
>          *  1) set engine mode to "LOAD"
>          *  2) write firmware data into program memory
>          */
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: Broken locking in leds-lp5523.c
  2014-01-07 20:42 ` Pavel Machek
  (?)
  (?)
@ 2014-01-08  0:32 ` Bryan Wu
  2014-01-09 14:24   ` Pali Rohár
  2014-01-09 22:45   ` Pavel Machek
  -1 siblings, 2 replies; 10+ messages in thread
From: Bryan Wu @ 2014-01-08  0:32 UTC (permalink / raw)
  To: Pavel Machek
  Cc: rpurdie, Linux LED Subsystem, lkml, milo kim, Pali Rohár,
	Sebastian Reichel, Sebastian Reichel

On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> There's some locking weirdness, and few missing comments in lp5523
> driver.
>
> Now, this is untested patch from my reverse-engineering. I hope I
> understood things right...
>
> In particular, there's unbalanced unlock in
> lp5523_update_program_memory, and lp5523_update_program_memory needs
> to be protected by the lock.
>
> Comments? Does someone maintain this?
>

Actually the locking issue was fixed in my for-next branch,
http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403795be9

Thanks,
-Bryan

> Thanks,
>                                                                 Pavel
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index c00f55e4..e8b83e9 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -34,7 +34,15 @@
>
>  #include "leds-lp55xx-common.h"
>
> -#define LP5523_PROGRAM_LENGTH          32
> +#define LP5523_PROGRAM_LENGTH          32      /* bytes */
> +/* Memory is used like this:
> +   0x00 engine 1 program
> +   0x10 engine 2 program
> +   0x20 engine 3 program
> +   0x30 engine 1 muxing info
> +   0x40 engine 2 muxing info
> +   0x50 engine 3 muxing info
> +   ...and offsets are hard-coded all around :-( */
>  #define LP5523_MAX_LEDS                        9
>
>  /* Registers */
> @@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
>         /* one pattern per engine setting LED MUX start and stop addresses */
>         static const u8 pattern[][LP5523_PROGRAM_LENGTH] =  {
>                 { 0x9c, 0x30, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
> +               /* 9c30 -- mux_map_start(0x30)
> +                  9cb0 -- mux_ld_end(0x50)
> +                  9d80 -- mux_sel???(0x00)
> +                  d800 -- invalid?? */
>                 { 0x9c, 0x40, 0x9c, 0xc0, 0x9d, 0x80, 0xd8, 0x00, 0},
>                 { 0x9c, 0x50, 0x9c, 0xd0, 0x9d, 0x80, 0xd8, 0x00, 0},
>         };
>
> -       /* hardcode 32 bytes of memory for each engine from program memory */
> +       /* hardcode LP5523_PROGRAM_LENGTH bytes of memory for each
> +          engine from program memory */
>         ret = lp55xx_write(chip, LP5523_REG_CH1_PROG_START, 0x00);
>         if (ret)
>                 return ret;
>
> -       ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, 0x10);
> +       ret = lp55xx_write(chip, LP5523_REG_CH2_PROG_START, LP5523_PROGRAM_LENGTH / 2);
>         if (ret)
>                 return ret;
>
> -       ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, 0x20);
> +       ret = lp55xx_write(chip, LP5523_REG_CH3_PROG_START, LP5523_PROGRAM_LENGTH);
>         if (ret)
>                 return ret;
>
> @@ -346,10 +359,8 @@ static int lp5523_update_program_memory(struct lp55xx_chip *chip,
>
>         for (i = 0; i < LP5523_PROGRAM_LENGTH; i++) {
>                 ret = lp55xx_write(chip, LP5523_REG_PROG_MEM + i, pattern[i]);
> -               if (ret) {
> -                       mutex_unlock(&chip->lock);
> +               if (ret)
>                         return -EINVAL;
> -               }
>         }
>
>         return size;
> @@ -551,15 +562,17 @@ static ssize_t store_engine_load(struct device *dev,
>  {
>         struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
>         struct lp55xx_chip *chip = led->chip;
> +       ssize_t res;
>
>         mutex_lock(&chip->lock);
>
>         chip->engine_idx = nr;
>         lp5523_load_engine_and_select_page(chip);
>
> +       res = lp5523_update_program_memory(chip, buf, len);
>         mutex_unlock(&chip->lock);
>
> -       return lp5523_update_program_memory(chip, buf, len);
> +       return res;
>  }
>  store_load(1)
>  store_load(2)
> diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
> index 8d55a780..ae5c6fe 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
>         }
>
>         /*
> -        * Program momery sequence
> +        * Program memory sequence
>          *  1) set engine mode to "LOAD"
>          *  2) write firmware data into program memory
>          */
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: Broken locking in leds-lp5523.c
  2014-01-08  0:32 ` Bryan Wu
@ 2014-01-09 14:24   ` Pali Rohár
  2014-01-09 17:51     ` Bryan Wu
  2014-01-09 22:45   ` Pavel Machek
  1 sibling, 1 reply; 10+ messages in thread
From: Pali Rohár @ 2014-01-09 14:24 UTC (permalink / raw)
  To: Bryan Wu
  Cc: Pavel Machek, rpurdie, Linux LED Subsystem, lkml, milo kim,
	Sebastian Reichel, Sebastian Reichel

[-- Attachment #1: Type: Text/Plain, Size: 1004 bytes --]

On Wednesday 08 January 2014 01:32:15 Bryan Wu wrote:
> On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <pavel@ucw.cz> 
wrote:
> > Hi!
> > 
> > There's some locking weirdness, and few missing comments in
> > lp5523 driver.
> > 
> > Now, this is untested patch from my reverse-engineering. I
> > hope I understood things right...
> > 
> > In particular, there's unbalanced unlock in
> > lp5523_update_program_memory, and
> > lp5523_update_program_memory needs to be protected by the
> > lock.
> > 
> > Comments? Does someone maintain this?
> 
> Actually the locking issue was fixed in my for-next branch,
> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-led
> s.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403
> 795be9
> 
> Thanks,
> -Bryan
> 

Now, with linus tree lp5523 driver does not work without above 
commit on Nokia N900 device. I think that commit should go to 
3.13 and stable... What do you think?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: Broken locking in leds-lp5523.c
  2014-01-09 14:24   ` Pali Rohár
@ 2014-01-09 17:51     ` Bryan Wu
  0 siblings, 0 replies; 10+ messages in thread
From: Bryan Wu @ 2014-01-09 17:51 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Pavel Machek, rpurdie, Linux LED Subsystem, lkml, milo kim,
	Sebastian Reichel, Sebastian Reichel

On Thu, Jan 9, 2014 at 6:24 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Wednesday 08 January 2014 01:32:15 Bryan Wu wrote:
>> On Tue, Jan 7, 2014 at 12:42 PM, Pavel Machek <pavel@ucw.cz>
> wrote:
>> > Hi!
>> >
>> > There's some locking weirdness, and few missing comments in
>> > lp5523 driver.
>> >
>> > Now, this is untested patch from my reverse-engineering. I
>> > hope I understood things right...
>> >
>> > In particular, there's unbalanced unlock in
>> > lp5523_update_program_memory, and
>> > lp5523_update_program_memory needs to be protected by the
>> > lock.
>> >
>> > Comments? Does someone maintain this?
>>
>> Actually the locking issue was fixed in my for-next branch,
>> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-led
>> s.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403
>> 795be9
>>
>> Thanks,
>> -Bryan
>>
>
> Now, with linus tree lp5523 driver does not work without above
> commit on Nokia N900 device. I think that commit should go to
> 3.13 and stable... What do you think?
>

OK, can you just apply this fix on Linus tree and verify it on N900?
If only this fix will make N900 LED work, I will send out to Linus as
a critical fix for 3.13 as well as stable tree. Because there 2 more
patches for LED comes from Milo also fixed something and this patch
probably depends on them.

Please help to verify.

Thanks,
-Bryan

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: Broken locking in leds-lp5523.c
  2014-01-08  0:32 ` Bryan Wu
  2014-01-09 14:24   ` Pali Rohár
@ 2014-01-09 22:45   ` Pavel Machek
  1 sibling, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2014-01-09 22:45 UTC (permalink / raw)
  To: Bryan Wu
  Cc: rpurdie, Linux LED Subsystem, lkml, milo kim, Pali Rohár,
	Sebastian Reichel, Sebastian Reichel

Hi!

> > Now, this is untested patch from my reverse-engineering. I hope I
> > understood things right...
> >
> > In particular, there's unbalanced unlock in
> > lp5523_update_program_memory, and lp5523_update_program_memory needs
> > to be protected by the lock.
> >
> > Comments? Does someone maintain this?
> >
> 
> Actually the locking issue was fixed in my for-next branch,
> http://git.kernel.org/cgit/linux/kernel/git/cooloney/linux-leds.git/commit/?h=for-next&id=0d70bdb957cde2c25a4b3e4c93d0d33403795be9
> 

Yes, that should solve it. Thanks!
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: Broken locking in leds-lp5523.c
  2014-01-07 23:57 ` Bryan Wu
@ 2014-01-09 23:08   ` Pavel Machek
  0 siblings, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2014-01-09 23:08 UTC (permalink / raw)
  To: Bryan Wu
  Cc: milo kim, rpurdie, Linux LED Subsystem, lkml, Pali Rohár,
	Sebastian Reichel, Sebastian Reichel

Hi!

> > There's some locking weirdness, and few missing comments in lp5523
> > driver.
> >
> > Now, this is untested patch from my reverse-engineering. I hope I
> > understood things right...
> >
> > In particular, there's unbalanced unlock in
> > lp5523_update_program_memory, and lp5523_update_program_memory needs
> > to be protected by the lock.
> >
> > Comments? Does someone maintain this?
> 
> I think Milo Kim is maintaining this driver for the LP55xx chip
 > family.

Yes, he's on the Cc list. I was hoping he would comment.

> > -#define LP5523_PROGRAM_LENGTH          32
> > +#define LP5523_PROGRAM_LENGTH          32      /* bytes */
> > +/* Memory is used like this:
> > +   0x00 engine 1 program
> > +   0x10 engine 2 program
> > +   0x20 engine 3 program
> > +   0x30 engine 1 muxing info
> > +   0x40 engine 2 muxing info
> > +   0x50 engine 3 muxing info
> > +   ...and offsets are hard-coded all around :-( */

For example, I'd like to increase limit here -- 62 byte programs
should be feasible AFAICT.

> >  #define LP5523_MAX_LEDS                        9
> >
> >  /* Registers */
> > @@ -265,20 +273,25 @@ static int lp5523_init_program_engine(struct lp55xx_chip *chip)
> >         /* one pattern per engine setting LED MUX start and stop addresses */
> >         static const u8 pattern[][LP5523_PROGRAM_LENGTH] =  {
> >                 { 0x9c, 0x30, 0x9c, 0xb0, 0x9d, 0x80, 0xd8, 0x00, 0},
> > +               /* 9c30 -- mux_map_start(0x30)
> > +                  9cb0 -- mux_ld_end(0x50)
> > +                  9d80 -- mux_sel???(0x00)
> > +                  d800 -- invalid?? */

And it would be good to get comment on what this does. I could not
disassemble/decipher it.

Regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [trivial] Comment improvements for lp5523 and friend
  2014-01-07 20:42 ` Pavel Machek
                   ` (2 preceding siblings ...)
  (?)
@ 2014-01-09 23:13 ` Pavel Machek
  2014-02-06 19:36   ` Bryan Wu
  -1 siblings, 1 reply; 10+ messages in thread
From: Pavel Machek @ 2014-01-09 23:13 UTC (permalink / raw)
  To: cooloney, rpurdie, linux-leds, linux-kernel, milo.kim
  Cc: pali.rohar, sre, sre

Add some comments that are not obvious from first look at the driver
to lp5523, fix typo in lp8501.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index c00f55e4..ac64ebc 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -34,7 +34,15 @@
 
 #include "leds-lp55xx-common.h"
 
-#define LP5523_PROGRAM_LENGTH		32
+#define LP5523_PROGRAM_LENGTH		32	/* bytes */
+/* Memory is used like this:
+   0x00 engine 1 program
+   0x10 engine 2 program
+   0x20 engine 3 program
+   0x30 engine 1 muxing info
+   0x40 engine 2 muxing info
+   0x50 engine 3 muxing info
+*/
 #define LP5523_MAX_LEDS			9
 
 /* Registers */
index 8d55a780..ae5c6fe 100644
--- a/drivers/leds/leds-lp8501.c
+++ b/drivers/leds/leds-lp8501.c
@@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
 	}
 
 	/*
-	 * Program momery sequence
+	 * Program memory sequence
 	 *  1) set engine mode to "LOAD"
 	 *  2) write firmware data into program memory
 	 */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [trivial] Comment improvements for lp5523 and friend
  2014-01-09 23:13 ` [trivial] Comment improvements for lp5523 and friend Pavel Machek
@ 2014-02-06 19:36   ` Bryan Wu
  0 siblings, 0 replies; 10+ messages in thread
From: Bryan Wu @ 2014-02-06 19:36 UTC (permalink / raw)
  To: Pavel Machek
  Cc: rpurdie, Linux LED Subsystem, lkml, milo kim, Pali Rohár,
	Sebastian Reichel, Sebastian Reichel

On Thu, Jan 9, 2014 at 3:13 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Add some comments that are not obvious from first look at the driver
> to lp5523, fix typo in lp8501.
>

Milo, can you take a look at this. If it's OK, I will take it.

-Bryan

> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index c00f55e4..ac64ebc 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -34,7 +34,15 @@
>
>  #include "leds-lp55xx-common.h"
>
> -#define LP5523_PROGRAM_LENGTH          32
> +#define LP5523_PROGRAM_LENGTH          32      /* bytes */
> +/* Memory is used like this:
> +   0x00 engine 1 program
> +   0x10 engine 2 program
> +   0x20 engine 3 program
> +   0x30 engine 1 muxing info
> +   0x40 engine 2 muxing info
> +   0x50 engine 3 muxing info
> +*/
>  #define LP5523_MAX_LEDS                        9
>
>  /* Registers */
> index 8d55a780..ae5c6fe 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -262,7 +262,7 @@ static void lp8501_firmware_loaded(struct lp55xx_chip *chip)
>         }
>
>         /*
> -        * Program momery sequence
> +        * Program memory sequence
>          *  1) set engine mode to "LOAD"
>          *  2) write firmware data into program memory
>          */
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-02-06 19:36 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-07 20:42 Broken locking in leds-lp5523.c Pavel Machek
2014-01-07 20:42 ` Pavel Machek
2014-01-07 23:57 ` Bryan Wu
2014-01-09 23:08   ` Pavel Machek
2014-01-08  0:32 ` Bryan Wu
2014-01-09 14:24   ` Pali Rohár
2014-01-09 17:51     ` Bryan Wu
2014-01-09 22:45   ` Pavel Machek
2014-01-09 23:13 ` [trivial] Comment improvements for lp5523 and friend Pavel Machek
2014-02-06 19:36   ` Bryan Wu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.