leds: netxbig: clean up a data type issue

This driver is pretty hardware specific so it's unlikely that we're going to be using it on 64 big endian systems. Still, the current code causes a static checker warning so we may as well change the type from "unsigned long" to "u32" and remove the casting.

Signed-off-by Dan Carpenter <[email protected]>

include/linux/platform_data/leds-kirkwood-netxbig.h +3 -3

index d2be19a..bbd1d99 100644
@@ -29,9 +29,9 @@ enum netxbig_led_mode {
 #define NETXBIG_LED_INVALID_MODE NETXBIG_LED_MODE_NUM
 
 struct netxbig_led_timer {
-	unsigned long		delay_on;
-	unsigned long		delay_off;
-	enum netxbig_led_mode	mode;
+	u32 delay_on;
+	u32 delay_off;
+	enum netxbig_led_mode mode;
 };
 
 struct netxbig_led {

drivers/leds/leds-netxbig.c +2 -2

index d0b743c..3da87be 100644
@@ -447,9 +447,9 @@ static int netxbig_leds_get_of_pdata(struct device *dev,
 			of_property_read_u32_index(np, "timers", 3 * i,
 						&timers[i].mode);
 			of_property_read_u32_index(np, "timers", 3 * i + 1,
-						(u32 *) &timers[i].delay_on);
+						   &timers[i].delay_on);
 			of_property_read_u32_index(np, "timers", 3 * i + 2,
-						(u32 *) &timers[i].delay_off);
+						   &timers[i].delay_off);
 		}
 		pdata->timer = timers;
 		pdata->num_timer = num_timers;

Quoted 4 lines · Dan Carpenter

This driver is pretty hardware specific so it's unlikely that we're going to be using it on 64 big endian systems. Still, the current code causes a static checker warning so we may as well change the type from "unsigned long" to "u32" and remove the casting.

Hi Dan,

Thanks for the patch.

Why do you think it would be an issue to use the u32 type for this variables on a 64-bit big-endian machine ? Note that this LED mechanism is actually used on ARM 32-bits and x86-64 NAS platforms. The latter are not mainlined yet. But indeed, for now, there is no plan to use it on a 64-bit big-endian machine.

Since the whole LED code uses the unsigned long type to hold the delay values, if possible, I would prefer to keep the delay_{on,off} variables consistent with that.

Is there an another way to make the "static checker" happy ?

Quoted 16 lines · Simon Guinot

Quoted 4 lines · Dan Carpenter

This driver is pretty hardware specific so it's unlikely that we're going to be using it on 64 big endian systems. Still, the current code causes a static checker warning so we may as well change the type from "unsigned long" to "u32" and remove the casting.

Hi Dan,

Thanks for the patch.

Why do you think it would be an issue to use the u32 type for this variables on a 64-bit big-endian machine ? Note that this LED mechanism is actually used on ARM 32-bits and x86-64 NAS platforms. The latter are not mainlined yet. But indeed, for now, there is no plan to use it on a 64-bit big-endian machine.

Quoted 5 lines

Since the whole LED code uses the unsigned long type to hold the delay values, if possible, I would prefer to keep the delay_{on,off} variables consistent with that.

Is there an another way to make the "static checker" happy ?

We're writing over 32 bits of a long. It's a dangerous habbit.

If long is u32 then of_property_read_u32_index() works, obviously. If it is a little endian and the last 32 bits have been zeroed out (as they have been here) then that also works.

If it's big endian and 64 bit then it doesn't work because we're writing to the wrong 32 bits.

Static checkers complain that "timers[i].delay_on" is an unsigned long but we're writing to only 32 bits of it. The code works on 32 bit systems and little endian 64 bit systems so it doesn't cause a problem in practise but it's still better to silence the warning.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: use a temporary variable

drivers/leds/leds-netxbig.c +0 -0

index 028686f2..6cb4537 100644

