[PATCH v2 0/4] Coccinelle cleanups

[PATCH v2 0/4] Coccinelle cleanups

[Jules Irenge]

This patch series clean up some warnings by the Coccinelle tool.

Jules Irenge (4):
  cpu: Remove Comparison to bool
  rcu: Replace 1 by true
  genirq: Replace 1 by true
  locking/rtmutex: Remove Comparison to bool

 kernel/cpu.c             | 2 +-
 kernel/irq/spurious.c    | 2 +-
 kernel/locking/rtmutex.c | 2 +-
 kernel/rcu/tree.c        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

-- 
Changes since v2:
- Improve on commit log 
- Correct mistake of mixing two different subsystems patches into one.
2.25.1

[PATCH v2 1/4] cpu: Remove Comparison to bool

[Jules Irenge]

Coccinelle reports a warning inside __cpuhp_state_add_instance_cpuslocked()

WARNING: Comparison to bool

To fix this a comparison to a bool variable to false is removed
and replaced with the operator ! before the variable

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

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);
-- 
2.25.1

[PATCH v2 2/4] rcu: Replace 1 by true

[Jules Irenge]

Coccinelle reports a warning at use_softirq declaration

WARNING: Assignment of 0/1 to bool variable

The root cause is
use_softirq a variable of bool type is initialised with the integer 1
Replacing 1 with value true solve the issue.

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

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 v2 2/4] rcu: Replace 1 by true

[Paul E. McKenney]

On Mon, Mar 30, 2020 at 02:24:48AM +0100, Jules Irenge wrote:
> Coccinelle reports a warning at use_softirq declaration
> 
> WARNING: Assignment of 0/1 to bool variable
> 
> The root cause is
> use_softirq a variable of bool type is initialised with the integer 1
> Replacing 1 with value true solve the issue.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>

Queued for review and testing, thank you!

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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
> 

[PATCH v2 3/4] genirq: Replace 1 by true

[Jules Irenge]

Coccinelle reports a warning at noirqdebug declaration

WARNING: Assignment of 0/1 to bool variable

The root cause is that
noirqdebug, a variable of bool type is initialised with the integer 1
Replacing 1 with value true fixes the issue.

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 v2 4/4] locking/rtmutex: Remove Comparison to bool

[Jules Irenge]

Coccinelle reports a warning inside __sched rt_mutex_slowunlock()

WARNING: Comparison to bool

To fix this,
a comparison (==) of a bool type function result to value true
together with the value are removed.

Signed-off-by: Jules Irenge <jbi.octave@gmail.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 v2 4/4] locking/rtmutex: Remove Comparison to bool

[Peter Zijlstra]

On Mon, Mar 30, 2020 at 02:24:50AM +0100, Jules Irenge wrote:
> Coccinelle reports a warning inside __sched rt_mutex_slowunlock()
> 
> WARNING: Comparison to bool

I don't mind the patch; but WTH is that a WARNING ?!? Superfluous, but
definitely not wrong or even dangerous AFAICT.

> To fix this,
> a comparison (==) of a bool type function result to value true
> together with the value are removed.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.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 v2 4/4] locking/rtmutex: Remove Comparison to bool

[Jules Irenge]

On Mon, 30 Mar 2020, Peter Zijlstra wrote:

> On Mon, Mar 30, 2020 at 02:24:50AM +0100, Jules Irenge wrote:
>> Coccinelle reports a warning inside __sched rt_mutex_slowunlock()
>>
>> WARNING: Comparison to bool
>
> I don't mind the patch; but WTH is that a WARNING ?!? Superfluous, but
> definitely not wrong or even dangerous AFAICT.
>
>> To fix this,
>> a comparison (==) of a bool type function result to value true
>> together with the value are removed.
>>
>> Signed-off-by: Jules Irenge <jbi.octave@gmail.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
>>
>
Thanks for the reply, I will take good note.

[PATCH 0/4] Checkpatch tool errors clean up

[Jules Irenge]

