[PATCH 01/10] locking/rtmutex: Remove Comparison to bool

[PATCH 01/10] locking/rtmutex: Remove Comparison to bool

[Jules Irenge]

From: Jules Irenge <jbi.octave@example.com>

Coccinelle reports a warning inside rt_mutex_slowunlock()

WARNING: Comparison to bool

To fix this, the comparison to bool is removed
This not only fixes the issue but also removes the unnecessary comparison.

Remove comparison to bool

Signed-off-by: Jules Irenge <jbi.octave@example.com>
---
 kernel/locking/rtmutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 851bbb10819d..7289e7b26be4 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1378,7 +1378,7 @@ static bool __sched rt_mutex_slowunlock(struct rt_mutex *lock,
 	 */
 	while (!rt_mutex_has_waiters(lock)) {
 		/* Drops lock->wait_lock ! */
-		if (unlock_rt_mutex_safe(lock, flags) == true)
+		if (unlock_rt_mutex_safe(lock, flags))
 			return false;
 		/* Relock the rtmutex and try again */
 		raw_spin_lock_irqsave(&lock->wait_lock, flags);
-- 
2.25.1

Re: [PATCH 01/10] locking/rtmutex: Remove Comparison to bool

[Thomas Gleixner]

Jules Irenge <jbi.octave@gmail.com> writes:

> From: Jules Irenge <jbi.octave@example.com>
>
> Coccinelle reports a warning inside rt_mutex_slowunlock()
>
> WARNING: Comparison to bool
>
> To fix this, the comparison to bool is removed
> This not only fixes the issue but also removes the unnecessary comparison.
>
> Remove comparison to bool

So you explain 3 times in different ways that the comparison to bool is
removed. What's the point?

Thanks,

        tglx

Re: [PATCH 01/10] locking/rtmutex: Remove Comparison to bool

[Jules Irenge]

On Sat, 28 Mar 2020, Thomas Gleixner wrote:

> Jules Irenge <jbi.octave@gmail.com> writes:
> 
> > From: Jules Irenge <jbi.octave@example.com>
> >
> > Coccinelle reports a warning inside rt_mutex_slowunlock()
> >
> > WARNING: Comparison to bool
> >
> > To fix this, the comparison to bool is removed
> > This not only fixes the issue but also removes the unnecessary comparison.
> >
> > Remove comparison to bool
> 
> So you explain 3 times in different ways that the comparison to bool is
> removed. What's the point?
> 
> Thanks,
> 
>         tglx
> 
> 
> 
Thanks for the feedback I take good note. I will improve next time.

Re: [PATCH 01/10] locking/rtmutex: Remove Comparison to bool

[Thomas Gleixner]

Jules Irenge <jbi.octave@gmail.com> writes:
> On Sat, 28 Mar 2020, Thomas Gleixner wrote:
>> 
> Thanks for the feedback I take good note. I will improve next time.

And please remove these broken From: ...@example.com lines.

[PATCH 02/10] sched/topology: Replace 1 and 0 by boolean value

[Jules Irenge]

From: Jules Irenge <jbi.octave@example.com>

Coccinelle reports a warning inside sched_energy_aware_handler()

WARNING: Assignment of 0/1 to bool variable

To fix this, 1 is replaced by true and 0 by false.
Given that variable sched_energy_update is of bool type.
This fixes the warnings.

Signed-off-by: Jules Irenge <jbi.octave@example.com>
---
 kernel/sched/topology.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index dfb64c08a407..9d70cc68e549 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -221,9 +221,9 @@ int sched_energy_aware_handler(struct ctl_table *table, int write,
 		state = static_branch_unlikely(&sched_energy_present);
 		if (state != sysctl_sched_energy_aware) {
 			mutex_lock(&sched_energy_mutex);
-			sched_energy_update = 1;
+			sched_energy_update = true;
 			rebuild_sched_domains();
-			sched_energy_update = 0;
+			sched_energy_update = false;
 			mutex_unlock(&sched_energy_mutex);
 		}
 	}
-- 
2.25.1

Re: [PATCH 02/10] sched/topology: Replace 1 and 0 by boolean value

[Peter Zijlstra]

On Fri, Mar 27, 2020 at 09:23:49PM +0000, Jules Irenge wrote:
> From: Jules Irenge <jbi.octave@example.com>
> 
> Coccinelle reports a warning inside sched_energy_aware_handler()
> 
> WARNING: Assignment of 0/1 to bool variable

That's definitely not a warning and quite sensible in any case. Please
learn C.

Re: [PATCH 02/10] sched/topology: Replace 1 and 0 by boolean value

[Jules Irenge]

On Mon, 30 Mar 2020, Peter Zijlstra wrote:

> On Fri, Mar 27, 2020 at 09:23:49PM +0000, Jules Irenge wrote:
>> From: Jules Irenge <jbi.octave@example.com>
>>
>> Coccinelle reports a warning inside sched_energy_aware_handler()
>>
>> WARNING: Assignment of 0/1 to bool variable
>
> That's definitely not a warning and quite sensible in any case. Please
> learn C.

Thanks for the reply, I think I took the warning literally. I will take 
the advice any way.

Thanks,
Jules

[PATCH 03/10] cpu: Remove Comparison to bool

[Jules Irenge]

From: Jules Irenge <jbi.octave@example.com>

Coccinelle reports a warning inside __cpuhp_state_add_instance_cpuslocked

WARNING: Comparison to bool

To fix this, the comparison to bool is removed
This not only fixes the issue but also removes the unnecessary comparison.

Remove comparison to bool

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/cpu.c      | 2 +-
 kernel/rcu/tree.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 9c706af713fb..97f8b79ba5f5 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1682,7 +1682,7 @@ int __cpuhp_state_add_instance_cpuslocked(enum cpuhp_state state,
 	lockdep_assert_cpus_held();
 
 	sp = cpuhp_get_step(state);
-	if (sp->multi_instance == false)
+	if (!sp->multi_instance)
 		return -EINVAL;
 
 	mutex_lock(&cpuhp_state_mutex);
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index d91c9156fab2..c2ffea31eae8 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -100,7 +100,7 @@ static struct rcu_state rcu_state = {
 static bool dump_tree;
 module_param(dump_tree, bool, 0444);
 /* By default, use RCU_SOFTIRQ instead of rcuc kthreads. */
-static bool use_softirq = 1;
+static bool use_softirq = true;
 module_param(use_softirq, bool, 0444);
 /* Control rcu_node-tree auto-balancing at boot time. */
 static bool rcu_fanout_exact;
-- 
2.25.1

Re: [PATCH 03/10] cpu: Remove Comparison to bool

[Thomas Gleixner]

Jules Irenge <jbi.octave@gmail.com> writes:
> ---
>  kernel/cpu.c      | 2 +-
>  kernel/rcu/tree.c | 2 +-

How is rcu/tree.c related to 'cpu:' ?

Thanks,

        tglx

[PATCH 04/10] irq: Replace 1 by true

[Jules Irenge]

Coccinelle reports a warning

WARNING: Assignment of 0/1 to bool variable

To fix this, 1 is replaced by true.
Given that variable noirqdebug is of bool type.
This fixes the warnings.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/irq/spurious.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index f865e5f4d382..70ba6d55d02a 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -431,7 +431,7 @@ bool noirqdebug __read_mostly;
 
 int noirqdebug_setup(char *str)
 {
-	noirqdebug = 1;
+	noirqdebug = true;
 	printk(KERN_INFO "IRQ lockup detection disabled\n");
 
 	return 1;
-- 
2.25.1

[PATCH 05/10] softirq: Replace BUG() after if statement with BUG_ON

[Jules Irenge]

Coccinelle reports a warning tasklet_action_common()

WARNING: Use BUG_ON instead of if condition followed by BUG

To fix this, BUG() is replaced by BUG_ON() with the recommended suggestion

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/softirq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 0427a86743a4..8a8f6ea0ff0a 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -517,9 +517,8 @@ static void tasklet_action_common(struct softirq_action *a,
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
-							&t->state))
-					BUG();
+				BUG_ON(!test_and_clear_bit(TASKLET_STATE_SCHED,
+							   &t->state));
 				t->func(t->data);
 				tasklet_unlock(t);
 				continue;
-- 
2.25.1

Re: [PATCH 05/10] softirq: Replace BUG() after if statement with BUG_ON

[Thomas Gleixner]

Jules,

Jules Irenge <jbi.octave@gmail.com> writes:
> Coccinelle reports a warning tasklet_action_common()
>
> WARNING: Use BUG_ON instead of if condition followed by BUG
>
> To fix this, BUG() is replaced by BUG_ON() with the recommended
> suggestion

Well, the suggestion is wrong to begin with. The suggestion should be:

      Is this BUG() actually necessary and is this the only way to deal
      with this problem?

Only if that question can be answered with yes, then the recommended
replacement is ok.

BUG() is the last resort and each and every instance wants to be looked
at and not mechanically changed to some 'better' version of it.

Thanks,

        tglx

[PATCH 06/10] rcu: Replace assigned pointer ret value by corresponding boolean value

[Jules Irenge]

Coccinelle reports warnings at rcu_read_lock_held_common()

WARNING: Assignment of 0/1 to bool variable

To fix this,
the assigned  pointer ret values are replaced by corresponding boolean value.
Given that ret is a pointer of bool type

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/rcu/update.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 6c4b862f57d6..24fb64fd1a1a 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -98,15 +98,15 @@ module_param(rcu_normal_after_boot, int, 0);
 static bool rcu_read_lock_held_common(bool *ret)
 {
 	if (!debug_lockdep_rcu_enabled()) {
-		*ret = 1;
+		*ret = true;
 		return true;
 	}
 	if (!rcu_is_watching()) {
-		*ret = 0;
+		*ret = false;
 		return true;
 	}
 	if (!rcu_lockdep_current_cpu_online()) {
-		*ret = 0;
+		*ret = false;
 		return true;
 	}
 	return false;
-- 
2.25.1

Re: [PATCH 06/10] rcu: Replace assigned pointer ret value by corresponding boolean value

[Paul E. McKenney]

On Fri, Mar 27, 2020 at 09:23:53PM +0000, Jules Irenge wrote:
> Coccinelle reports warnings at rcu_read_lock_held_common()
> 
> WARNING: Assignment of 0/1 to bool variable
> 
> To fix this,
> the assigned  pointer ret values are replaced by corresponding boolean value.
> Given that ret is a pointer of bool type
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Queued for further review and testing, thank you!

							Thanx, Paul

> ---
>  kernel/rcu/update.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 6c4b862f57d6..24fb64fd1a1a 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -98,15 +98,15 @@ module_param(rcu_normal_after_boot, int, 0);
>  static bool rcu_read_lock_held_common(bool *ret)
>  {
>  	if (!debug_lockdep_rcu_enabled()) {
> -		*ret = 1;
> +		*ret = true;
>  		return true;
>  	}
>  	if (!rcu_is_watching()) {
> -		*ret = 0;
> +		*ret = false;
>  		return true;
>  	}
>  	if (!rcu_lockdep_current_cpu_online()) {
> -		*ret = 0;
> +		*ret = false;
>  		return true;
>  	}
>  	return false;
> -- 
> 2.25.1
> 

[PATCH 07/10] sched/fair: Replace 1 and 0 by boolean value

[Jules Irenge]

Coccinelle reports a warning at voluntary_active_balance

WARNING: return of 0/1 in function voluntary_active_balance with return type bool

To fix this, 1 is replaced by true and 0 by false.
Given that voluntary_active_balance() is of bool type.
This fixes the warnings.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/sched/fair.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index c1217bfe5e81..b11bb08687a9 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9082,7 +9082,7 @@ voluntary_active_balance(struct lb_env *env)
 	struct sched_domain *sd = env->sd;
 
 	if (asym_active_balance(env))
-		return 1;
+		return true;
 
 	/*
 	 * The dst_cpu is idle and the src_cpu CPU has only 1 CFS task.
@@ -9094,13 +9094,13 @@ voluntary_active_balance(struct lb_env *env)
 	    (env->src_rq->cfs.h_nr_running == 1)) {
 		if ((check_cpu_capacity(env->src_rq, sd)) &&
 		    (capacity_of(env->src_cpu)*sd->imbalance_pct < capacity_of(env->dst_cpu)*100))
-			return 1;
+			return true;
 	}
 
 	if (env->migration_type == migrate_misfit)
-		return 1;
+		return true;
 
-	return 0;
+	return false;
 }
 
 static int need_active_balance(struct lb_env *env)
-- 
2.25.1

Re: [PATCH 07/10] sched/fair: Replace 1 and 0 by boolean value

[Peter Zijlstra]

On Fri, Mar 27, 2020 at 09:23:54PM +0000, Jules Irenge wrote:
> Coccinelle reports a warning at voluntary_active_balance
> 
> WARNING: return of 0/1 in function voluntary_active_balance with return type bool

Please, can someone take that senseless script away? This is whitespace
wankery.

[PATCH 08/10] trace: Replace printk and WARN_ON with WARN

[Jules Irenge]

Coccinelle suggests replacing printk and WARN_ON with WARN

SUGGESTION: printk + WARN_ON can be just WARN.
Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/trace/trace_output.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index b4909082f6a4..c0efab8a40c4 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -764,8 +764,7 @@ int register_trace_event(struct trace_event *event)
 		list_add_tail(&event->list, list);
 
 	} else if (event->type > __TRACE_LAST_TYPE) {
-		printk(KERN_WARNING "Need to add type to trace.h\n");
-		WARN_ON(1);
+		WARN(1, "Need to add type to trace.h");
 		goto out;
 	} else {
 		/* Is this event already used */
-- 
2.25.1

[PATCH 09/10] kernel/sysctl.c: Replace 1 and 0 by corresponding boolean value

[Jules Irenge]

Coccinelle reports a warning

WARNING: Assignment of 0/1 to bool variable

To fix this, values 1 and 0 of first variable
are replaced by true and false respectively.
Given that variable first is of bool type.
This fixes the warnings.

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/sysctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ad5b88a53c5a..4132a35e85bd 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3158,7 +3158,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	int err = 0;
-	bool first = 1;
+	bool first = true;
 	size_t left = *lenp;
 	unsigned long bitmap_len = table->maxlen;
 	unsigned long *bitmap = *(unsigned long **) table->data;
@@ -3249,7 +3249,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 			}
 
 			bitmap_set(tmp_bitmap, val_a, val_b - val_a + 1);
-			first = 0;
+			first = false;
 			proc_skip_char(&p, &left, '\n');
 		}
 		kfree(kbuf);
@@ -3281,7 +3281,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
 					break;
 			}
 
-			first = 0; bit_b++;
+			first = false; bit_b++;
 		}
 		if (!err)
 			err = proc_put_char(&buffer, &left, '\n');
-- 
2.25.1

Re: [PATCH 09/10] kernel/sysctl.c: Replace 1 and 0 by corresponding boolean value

[Kees Cook]

On Fri, Mar 27, 2020 at 09:23:56PM +0000, Jules Irenge wrote:
> Coccinelle reports a warning
> 
> WARNING: Assignment of 0/1 to bool variable
> 
> To fix this, values 1 and 0 of first variable
> are replaced by true and false respectively.
> Given that variable first is of bool type.
> This fixes the warnings.

Sure! Thanks. :)

> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  kernel/sysctl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ad5b88a53c5a..4132a35e85bd 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -3158,7 +3158,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  			 void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
>  	int err = 0;
> -	bool first = 1;
> +	bool first = true;
>  	size_t left = *lenp;
>  	unsigned long bitmap_len = table->maxlen;
>  	unsigned long *bitmap = *(unsigned long **) table->data;
> @@ -3249,7 +3249,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  			}
>  
>  			bitmap_set(tmp_bitmap, val_a, val_b - val_a + 1);
> -			first = 0;
> +			first = false;
>  			proc_skip_char(&p, &left, '\n');
>  		}
>  		kfree(kbuf);
> @@ -3281,7 +3281,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  					break;
>  			}
>  
> -			first = 0; bit_b++;
> +			first = false; bit_b++;
>  		}
>  		if (!err)
>  			err = proc_put_char(&buffer, &left, '\n');
> -- 
> 2.25.1
> 

