[PATCH] gpiolib: Replace WARN_ON with WARN_ON_ONCE

[PATCH] gpiolib: Replace WARN_ON with WARN_ON_ONCE

[Ezequiel Garcia]

These warnings can be very spammy, since they could be called from
kernel threads. Use WARN_ON_ONCE, which is enough to warn developers
about the 'can_sleep' usage.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/gpio/gpiolib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7dd4461..9568998 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1927,7 +1927,7 @@ int gpiod_get_raw_value(const struct gpio_desc *desc)
 	if (!desc)
 		return 0;
 	/* Should be using gpio_get_value_cansleep() */
-	WARN_ON(desc->chip->can_sleep);
+	WARN_ON_ONCE(desc->chip->can_sleep);
 	return _gpiod_get_raw_value(desc);
 }
 EXPORT_SYMBOL_GPL(gpiod_get_raw_value);
@@ -1948,7 +1948,7 @@ int gpiod_get_value(const struct gpio_desc *desc)
 	if (!desc)
 		return 0;
 	/* Should be using gpio_get_value_cansleep() */
-	WARN_ON(desc->chip->can_sleep);
+	WARN_ON_ONCE(desc->chip->can_sleep);
 
 	value = _gpiod_get_raw_value(desc);
 	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
@@ -2042,7 +2042,7 @@ void gpiod_set_raw_value(struct gpio_desc *desc, int value)
 	if (!desc)
 		return;
 	/* Should be using gpio_set_value_cansleep() */
-	WARN_ON(desc->chip->can_sleep);
+	WARN_ON_ONCE(desc->chip->can_sleep);
 	_gpiod_set_raw_value(desc, value);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_raw_value);
@@ -2063,7 +2063,7 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
 	if (!desc)
 		return;
 	/* Should be using gpio_set_value_cansleep() */
-	WARN_ON(desc->chip->can_sleep);
+	WARN_ON_ONCE(desc->chip->can_sleep);
 	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
 		value = !value;
 	_gpiod_set_raw_value(desc, value);
-- 
1.8.1.5

Re: [PATCH] gpiolib: Replace WARN_ON with WARN_ON_ONCE

[Ezequiel Garcia]

Hi Linus,

On Sun, Nov 24, 2013 at 09:37:45AM -0300, Ezequiel Garcia wrote:
> These warnings can be very spammy, since they could be called from
> kernel threads. Use WARN_ON_ONCE, which is enough to warn developers
> about the 'can_sleep' usage.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/gpio/gpiolib.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7dd4461..9568998 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1927,7 +1927,7 @@ int gpiod_get_raw_value(const struct gpio_desc *desc)
>  	if (!desc)
>  		return 0;
>  	/* Should be using gpio_get_value_cansleep() */
> -	WARN_ON(desc->chip->can_sleep);
> +	WARN_ON_ONCE(desc->chip->can_sleep);
>  	return _gpiod_get_raw_value(desc);
>  }
>  EXPORT_SYMBOL_GPL(gpiod_get_raw_value);
> @@ -1948,7 +1948,7 @@ int gpiod_get_value(const struct gpio_desc *desc)
>  	if (!desc)
>  		return 0;
>  	/* Should be using gpio_get_value_cansleep() */
> -	WARN_ON(desc->chip->can_sleep);
> +	WARN_ON_ONCE(desc->chip->can_sleep);
>  
>  	value = _gpiod_get_raw_value(desc);
>  	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> @@ -2042,7 +2042,7 @@ void gpiod_set_raw_value(struct gpio_desc *desc, int value)
>  	if (!desc)
>  		return;
>  	/* Should be using gpio_set_value_cansleep() */
> -	WARN_ON(desc->chip->can_sleep);
> +	WARN_ON_ONCE(desc->chip->can_sleep);
>  	_gpiod_set_raw_value(desc, value);
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_raw_value);
> @@ -2063,7 +2063,7 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
>  	if (!desc)
>  		return;
>  	/* Should be using gpio_set_value_cansleep() */
> -	WARN_ON(desc->chip->can_sleep);
> +	WARN_ON_ONCE(desc->chip->can_sleep);
>  	if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
>  		value = !value;
>  	_gpiod_set_raw_value(desc, value);
> -- 
> 1.8.1.5
> 