Hi
I am proposing these 4 patches. 
I am currently learning the core kernel the hard way. 
I will appreciate any feedback negative or positive.
Thanks 

Jules Irenge (4):
  acct: Add required space between variable and operator
  audit: uninitialize global variable audit_sig_sid
  audit: uninitialize static variables
  context_tracking: uninitialize static variables

 kernel/acct.c             |  2 +-
 kernel/audit.c            | 10 +++++-----
 kernel/context_tracking.c |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/4] acct: Add required space between variable and operator

[Jules Irenge]

Checkpatch tool reports an error

"ERROR: spaces required around that == (ctx:VxV)"

To fix this space has been added between the variable,
the operator and the value.

Add the missing required space.

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

diff --git a/kernel/acct.c b/kernel/acct.c
index b0c5b3a9f5af..d7cc5f917e11 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -451,7 +451,7 @@ static void fill_ac(acct_t *ac)
 	do_div(elapsed, AHZ);
 	btime = ktime_get_real_seconds() - elapsed;
 	ac->ac_btime = clamp_t(time64_t, btime, 0, U32_MAX);
-#if ACCT_VERSION==2
+#if ACCT_VERSION == 2
 	ac->ac_ahz = AHZ;
 #endif
 
-- 
2.26.2

[PATCH 2/4] audit: uninitialize global variable audit_sig_sid

[Jules Irenge]

Checkpatch tool reports an error at variable audit_sig_sid declaration

"ERROR: do not initialise globals to 0"

To fix this, the global variable has been uninitialized.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index 8c201f414226..7b1a38a211a9 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -125,7 +125,7 @@ static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
 /* The identity of the user shutting down the audit system. */
 kuid_t		audit_sig_uid = INVALID_UID;
 pid_t		audit_sig_pid = -1;
-u32		audit_sig_sid = 0;
+u32		audit_sig_sid;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]
-- 
2.26.2

Re: [PATCH 2/4] audit: uninitialize global variable audit_sig_sid

[Joe Perches]

On Sat, 2020-08-01 at 19:46 +0100, Jules Irenge wrote:
> Checkpatch tool reports an error at variable audit_sig_sid declaration
[]
> diff --git a/kernel/audit.c b/kernel/audit.c
[]
> @@ -125,7 +125,7 @@ static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
>  /* The identity of the user shutting down the audit system. */
>  kuid_t		audit_sig_uid = INVALID_UID;
>  pid_t		audit_sig_pid = -1;
> -u32		audit_sig_sid = 0;
> +u32		audit_sig_sid;

All of these are unused outside of audit.c and might as
well be static and removed from the .h file.

Re: [PATCH 2/4] audit: uninitialize global variable audit_sig_sid

[Paul Moore]

On Sat, Aug 1, 2020 at 2:55 PM Joe Perches <joe@perches.com> wrote:
> On Sat, 2020-08-01 at 19:46 +0100, Jules Irenge wrote:
> > Checkpatch tool reports an error at variable audit_sig_sid declaration
> []
> > diff --git a/kernel/audit.c b/kernel/audit.c
> []
> > @@ -125,7 +125,7 @@ static u32        audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> >  /* The identity of the user shutting down the audit system. */
> >  kuid_t               audit_sig_uid = INVALID_UID;
> >  pid_t                audit_sig_pid = -1;
> > -u32          audit_sig_sid = 0;
> > +u32          audit_sig_sid;
>
> All of these are unused outside of audit.c and might as
> well be static and removed from the .h file.

There's plenty of time before the merge window closes, doing this
would definitely make this patch much more useful than the typical
checkpatch noise.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/4] audit: uninitialize global variable audit_sig_sid

[Jules Irenge]

On Sat, 1 Aug 2020, Joe Perches wrote:

> On Sat, 2020-08-01 at 19:46 +0100, Jules Irenge wrote:
> > Checkpatch tool reports an error at variable audit_sig_sid declaration
> []
> > diff --git a/kernel/audit.c b/kernel/audit.c
> []
> > @@ -125,7 +125,7 @@ static u32	audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> >  /* The identity of the user shutting down the audit system. */
> >  kuid_t		audit_sig_uid = INVALID_UID;
> >  pid_t		audit_sig_pid = -1;
> > -u32		audit_sig_sid = 0;
> > +u32		audit_sig_sid;
> 
> All of these are unused outside of audit.c and might as
> well be static and removed from the .h file.
> 

Thanks for reply, I will resend a second version with the recommendation,  
namely make the above static and remove them from the .h file.

Jules

[PATCH 3/4] audit: uninitialize static variables

[Jules Irenge]

Checkpatch tool reports an error at variable declaration

"ERROR: do not initialise statics to 0"

This is due to the fact that these variables are stored in the buffer
In the .bss section, one can not set an initial value
Here we can trust the compiler to automatically set them to zero.
The variable has since been uninitialized.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b1a38a211a9..7d79ecb58b01 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -311,8 +311,8 @@ void audit_panic(const char *message)
 
 static inline int audit_rate_check(void)
 {
-	static unsigned long	last_check = 0;
-	static int		messages   = 0;
+	static unsigned long	last_check;
+	static int		messages;
 	static DEFINE_SPINLOCK(lock);
 	unsigned long		flags;
 	unsigned long		now;
@@ -348,7 +348,7 @@ static inline int audit_rate_check(void)
 */
 void audit_log_lost(const char *message)
 {
-	static unsigned long	last_msg = 0;
+	static unsigned long	last_msg;
 	static DEFINE_SPINLOCK(lock);
 	unsigned long		flags;
 	unsigned long		now;
@@ -713,7 +713,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
 {
 	int rc = 0;
 	struct sk_buff *skb;
-	static unsigned int failed = 0;
+	static unsigned int failed;
 
 	/* NOTE: kauditd_thread takes care of all our locking, we just use
 	 *       the netlink info passed to us (e.g. sk and portid) */
-- 
2.26.2

Re: [PATCH 3/4] audit: uninitialize static variables

[Paul Moore]

On Sat, Aug 1, 2020 at 2:46 PM Jules Irenge <jbi.octave@gmail.com> wrote:
>
> Checkpatch tool reports an error at variable declaration
>
> "ERROR: do not initialise statics to 0"
>
> This is due to the fact that these variables are stored in the buffer
> In the .bss section, one can not set an initial value
> Here we can trust the compiler to automatically set them to zero.
> The variable has since been uninitialized.
>
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  kernel/audit.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

In general this is fine, but it will have to wait until after the
merge window closes.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b1a38a211a9..7d79ecb58b01 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -311,8 +311,8 @@ void audit_panic(const char *message)
>
>  static inline int audit_rate_check(void)
>  {
> -       static unsigned long    last_check = 0;
> -       static int              messages   = 0;
> +       static unsigned long    last_check;
> +       static int              messages;
>         static DEFINE_SPINLOCK(lock);
>         unsigned long           flags;
>         unsigned long           now;
> @@ -348,7 +348,7 @@ static inline int audit_rate_check(void)
>  */
>  void audit_log_lost(const char *message)
>  {
> -       static unsigned long    last_msg = 0;
> +       static unsigned long    last_msg;
>         static DEFINE_SPINLOCK(lock);
>         unsigned long           flags;
>         unsigned long           now;
> @@ -713,7 +713,7 @@ static int kauditd_send_queue(struct sock *sk, u32 portid,
>  {
>         int rc = 0;
>         struct sk_buff *skb;
> -       static unsigned int failed = 0;
> +       static unsigned int failed;
>
>         /* NOTE: kauditd_thread takes care of all our locking, we just use
>          *       the netlink info passed to us (e.g. sk and portid) */
> --
> 2.26.2

-- 
paul moore
www.paul-moore.com

[PATCH 4/4] context_tracking: uninitialize static variables

[Jules Irenge]

Checkpatch tool reports an error at a staic variable declaration

"ERROR: do not initialise statics to false"

This is due to the fact that this variable is stored in the buffer
In the .bss section, one can not set an initial value
Here we can trust the compiler to automatically set them to false.
The variable has since been uninitialized.

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

diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 36a98c48aedc..21881c534152 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -190,7 +190,7 @@ NOKPROBE_SYMBOL(context_tracking_user_exit);
 
 void __init context_tracking_cpu_set(int cpu)
 {
-	static __initdata bool initialized = false;
+	static __initdata bool initialized;
 
 	if (!per_cpu(context_tracking.active, cpu)) {
 		per_cpu(context_tracking.active, cpu) = true;
-- 
2.26.2

Re: [PATCH 4/4] context_tracking: uninitialize static variables

[peterz]

On Sat, Aug 01, 2020 at 07:46:03PM +0100, Jules Irenge wrote:
> Checkpatch tool reports an error at a staic variable declaration
> 
> "ERROR: do not initialise statics to false"
> 
> This is due to the fact that this variable is stored in the buffer
> In the .bss section, one can not set an initial value
> Here we can trust the compiler to automatically set them to false.
> The variable has since been uninitialized.
> 
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
>  kernel/context_tracking.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
> index 36a98c48aedc..21881c534152 100644
> --- a/kernel/context_tracking.c
> +++ b/kernel/context_tracking.c
> @@ -190,7 +190,7 @@ NOKPROBE_SYMBOL(context_tracking_user_exit);
>  
>  void __init context_tracking_cpu_set(int cpu)
>  {
> -	static __initdata bool initialized = false;
> +	static __initdata bool initialized;

So personally I prefer having the '= false' there. It used to be that
compilers were stupid and would put any initialized static variable in
.data, even if it was initialized with 0. But AFAIK compilers are no
longer that stupid.

Re: [PATCH 4/4] context_tracking: uninitialize static variables

[Jules Irenge]

On Mon, 3 Aug 2020, peterz@infradead.org wrote:
 
> So personally I prefer having the '= false' there. It used to be that
> compilers were stupid and would put any initialized static variable in
> .data, even if it was initialized with 0. But AFAIK compilers are no
> longer that stupid.
> 
Thanks for the reply, I completely understand the precaution measure.  

Re: [PATCH 0/4] Checkpatch tool errors clean up

[Joe Perches]

On Sat, 2020-08-01 at 19:45 +0100, Jules Irenge wrote:
> Hi
> I am proposing these 4 patches. 
> I am currently learning the core kernel the hard way. 
> I will appreciate any feedback negative or positive.
> Thanks 

Generally, whitespace only changes outside of drivers/staging
are not encouraged.

> Jules Irenge (4):
>   acct: Add required space between variable and operator
>   audit: uninitialize global variable audit_sig_sid
>   audit: uninitialize static variables
>   context_tracking: uninitialize static variables
> 
>  kernel/acct.c             |  2 +-
>  kernel/audit.c            | 10 +++++-----
>  kernel/context_tracking.c |  2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 

Re: [PATCH 0/4] Checkpatch tool errors clean up

[Jules Irenge]

On Sat, 1 Aug 2020, Joe Perches wrote:

> On Sat, 2020-08-01 at 19:45 +0100, Jules Irenge wrote:
> > Hi
> > I am proposing these 4 patches. 
> > I am currently learning the core kernel the hard way. 
> > I will appreciate any feedback negative or positive.
> > Thanks 
> 
> Generally, whitespace only changes outside of drivers/staging
> are not encouraged.
> 
> > Jules Irenge (4):
> >   acct: Add required space between variable and operator
> >   audit: uninitialize global variable audit_sig_sid
> >   audit: uninitialize static variables
> >   context_tracking: uninitialize static variables
> > 
> >  kernel/acct.c             |  2 +-
> >  kernel/audit.c            | 10 +++++-----
> >  kernel/context_tracking.c |  2 +-
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> > 
> 

Thanks, I take good note.

Kind regards,
Jules