[patch 2/2] leds: netxbig: clean up a data type issue

[patch 2/2] leds: netxbig: clean up a data type issue

[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.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/include/linux/platform_data/leds-kirkwood-netxbig.h b/include/linux/platform_data/leds-kirkwood-netxbig.h
index d2be19a..bbd1d99 100644
--- a/include/linux/platform_data/leds-kirkwood-netxbig.h
+++ b/include/linux/platform_data/leds-kirkwood-netxbig.h
@@ -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 {
diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
index d0b743c..3da87be 100644
--- a/drivers/leds/leds-netxbig.c
+++ b/drivers/leds/leds-netxbig.c
@@ -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;

[patch 2/2] leds: netxbig: clean up a data type issue

[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.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/include/linux/platform_data/leds-kirkwood-netxbig.h b/include/linux/platform_data/leds-kirkwood-netxbig.h
index d2be19a..bbd1d99 100644
--- a/include/linux/platform_data/leds-kirkwood-netxbig.h
+++ b/include/linux/platform_data/leds-kirkwood-netxbig.h
@@ -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 {
diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
index d0b743c..3da87be 100644
--- a/drivers/leds/leds-netxbig.c
+++ b/drivers/leds/leds-netxbig.c
@@ -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;

Re: [patch 2/2] leds: netxbig: clean up a data type issue

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> 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 ?

Thanks,

Simon

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/include/linux/platform_data/leds-kirkwood-netxbig.h b/include/linux/platform_data/leds-kirkwood-netxbig.h
> index d2be19a..bbd1d99 100644
> --- a/include/linux/platform_data/leds-kirkwood-netxbig.h
> +++ b/include/linux/platform_data/leds-kirkwood-netxbig.h
> @@ -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 {
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index d0b743c..3da87be 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2] leds: netxbig: clean up a data type issue

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2321 bytes --]

On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> 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 ?

Thanks,

Simon

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/include/linux/platform_data/leds-kirkwood-netxbig.h b/include/linux/platform_data/leds-kirkwood-netxbig.h
> index d2be19a..bbd1d99 100644
> --- a/include/linux/platform_data/leds-kirkwood-netxbig.h
> +++ b/include/linux/platform_data/leds-kirkwood-netxbig.h
> @@ -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 {
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index d0b743c..3da87be 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[patch 2/2] leds: netxbig: clean up a data type issue

[Simon Guinot]

On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> 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 ?

Thanks,

Simon

> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/include/linux/platform_data/leds-kirkwood-netxbig.h b/include/linux/platform_data/leds-kirkwood-netxbig.h
> index d2be19a..bbd1d99 100644
> --- a/include/linux/platform_data/leds-kirkwood-netxbig.h
> +++ b/include/linux/platform_data/leds-kirkwood-netxbig.h
> @@ -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 {
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index d0b743c..3da87be 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150409/a1670a56/attachment.sig>

Re: [patch 2/2] leds: netxbig: clean up a data type issue

[Dan Carpenter]

On Thu, Apr 09, 2015 at 09:25:57PM +0200, Simon Guinot wrote:
> On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> > 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 ?

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.

regards,
dan carpenter

Re: [patch 2/2] leds: netxbig: clean up a data type issue

[Dan Carpenter]

On Thu, Apr 09, 2015 at 09:25:57PM +0200, Simon Guinot wrote:
> On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> > 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 ?

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.

regards,
dan carpenter

[patch 2/2] leds: netxbig: clean up a data type issue

[Dan Carpenter]

On Thu, Apr 09, 2015 at 09:25:57PM +0200, Simon Guinot wrote:
> On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> > 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 ?

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.

regards,
dan carpenter

Re: [patch 2/2] leds: netxbig: clean up a data type issue

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1871 bytes --]

On Thu, Apr 09, 2015 at 10:54:26PM +0300, Dan Carpenter wrote:
> On Thu, Apr 09, 2015 at 09:25:57PM +0200, Simon Guinot wrote:
> > On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> > > 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 ?
> 
> 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.

Sorry, I misunderstood your commit message.

But still, rather than changing the type from unsigned long to u32 for
the delay_{on,off} variables, I think we should use a temporary u32
variable for the of_property_read_u32_index calls. This way we can keep
the type of the delay_{on,off} variables consistent with the LED core
code.

Regards,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2] leds: netxbig: clean up a data type issue

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1871 bytes --]

On Thu, Apr 09, 2015 at 10:54:26PM +0300, Dan Carpenter wrote:
> On Thu, Apr 09, 2015 at 09:25:57PM +0200, Simon Guinot wrote:
> > On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> > > 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 ?
> 
> 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.

Sorry, I misunderstood your commit message.

But still, rather than changing the type from unsigned long to u32 for
the delay_{on,off} variables, I think we should use a temporary u32
variable for the of_property_read_u32_index calls. This way we can keep
the type of the delay_{on,off} variables consistent with the LED core
code.

Regards,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[patch 2/2] leds: netxbig: clean up a data type issue

[Simon Guinot]

On Thu, Apr 09, 2015 at 10:54:26PM +0300, Dan Carpenter wrote:
> On Thu, Apr 09, 2015 at 09:25:57PM +0200, Simon Guinot wrote:
> > On Thu, Apr 09, 2015 at 12:07:12PM +0300, Dan Carpenter wrote:
> > > 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 ?
> 
> 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.

Sorry, I misunderstood your commit message.

But still, rather than changing the type from unsigned long to u32 for
the delay_{on,off} variables, I think we should use a temporary u32
variable for the of_property_read_u32_index calls. This way we can keep
the type of the delay_{on,off} variables consistent with the LED core
code.

Regards,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150410/493f813c/attachment.sig>

[patch 2/2 v2] leds: netxbig: silence a static checker warning

[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 <dan.carpenter@oracle.com>
---
v2: use a temporary variable

diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
index 028686f2..6cb4537 100644
--- a/drivers/leds/leds-netxbig.c
+++ b/drivers/leds/leds-netxbig.c
@@ -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;

[patch 2/2 v2] leds: netxbig: silence a static checker warning

[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 <dan.carpenter@oracle.com>
---
v2: use a temporary variable

diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
index 028686f2..6cb4537 100644
--- a/drivers/leds/leds-netxbig.c
+++ b/drivers/leds/leds-netxbig.c
@@ -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;

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski]

Hi Dan,

On 04/10/2015 10:30 AM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> v2: use a temporary variable
>
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index 028686f2..6cb4537 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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.

>   			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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Best Regards,
Jacek Anaszewski

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski]

Hi Dan,

On 04/10/2015 10:30 AM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> v2: use a temporary variable
>
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index 028686f2..6cb4537 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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.

>   			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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Best Regards,
Jacek Anaszewski

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

On Fri, Apr 10, 2015 at 11:30:41AM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>

Acked-by: Simon Guinot <simon.guinot@sequanux.org>

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

> ---
> v2: use a temporary variable
> 
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index 028686f2..6cb4537 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

On Fri, Apr 10, 2015 at 11:30:41AM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>

Acked-by: Simon Guinot <simon.guinot@sequanux.org>

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

> ---
> v2: use a temporary variable
> 
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index 028686f2..6cb4537 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -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;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Dan Carpenter]

On Fri, Apr 10, 2015 at 04:18:34PM +0200, Jacek Anaszewski wrote:
> Hi Dan,
> 
> On 04/10/2015 10:30 AM, Dan Carpenter wrote:
> >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 <dan.carpenter@oracle.com>
> >---
> >v2: use a temporary variable
> >
> >diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> >index 028686f2..6cb4537 100644
> >--- a/drivers/leds/leds-netxbig.c
> >+++ b/drivers/leds/leds-netxbig.c
> >@@ -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

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Dan Carpenter]