Any comments on this?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gpiolib: Replace WARN_ON with WARN_ON_ONCE

[Linus Walleij]

On Thu, Nov 28, 2013 at 8:16 PM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> Hi Linus,
> On Sun, Nov 24, 2013 at 09:37:45AM -0300, Ezequiel Garcia wrote:
>> These warnings can be very spammy, since they could be called from
>> kernel threads. Use WARN_ON_ONCE, which is enough to warn developers
>> about the 'can_sleep' usage.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
(...)
> Any comments on this?

I'm a bit hesitant because I don't know the conventions for WARN_ON_ONCE()
vs WARN_ON().

I'd like some wider input if possible, Alexandre, Grant, what do you say?

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Replace WARN_ON with WARN_ON_ONCE

[Alexandre Courbot]

On Fri, Nov 29, 2013 at 9:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Nov 28, 2013 at 8:16 PM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
>> Hi Linus,
>> On Sun, Nov 24, 2013 at 09:37:45AM -0300, Ezequiel Garcia wrote:
>>> These warnings can be very spammy, since they could be called from
>>> kernel threads. Use WARN_ON_ONCE, which is enough to warn developers
>>> about the 'can_sleep' usage.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> (...)
>> Any comments on this?
>
> I'm a bit hesitant because I don't know the conventions for WARN_ON_ONCE()
> vs WARN_ON().
>
> I'd like some wider input if possible, Alexandre, Grant, what do you say?

I'd say WARN_ON() is just appropriate here. Calling a preemptible
function from a context that cannot sleep is a serious issue and you
cannot nag the user enough about it. At least if something bad happens
due to this condition the warning is guaranteed to be one of the last
messages the user will see before the system locks. Change it with
WARN_ON_ONCE() and chances are high that the system will hang without
any meaningful log near the point of failure.

Each caller should know whether it is running in preemptible context
or not and can thus use the right function, so I don't see any reason
to relax the rules here.

If we could know for sure at runtime whether we are running in
preemptible context or not we could probably merge these functions
into one and warn more appropriately on a per-case basis, but as far
as I know (which is not very far) there is no such way.

Alex.

Re: [PATCH] gpiolib: Replace WARN_ON with WARN_ON_ONCE

[Ezequiel Garcia]

On Sun, Dec 01, 2013 at 12:07:26PM +0900, Alexandre Courbot wrote:
> On Fri, Nov 29, 2013 at 9:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Nov 28, 2013 at 8:16 PM, Ezequiel Garcia
> > <ezequiel.garcia@free-electrons.com> wrote:
> >> Hi Linus,
> >> On Sun, Nov 24, 2013 at 09:37:45AM -0300, Ezequiel Garcia wrote:
> >>> These warnings can be very spammy, since they could be called from
> >>> kernel threads. Use WARN_ON_ONCE, which is enough to warn developers
> >>> about the 'can_sleep' usage.
> >>>
> >>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > (...)
> >> Any comments on this?
> >
> > I'm a bit hesitant because I don't know the conventions for WARN_ON_ONCE()
> > vs WARN_ON().
> >
> > I'd like some wider input if possible, Alexandre, Grant, what do you say?
> 
> I'd say WARN_ON() is just appropriate here. Calling a preemptible
> function from a context that cannot sleep is a serious issue and you
> cannot nag the user enough about it. At least if something bad happens
> due to this condition the warning is guaranteed to be one of the last
> messages the user will see before the system locks. Change it with
> WARN_ON_ONCE() and chances are high that the system will hang without
> any meaningful log near the point of failure.
> 
> Each caller should know whether it is running in preemptible context
> or not and can thus use the right function, so I don't see any reason
> to relax the rules here.
> 
> If we could know for sure at runtime whether we are running in
> preemptible context or not we could probably merge these functions
> into one and warn more appropriately on a per-case basis, but as far
> as I know (which is not very far) there is no such way.
> 

OK, that's fine by me.

Thanks for the feedback,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html