[RFC] leds: core: Fix LED_COLOR_MAX_ID

Message ID 20191002163400.25317-1-dmurphy@ti.com
State New
Headers show
Series
  • [RFC] leds: core: Fix LED_COLOR_MAX_ID
Related show

Commit Message

Dan Murphy Oct. 2, 2019, 4:34 p.m.
The LED_COLOR_MAX_ID is incorrect.  THe MAX_ID should
be the last COLOR_ID in the list.  If an array was allocate
with MAX_ID the allocation would be correct but the meaning
is wrong.

So for array allocation the code should use LED_NUM_COLOR_IDS
which will allocate MAX_ID + 1.  Whent the code needs to validate
that the color ID is not out of bounds then the code should use
LED_COLOR_MAX_ID.

Signed-off-by: Dan Murphy <dmurphy@ti.com>

---
 drivers/leds/led-core.c           | 4 ++--
 drivers/leds/leds.h               | 2 +-
 include/dt-bindings/leds/common.h | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.22.0.214.g8dca754b1e

Comments

Pavel Machek Oct. 2, 2019, 6:36 p.m. | #1
On Wed 2019-10-02 11:34:00, Dan Murphy wrote:
> The LED_COLOR_MAX_ID is incorrect.  THe MAX_ID should

> be the last COLOR_ID in the list.  If an array was allocate

> with MAX_ID the allocation would be correct but the meaning

> is wrong.

> 

> So for array allocation the code should use LED_NUM_COLOR_IDS

> which will allocate MAX_ID + 1.  Whent the code needs to validate

> that the color ID is not out of bounds then the code should use

> LED_COLOR_MAX_ID.


Renaming original define might have been okay. Having two defines is
not. I'd say we can keep it as is...

								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dan Murphy Oct. 2, 2019, 6:42 p.m. | #2
Hello

On 10/2/19 1:36 PM, Pavel Machek wrote:
> On Wed 2019-10-02 11:34:00, Dan Murphy wrote:

>> The LED_COLOR_MAX_ID is incorrect.  THe MAX_ID should

>> be the last COLOR_ID in the list.  If an array was allocate

>> with MAX_ID the allocation would be correct but the meaning

>> is wrong.

>>

>> So for array allocation the code should use LED_NUM_COLOR_IDS

>> which will allocate MAX_ID + 1.  Whent the code needs to validate

>> that the color ID is not out of bounds then the code should use

>> LED_COLOR_MAX_ID.

> Renaming original define might have been okay. Having two defines is

> not. I'd say we can keep it as is...


OK.  It was just not logical that MAX_ID will always be 1 more then the 
actual MAX_ID.

So every ID boundary check will need to be ">=" as opposed to ">" which 
means we have to take care in reviews to make sure this is what is intended.

But it was just a RFC so I am not pushing this fix.  It would mean I 
would have to re-touch the MC framework patches.

Dan


>

> 								Pavel

>

Patch

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..4e57d47c97e0 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -25,7 +25,7 @@  EXPORT_SYMBOL_GPL(leds_list_lock);
 LIST_HEAD(leds_list);
 EXPORT_SYMBOL_GPL(leds_list);
 
-const char * const led_colors[LED_COLOR_ID_MAX] = {
+const char * const led_colors[LED_NUM_COLOR_IDS] = {
 	[LED_COLOR_ID_WHITE] = "white",
 	[LED_COLOR_ID_RED] = "red",
 	[LED_COLOR_ID_GREEN] = "green",
@@ -385,7 +385,7 @@  static void led_parse_fwnode_props(struct device *dev,
 		ret = fwnode_property_read_u32(fwnode, "color", &props->color);
 		if (ret)
 			dev_err(dev, "Error parsing 'color' property (%d)\n", ret);
-		else if (props->color >= LED_COLOR_ID_MAX)
+		else if (props->color > LED_COLOR_ID_MAX)
 			dev_err(dev, "LED color identifier out of range\n");
 		else
 			props->color_present = true;
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 2d9eb48bbed9..2c493efc8f55 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -33,6 +33,6 @@  ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
 extern struct list_head trigger_list;
-extern const char * const led_colors[LED_COLOR_ID_MAX];
+extern const char * const led_colors[LED_NUM_COLOR_IDS];
 
 #endif	/* __LEDS_H_INCLUDED */
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 9e1256a7c1bf..ce82f0b5f6d6 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -29,7 +29,8 @@ 
 #define LED_COLOR_ID_VIOLET	5
 #define LED_COLOR_ID_YELLOW	6
 #define LED_COLOR_ID_IR		7
-#define LED_COLOR_ID_MAX	8
+#define LED_COLOR_ID_MAX	LED_COLOR_ID_IR
+#define LED_NUM_COLOR_IDS	(LED_COLOR_ID_MAX + 1)
 
 /* Standard LED functions */
 #define LED_FUNCTION_ACTIVITY "activity"