On Fri, Apr 10, 2015 at 04:18:34PM +0200, Jacek Anaszewski wrote:
> Hi Dan,
> 
> On 04/10/2015 10:30 AM, Dan Carpenter wrote:
> >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 <dan.carpenter@oracle.com>
> >---
> >v2: use a temporary variable
> >
> >diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> >index 028686f2..6cb4537 100644
> >--- a/drivers/leds/leds-netxbig.c
> >+++ b/drivers/leds/leds-netxbig.c
> >@@ -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

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Fri, Apr 10, 2015 at 05:30:56PM +0300, Dan Carpenter wrote:
> On Fri, Apr 10, 2015 at 04:18:34PM +0200, Jacek Anaszewski wrote:
> > Hi Dan,
> > 
> > On 04/10/2015 10:30 AM, Dan Carpenter wrote:
> > >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 <dan.carpenter@oracle.com>
> > >---
> > >v2: use a temporary variable
> > >
> > >diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> > >index 028686f2..6cb4537 100644
> > >--- a/drivers/leds/leds-netxbig.c
> > >+++ b/drivers/leds/leds-netxbig.c
> > >@@ -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.

Yes, Jacek is right, you don't need to initialize the variables. Please,
don't worry about testing, I'll take care of that.

Simon

> 
> regards,
> dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Fri, Apr 10, 2015 at 05:30:56PM +0300, Dan Carpenter wrote:
> On Fri, Apr 10, 2015 at 04:18:34PM +0200, Jacek Anaszewski wrote:
> > Hi Dan,
> > 
> > On 04/10/2015 10:30 AM, Dan Carpenter wrote:
> > >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 <dan.carpenter@oracle.com>
> > >---
> > >v2: use a temporary variable
> > >
> > >diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> > >index 028686f2..6cb4537 100644
> > >--- a/drivers/leds/leds-netxbig.c
> > >+++ b/drivers/leds/leds-netxbig.c
> > >@@ -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.

Yes, Jacek is right, you don't need to initialize the variables. Please,
don't worry about testing, I'll take care of that.

Simon

> 
> regards,
> dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[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

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[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

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
> 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.

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.

Regards,

Simon

> 
> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
> 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.

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.

Regards,

Simon

> 
> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski]

On 04/10/2015 09:52 PM, Simon Guinot wrote:
> On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
>> 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.
>
> 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.

-- 
Best Regards,
Jacek Anaszewski

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski]