-- 
Kees Cook

[PATCH 10/10] trace: Replace printk and WARN_ON with WARN

[Jules Irenge]

Coccinelle suggests replacing printk and WARN_ON with WARN

SUGGESTION: printk + WARN_ON can be just WARN.
Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
---
 kernel/trace/trace.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6b11e4e2150c..1fe31272ea73 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1799,9 +1799,7 @@ static int run_tracer_selftest(struct tracer *type)
 	/* the test is responsible for resetting too */
 	tr->current_trace = saved_tracer;
 	if (ret) {
-		printk(KERN_CONT "FAILED!\n");
-		/* Add the warning after printing 'FAILED' */
-		WARN_ON(1);
+		WARN(1, "FAILED!\n");
 		return -1;
 	}
 	/* Only reset on passing, to avoid touching corrupted buffers */
-- 
2.25.1

Re: [PATCH 10/10] trace: Replace printk and WARN_ON with WARN

[Steven Rostedt]

On Fri, 27 Mar 2020 21:23:57 +0000
Jules Irenge <jbi.octave@gmail.com> wrote:

> Coccinelle suggests replacing printk and WARN_ON with WARN
> 
> SUGGESTION: printk + WARN_ON can be just WARN.
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  kernel/trace/trace.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6b11e4e2150c..1fe31272ea73 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1799,9 +1799,7 @@ static int run_tracer_selftest(struct tracer *type)
>  	/* the test is responsible for resetting too */
>  	tr->current_trace = saved_tracer;
>  	if (ret) {
> -		printk(KERN_CONT "FAILED!\n");
> -		/* Add the warning after printing 'FAILED' */

NACK! Did you not read the above comment. The FAILED goes with another
print and should NOT be part of the WARN_ON.

-- Steve

> -		WARN_ON(1);
> +		WARN(1, "FAILED!\n");
>  		return -1;
>  	}
>  	/* Only reset on passing, to avoid touching corrupted buffers */