drivers/leds/leds-netxbig.c +4 -3

  			of_property_read_u32_index(np, "timers", 3 * i,
  						&timers[i].mode);
  			of_property_read_u32_index(np, "timers", 3 * i + 1,
-						(u32 *) &timers[i].delay_on);
+						   &delay_on);
  			of_property_read_u32_index(np, "timers", 3 * i + 2,
-						(u32 *) &timers[i].delay_off);
+						   &delay_off);
+			timers[i].delay_on = delay_on;
+			timers[i].delay_off = delay_off;
  		}
  		pdata->timer = timers;
  		pdata->num_timer = num_timers;
--

To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Dan,

Quoted 9 lines · Dan Carpenter

Static checkers complain that "timers[i].delay_on" is an unsigned long but we're writing to only 32 bits of it. The code works on 32 bit systems and little endian 64 bit systems so it doesn't cause a problem in practise but it's still better to silence the warning.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: use a temporary variable

drivers/leds/leds-netxbig.c static int netxbig_leds_get_of_pdata(struct device *dev, · line 448

Quoted 10 lines

drivers/leds/leds-netxbig.c +2 -0

index 028686f2..6cb4537 100644
@@ -444,12 +444,17 @@ static int netxbig_leds_get_of_pdata(struct device *dev,
 		if (!timers)
 			return -ENOMEM;
 		for (i = 0; i < num_timers; i++) {
+			u32 delay_on = 0;
+			u32 delay_off = 0;

These variables don't need initialization, as they are assigned a new value in of_property_read_u32_index anyway.

Quoted 12 lines · Jacek Anaszewski

Hi Dan,

Quoted 9 lines · Dan Carpenter

Static checkers complain that "timers[i].delay_on" is an unsigned long but we're writing to only 32 bits of it. The code works on 32 bit systems and little endian 64 bit systems so it doesn't cause a problem in practise but it's still better to silence the warning.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: use a temporary variable

Quoted 13 lines

drivers/leds/leds-netxbig.c static int netxbig_leds_get_of_pdata(struct device *dev, · line 448

Quoted 10 lines

drivers/leds/leds-netxbig.c +2 -0

index 028686f2..6cb4537 100644
@@ -444,12 +444,17 @@ static int netxbig_leds_get_of_pdata(struct device *dev,
 		if (!timers)
 			return -ENOMEM;
 		for (i = 0; i < num_timers; i++) {
+			u32 delay_on = 0;
+			u32 delay_off = 0;

These variables don't need initialization, as they are assigned a new value in of_property_read_u32_index anyway.

I don't know this hardware and I can't even test it so I didn't feel comfortable leaving it out. Also static checkers will complain that we are ignoring the error paths.

regards,
dan carpenter

I've looked at this some more. Most of the places which call of_property_read_u32_index() check the return code. The ones that don't mostly initialize their values going in. The remainder introduce static checker warnings like:

	drivers/clk/ti/divider.c:472 ti_clk_get_div_table()
	error: potentially using uninitialized 'val'.

These warnings cause me pain. It calls of_get_property() earlier so it won't return -EINVAL. I don't know if it can return -ENODATA or -EOVERFLOW?

I guess not.

Honestly, I hate ambigous code like this. If it were just a clear bug then I could fix it but I invest more time in ambiguous code and end up not writing a patch.

regards,
dan carpenter

Quoted 12 lines · Dan Carpenter

I've looked at this some more. Most of the places which call of_property_read_u32_index() check the return code. The ones that don't mostly initialize their values going in. The remainder introduce static checker warnings like:

	drivers/clk/ti/divider.c:472 ti_clk_get_div_table()
	error: potentially using uninitialized 'val'.

These warnings cause me pain. It calls of_get_property() earlier so it won't return -EINVAL. I don't know if it can return -ENODATA or -EOVERFLOW?

Quoted 1 line

I guess not.

I think it can't. Above, we are calling of_property_count_u32_elems() to count the number of u32 elements in the "timers" property. After we are ensuring that there is three u32 elements available per timer. That's why the return codes for the three of_property_read_u32_index() calls are not checked.

Quoted 13 lines · Simon Guinot

Quoted 12 lines · Dan Carpenter

I've looked at this some more. Most of the places which call of_property_read_u32_index() check the return code. The ones that don't mostly initialize their values going in. The remainder introduce static checker warnings like:

	drivers/clk/ti/divider.c:472 ti_clk_get_div_table()
	error: potentially using uninitialized 'val'.

These warnings cause me pain. It calls of_get_property() earlier so it won't return -EINVAL. I don't know if it can return -ENODATA or -EOVERFLOW?

Quoted 7 lines

Quoted 1 line

I guess not.

I think it can't. Above, we are calling of_property_count_u32_elems() to count the number of u32 elements in the "timers" property. After we are ensuring that there is three u32 elements available per timer. That's why the return codes for the three of_property_read_u32_index() calls are not checked.

After looking at Documentation/devicetree/bindings/leds/leds-netxbig.txt I noticed inconsistency: timers property is defined as required, but the comment over the call to of_property_count_u32_elems says that it is optional.

I think that DT documentation should be changed to make the property optional. How do you think?

Besides, I am wondering if we shouldn't check if the values read are sane? In such a case initializing delay_on and delay_off to 0 would be useful. We could check if both delays don't equal 0, which could happen if the of_property_read_u32_index returned negative value because of providing values out of bounds or not numerical values.

Quoted 22 lines · Jacek Anaszewski

Quoted 13 lines · Simon Guinot

Quoted 12 lines · Dan Carpenter

I've looked at this some more. Most of the places which call of_property_read_u32_index() check the return code. The ones that don't mostly initialize their values going in. The remainder introduce static checker warnings like:

	drivers/clk/ti/divider.c:472 ti_clk_get_div_table()
	error: potentially using uninitialized 'val'.

These warnings cause me pain. It calls of_get_property() earlier so it won't return -EINVAL. I don't know if it can return -ENODATA or -EOVERFLOW?

Quoted 7 lines

Quoted 1 line

I guess not.

I think it can't. Above, we are calling of_property_count_u32_elems() to count the number of u32 elements in the "timers" property. After we are ensuring that there is three u32 elements available per timer. That's why the return codes for the three of_property_read_u32_index() calls are not checked.

Quoted 7 lines

After looking at Documentation/devicetree/bindings/leds/leds-netxbig.txt I noticed inconsistency: timers property is defined as required, but the comment over the call to of_property_count_u32_elems says that it is optional.

I think that DT documentation should be changed to make the property optional. How do you think?

Thanks for spotting this. At first, I wrote the binding document. And since there is always "blink timers" defined with this LED mechanism, I made the property mandatory. But after, in the code, I made it optional because there is no point in discarding a LED if timers are missing.

I'll update the documentation accordingly.

Quoted 6 lines

Besides, I am wondering if we shouldn't check if the values read are sane? In such a case initializing delay_on and delay_off to 0 would be useful. We could check if both delays don't equal 0, which could happen if the of_property_read_u32_index returned negative value because of providing values out of bounds or not numerical values.

Well, here I didn't checked too much intentionally. IMO, calling of_property_count_u32_elems() is enough to make sure that the following u32 read will succeed. After that, there is not real way to check if a delay value is sane, or not. To make work an hardware timer, correct values have to be defined in the DT. If not, then this hardware timer will be either broken or not reachable. It is not really harmful.

Now, I can see, that I have also missed a check on the timer "mode" value. And since mode is used as an array index, I think it is a little bit more serious...

Dan, given that the patches adding DT support for the leds-netxbig driver have not been merged with Linux v4.1, I propose you drop your patch. Instead, I'll send a v2, trying to take into account all the comments here.

Thanks for the review and the comments.

Simon

Static checkers complain that "timers[i].delay_on" is an unsigned long but we're writing to only 32 bits of it. The code works on 32 bit systems and little endian 64 bit systems so it doesn't cause a problem in practise but it's still better to silence the warning.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: use a temporary variable

drivers/leds/leds-netxbig.c +7 -2

index 028686f2..6cb4537 100644
@@ -444,12 +444,17 @@ static int netxbig_leds_get_of_pdata(struct device *dev,
 		if (!timers)
 			return -ENOMEM;
 		for (i = 0; i < num_timers; i++) {
+			u32 delay_on = 0;
+			u32 delay_off = 0;
+
 			of_property_read_u32_index(np, "timers", 3 * i,
 						&timers[i].mode);
 			of_property_read_u32_index(np, "timers", 3 * i + 1,
-						(u32 *) &timers[i].delay_on);
+						   &delay_on);
 			of_property_read_u32_index(np, "timers", 3 * i + 2,
-						(u32 *) &timers[i].delay_off);
+						   &delay_off);
+			timers[i].delay_on = delay_on;
+			timers[i].delay_off = delay_off;
 		}
 		pdata->timer = timers;
 		pdata->num_timer = num_timers;

Quoted 6 lines · Dan Carpenter

Static checkers complain that "timers[i].delay_on" is an unsigned long but we're writing to only 32 bits of it. The code works on 32 bit systems and little endian 64 bit systems so it doesn't cause a problem in practise but it's still better to silence the warning.

Signed-off-by Dan Carpenter <[email protected]>

Acked-by Simon Guinot <[email protected]>

Note that your patch applies on the top of a patch "leds: netxbig: add device tree binding" which has not been merged yet by Bryan. For now, this patch only sits in the mvebu/for-next branch for testing purpose. And it is still not clear to me in which tree the patch will go. That's why I think you should resend your patch to the mvebu maintainers (added in Cc) on the LAKML. Probably they will be interested in merging your clean-up patch in the mvebu/for-next branch as well.

Regards,

Simon

Quoted 27 lines

---
v2: use a temporary variable

drivers/leds/leds-netxbig.c +7 -2

index 028686f2..6cb4537 100644
@@ -444,12 +444,17 @@ static int netxbig_leds_get_of_pdata(struct device *dev,
 		if (!timers)
 			return -ENOMEM;
 		for (i = 0; i < num_timers; i++) {
+			u32 delay_on = 0;
+			u32 delay_off = 0;
+
 			of_property_read_u32_index(np, "timers", 3 * i,
 						&timers[i].mode);
 			of_property_read_u32_index(np, "timers", 3 * i + 1,
-						(u32 *) &timers[i].delay_on);
+						   &delay_on);
 			of_property_read_u32_index(np, "timers", 3 * i + 2,
-						(u32 *) &timers[i].delay_off);
+						   &delay_off);
+			timers[i].delay_on = delay_on;
+			timers[i].delay_off = delay_off;
 		}
 		pdata->timer = timers;
 		pdata->num_timer = num_timers;

Hi Simon, Dan,

Quoted 10 lines · Simon Guinot

Quoted 6 lines · Dan Carpenter

Static checkers complain that "timers[i].delay_on" is an unsigned long but we're writing to only 32 bits of it. The code works on 32 bit systems and little endian 64 bit systems so it doesn't cause a problem in practise but it's still better to silence the warning.

Signed-off-by Dan Carpenter <[email protected]>

Acked-by Simon Guinot <[email protected]>

Quoted 7 lines

Note that your patch applies on the top of a patch "leds: netxbig: add device tree binding" which has not been merged yet by Bryan. For now, this patch only sits in the mvebu/for-next branch for testing purpose. And it is still not clear to me in which tree the patch will go. That's why I think you should resend your patch to the mvebu maintainers (added in Cc) on the LAKML. Probably they will be interested in merging your clean-up patch in the mvebu/for-next branch as well.

Actually the reason to get all the series in mvebu/for-next was to be confident enough to make a pull request as soon as the driver part was acked by the maintainer. Unfortunately it didn't occur until now and the merge windows for arm-soc is now closed so it won't be part of v4.1 (at least the mvebu related part).