On 04/10/2015 09:52 PM, Simon Guinot wrote:
> On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
>> 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.
>
> 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.

-- 
Best Regards,
Jacek Anaszewski

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Gregory CLEMENT]

Hi Simon, Dan,

On 10/04/2015 16:30, Simon Guinot wrote:
> On Fri, Apr 10, 2015 at 11:30:41AM +0300, Dan Carpenter wrote:
>> 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 <dan.carpenter@oracle.com>
> 
> Acked-by: Simon Guinot <simon.guinot@sequanux.org>
> 
> 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).

Thanks,

Gregory

> 
> Regards,
> 
> Simon
> 
>> ---
>> v2: use a temporary variable
>>
>> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
>> index 028686f2..6cb4537 100644
>> --- a/drivers/leds/leds-netxbig.c
>> +++ b/drivers/leds/leds-netxbig.c
>> @@ -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;


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Gregory CLEMENT]

Hi Simon, Dan,

On 10/04/2015 16:30, Simon Guinot wrote:
> On Fri, Apr 10, 2015 at 11:30:41AM +0300, Dan Carpenter wrote:
>> 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 <dan.carpenter@oracle.com>
> 
> Acked-by: Simon Guinot <simon.guinot@sequanux.org>
> 
> 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).

Thanks,

Gregory

> 
> Regards,
> 
> Simon
> 
>> ---
>> v2: use a temporary variable
>>
>> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
>> index 028686f2..6cb4537 100644
>> --- a/drivers/leds/leds-netxbig.c
>> +++ b/drivers/leds/leds-netxbig.c
>> @@ -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;


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Mon, Apr 13, 2015 at 10:25:45AM +0200, Gregory CLEMENT wrote:
> Hi Simon, Dan,
> 
> On 10/04/2015 16:30, Simon Guinot wrote:
> > On Fri, Apr 10, 2015 at 11:30:41AM +0300, Dan Carpenter wrote:
> >> 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 <dan.carpenter@oracle.com>
> > 
> > Acked-by: Simon Guinot <simon.guinot@sequanux.org>
> > 
> > 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).

Hi Gregory,

Thanks for your efforts. I'll try again with v4.2 ...

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Mon, Apr 13, 2015 at 10:25:45AM +0200, Gregory CLEMENT wrote:
> Hi Simon, Dan,
> 
> On 10/04/2015 16:30, Simon Guinot wrote:
> > On Fri, Apr 10, 2015 at 11:30:41AM +0300, Dan Carpenter wrote:
> >> 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 <dan.carpenter@oracle.com>
> > 
> > Acked-by: Simon Guinot <simon.guinot@sequanux.org>
> > 
> > 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).

Hi Gregory,

Thanks for your efforts. I'll try again with v4.2 ...

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

On Mon, Apr 13, 2015 at 09:35:19AM +0200, Jacek Anaszewski wrote:
> On 04/10/2015 09:52 PM, Simon Guinot wrote:
> >On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
> >>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.
> >
> >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?

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.

> 
> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

On Mon, Apr 13, 2015 at 09:35:19AM +0200, Jacek Anaszewski wrote:
> On 04/10/2015 09:52 PM, Simon Guinot wrote:
> >On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
> >>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.
> >
> >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?

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.

> 
> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Dan Carpenter]

The subject of this patch is "silence a static checker warning."  I'm
just not going to send patches to deliberately introduce static checker
warnings.  :P  Forget about it.

This is like those patches that Markus Elfring sent removing if
conditions.  It seems like it should make the code shorter an more
simple to remove code but it actually makes the code more subtle and
difficult to understand.

regards,
dan carpenter

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Dan Carpenter]

The subject of this patch is "silence a static checker warning."  I'm
just not going to send patches to deliberately introduce static checker
warnings.  :P  Forget about it.

This is like those patches that Markus Elfring sent removing if
conditions.  It seems like it should make the code shorter an more
simple to remove code but it actually makes the code more subtle and
difficult to understand.

regards,
dan carpenter

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski]

On 04/13/2015 12:16 PM, Simon Guinot wrote:
> On Mon, Apr 13, 2015 at 09:35:19AM +0200, Jacek Anaszewski wrote:
>> On 04/10/2015 09:52 PM, Simon Guinot wrote:
>>> On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
>>>> 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.
>>>
>>> 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?
>
> 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.
>
>>
>> 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.

OK, that makes sense.

> 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...

Yeah, the mode needs to be validated.

> 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
>


-- 
Best Regards,
Jacek Anaszewski

Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski]

On 04/13/2015 12:16 PM, Simon Guinot wrote:
> On Mon, Apr 13, 2015 at 09:35:19AM +0200, Jacek Anaszewski wrote:
>> On 04/10/2015 09:52 PM, Simon Guinot wrote:
>>> On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
>>>> 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.
>>>
>>> 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?
>
> 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.
>
>>
>> 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.

OK, that makes sense.

> 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...

Yeah, the mode needs to be validated.

> 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
>


-- 
Best Regards,
Jacek Anaszewski