Re: [PATCH 10/10] trace: Replace printk and WARN_ON with WARN

[Jules Irenge]

On Fri, 27 Mar 2020, Steven Rostedt wrote:

> On Fri, 27 Mar 2020 21:23:57 +0000
> Jules Irenge <jbi.octave@gmail.com> wrote:
> 
> > Coccinelle suggests replacing printk and WARN_ON with WARN
> > 
> > SUGGESTION: printk + WARN_ON can be just WARN.
> > Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> > ---
> >  kernel/trace/trace.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 6b11e4e2150c..1fe31272ea73 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -1799,9 +1799,7 @@ static int run_tracer_selftest(struct tracer *type)
> >  	/* the test is responsible for resetting too */
> >  	tr->current_trace = saved_tracer;
> >  	if (ret) {
> > -		printk(KERN_CONT "FAILED!\n");
> > -		/* Add the warning after printing 'FAILED' */
> 
> NACK! Did you not read the above comment. The FAILED goes with another
> print and should NOT be part of the WARN_ON.
> 
> -- Steve
> 
> > -		WARN_ON(1);
> > +		WARN(1, "FAILED!\n");
> >  		return -1;
> >  	}
> >  	/* Only reset on passing, to avoid touching corrupted buffers */
> 
> 
Thanks for the feedback, I will be more careful next time.
Kind regards,
Jules