Re: [PATCH 1/1] s390/net: Deletion of unnecessary checks before two function calls

Re: [PATCH 1/1] s390/net: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

The functions debug_unregister() and kfree_fsm() test whether their argument
is NULL and then return immediately. Thus the test around the call
is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/net/claw.c      |  6 ++----
 drivers/s390/net/ctcm_main.c |  6 ++----
 drivers/s390/net/lcs.c       |  6 ++----
 drivers/s390/net/netiucv.c   | 12 ++++--------
 4 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/s390/net/claw.c b/drivers/s390/net/claw.c
index 213e54e..d609ca0 100644
--- a/drivers/s390/net/claw.c
+++ b/drivers/s390/net/claw.c
@@ -109,10 +109,8 @@ static debug_info_t *claw_dbf_trace;
 static void
 claw_unregister_debug_facility(void)
 {
-	if (claw_dbf_setup)
-		debug_unregister(claw_dbf_setup);
-	if (claw_dbf_trace)
-		debug_unregister(claw_dbf_trace);
+	debug_unregister(claw_dbf_setup);
+	debug_unregister(claw_dbf_trace);
 }

 static int
diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index e056dd4..34dc0f3 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -1074,8 +1074,7 @@ static void ctcm_free_netdevice(struct net_device *dev)
 	if (priv) {
 		grp = priv->mpcg;
 		if (grp) {
-			if (grp->fsm)
-				kfree_fsm(grp->fsm);
+			kfree_fsm(grp->fsm);
 			if (grp->xid_skb)
 				dev_kfree_skb(grp->xid_skb);
 			if (grp->rcvd_xid_skb)
@@ -1672,8 +1671,7 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
 		ctcm_free_netdevice(dev);
 	}

-	if (priv->fsm)
-		kfree_fsm(priv->fsm);
+	kfree_fsm(priv->fsm);

 	ccw_device_set_offline(cgdev->cdev[1]);
 	ccw_device_set_offline(cgdev->cdev[0]);
diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
index 0a7d87c..5dfa7dd 100644
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -88,10 +88,8 @@ static debug_info_t *lcs_dbf_trace;
 static void
 lcs_unregister_debug_facility(void)
 {
-	if (lcs_dbf_setup)
-		debug_unregister(lcs_dbf_setup);
-	if (lcs_dbf_trace)
-		debug_unregister(lcs_dbf_trace);
+	debug_unregister(lcs_dbf_setup);
+	debug_unregister(lcs_dbf_trace);
 }

 static int
diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
index 0a87809..bdcc3fe 100644
--- a/drivers/s390/net/netiucv.c
+++ b/drivers/s390/net/netiucv.c
@@ -487,12 +487,9 @@ DEFINE_PER_CPU(char[256], iucv_dbf_txt_buf);

 static void iucv_unregister_dbf_views(void)
 {
-	if (iucv_dbf_setup)
-		debug_unregister(iucv_dbf_setup);
-	if (iucv_dbf_data)
-		debug_unregister(iucv_dbf_data);
-	if (iucv_dbf_trace)
-		debug_unregister(iucv_dbf_trace);
+	debug_unregister(iucv_dbf_setup);
+	debug_unregister(iucv_dbf_data);
+	debug_unregister(iucv_dbf_trace);
 }
 static int iucv_register_dbf_views(void)
 {
@@ -1975,8 +1972,7 @@ static void netiucv_free_netdevice(struct net_device *dev)
 	if (privptr) {
 		if (privptr->conn)
 			netiucv_remove_connection(privptr->conn);
-		if (privptr->fsm)
-			kfree_fsm(privptr->fsm);
+		kfree_fsm(privptr->fsm);
 		privptr->conn = NULL; privptr->fsm = NULL;
 		/* privptr gets freed by free_netdev() */
 	}
-- 
2.1.3

Re: [PATCH 1/1] s390/net: Deletion of unnecessary checks before two function calls

[Dan Carpenter]

This one is buggy.

I'm sorry, but please stop sending these.

For kfree(), at least we all know that kfree() accepts NULL pointer.
But for this one:
1) I don't know what the functions do so I have to look at the code.
2) It's in a arch that I don't compile so cscope isn't set up meaning
   it's hard to find the functions.

You're sending a lot of patches and they are all hard to review and some
of them are buggy and none of them really add any value.  It's a waste
of your time and it's a waste of my time.

regards,
dan carpenter

Re: s390/net: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

> This one is buggy.

I am still interested to clarify this opinion a bit more.


> I'm sorry, but please stop sending these.

I am going to improve more implementation details in affected source files.


> But for this one:
> 1) I don't know what the functions do so I have to look at the code.

I hope that static source code analysis can help here.


> 2) It's in a arch that I don't compile so cscope isn't set up meaning
>    it's hard to find the functions.

Do you find the Coccinelle software also useful for your area?


> You're sending a lot of patches and they are all hard to review and some
> of them are buggy and none of them really add any value.

Thanks for your feedback.


The suggested source code clean-up might result in a measurable effect
depending on the call frequency for the changed functions.
Can I help you in any ways to make corresponding review easier?


> It's a waste of your time and it's a waste of my time.

It can be your choice to reject my update suggestion.

Regards,
Markus

Re: s390/net: Deletion of unnecessary checks before two function calls

[Dan Carpenter]

On Mon, Nov 03, 2014 at 04:55:12PM +0100, SF Markus Elfring wrote:
> > This one is buggy.
> 
> I am still interested to clarify this opinion a bit more.
> 

After your patch then it will print warning messages.

The truth is I think that all these patches are bad and they make the
code harder to read.

Before:  The code is clear and there is no NULL dereference.
 After:  You have to remember that rtw_free_netdev() accepts NULL
	 pointers but free_netdev() does not accept NULL pointers.

The if statements are there for *human* readers to understand and you are
making it harder for humans to understand the code.

Even for kfree(), just removing the if statement is not really the right
fix.  We do it because everyone knows kfree(), but what Julia Lawall
said is the real correct way change the code and make it simpler for
people to understand:

https://lkml.org/lkml/2014/10/31/452

I know it's fun to send automated patches but these make the code worse
and they waste reviewer time.

regards,
dan carpenter

Re: s390/net: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

> After your patch then it will print warning messages.

To which messages do you refer to?


> The truth is I think that all these patches are bad and they make the
> code harder to read.
> 
> Before:  The code is clear and there is no NULL dereference.

Where do you stumble on a null pointer access?


>  After:  You have to remember that rtw_free_netdev() accepts NULL
> 	 pointers but free_netdev() does not accept NULL pointers.

Are any improvements needed for the corresponding documentation to make it
better accessible besides the source code?


> The if statements are there for *human* readers to understand and you are
> making it harder for humans to understand the code.

Is there a target conflict between source code understandability
and software efficiency?


> Even for kfree(), just removing the if statement is not really the right
> fix.  We do it because everyone knows kfree(), but what Julia Lawall
> said is the real correct way change the code and make it simpler for
> people to understand:
> 
> https://lkml.org/lkml/2014/10/31/452

You refer to another update suggestion for the software area
"staging: rtl8188eu".
Do you find adjustments for jump labels easier to accept than the simple
deletion of specific null pointer checks?


> I know it's fun to send automated patches but these make the code worse
> and they waste reviewer time.

I hope that small automated changes can also help to improve affected
source files.

Regards,
Markus

Re: s390/net: Deletion of unnecessary checks before two function calls

[Julia Lawall]

> > After your patch then it will print warning messages.
> >  After:  You have to remember that rtw_free_netdev() accepts NULL
> > 	 pointers but free_netdev() does not accept NULL pointers.
>
> Are any improvements needed for the corresponding documentation to make it
> better accessible besides the source code?

When people are writing or reading code, they will not necessarily look at
the documentation for every function that they use.

> > The if statements are there for *human* readers to understand and you are
> > making it harder for humans to understand the code.
>
> Is there a target conflict between source code understandability
> and software efficiency?

Efficiency is not an issue.  This code is all in rare error handling paths
or in service removal functions.  None of it is in a critical path.  What
is important is to be able to easily check that what needs to be done is
actually done.  Removing null tests makes it more obscure what needs to be
done, because it means that the conditions under which a function needs to
be called (which may be different than the conditions under which it can
be called) are less apparent.

julia

Re: s390/net: Deletion of unnecessary checks before two function calls

[Dan Carpenter]

On Mon, Nov 03, 2014 at 05:50:48PM +0100, SF Markus Elfring wrote:
> > After your patch then it will print warning messages.
> 
> To which messages do you refer to?
> 

Open your eyeballs up and read the code.

> 
> > The truth is I think that all these patches are bad and they make the
> > code harder to read.
> > 
> > Before:  The code is clear and there is no NULL dereference.
> 
> Where do you stumble on a null pointer access?
> 

I'm not talking about bugs, I'm talking about code clarity.  Before is
more clear than after.

> 
> >  After:  You have to remember that rtw_free_netdev() accepts NULL
> > 	 pointers but free_netdev() does not accept NULL pointers.
> 
> Are any improvements needed for the corresponding documentation to make it
> better accessible besides the source code?
> 

Documentation doesn't reduce the number of things to remember it just
documents it.  Meanwhile if we leave the code as-is there is no need for
documentation because the code is clear.

> 
> > The if statements are there for *human* readers to understand and you are
> > making it harder for humans to understand the code.
> 
> Is there a target conflict between source code understandability
> and software efficiency?

If you can benchmark the code and the new code is faster then, yes, this
patch is good and we will apply it.  If you have no benchmarks then do
not send the patch.

> 
> > Even for kfree(), just removing the if statement is not really the right
> > fix.  We do it because everyone knows kfree(), but what Julia Lawall
> > said is the real correct way change the code and make it simpler for
> > people to understand:
> > 
> > https://lkml.org/lkml/2014/10/31/452
> 
> You refer to another update suggestion for the software area
> "staging: rtl8188eu".
> Do you find adjustments for jump labels easier to accept than the simple
> deletion of specific null pointer checks?

Yes.

> 
> 
> > I know it's fun to send automated patches but these make the code worse
> > and they waste reviewer time.
> 
> I hope that small automated changes can also help to improve affected
> source files.

No.  The changes make the code less clear.

regards,
dan carpenter

Re: s390/net: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

> If you can benchmark the code and the new code is faster then, yes, this
> patch is good and we will apply it.

I guess that I do not have enough resources myself to measure different run time
effects in a S390 environment.


> If you have no benchmarks then do not send the patch.

Are other software developers and testers eventually interested to try a few
pointer check adjustments out a bit more?

Regards,
Markus

Re: [PATCH 1/1] s390/net: Deletion of unnecessary checks before two function calls

[Brian Norris]

I went digging through some of Markus's old patch history, and noticed
this...

On Mon, Nov 03, 2014 at 12:50:59PM +0300, Dan Carpenter wrote:
> This one is buggy.
> 
> I'm sorry, but please stop sending these.

I'm tending to concur.

> For kfree(), at least we all know that kfree() accepts NULL pointer.
> But for this one:
> 1) I don't know what the functions do so I have to look at the code.
> 2) It's in a arch that I don't compile so cscope isn't set up meaning
>    it's hard to find the functions.
> 
> You're sending a lot of patches and they are all hard to review and some
> of them are buggy and none of them really add any value.  It's a waste
> of your time and it's a waste of my time.

And you're still sending buggy patches that exhibit the same qualities.
They're still wasting Dan's time, and now they're wasting mine.

I appreciate automated checkers where they provide added value, but I
really feel you haven't done your diligence on them.

Regards,
Brian

Re: [PATCH 1/1] s390/net: Deletion of unnecessary checks before two function calls

[Ursula Braun]

I agree with your proposed debug_unregister() changes, but not with your
kfree_fsm() change.

Regards, Ursula Braun

On Fri, 2014-10-31 at 18:40 +0100, SF Markus Elfring wrote:
> The functions debug_unregister() and kfree_fsm() test whether their argument
> is NULL and then return immediately. Thus the test around the call
> is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/net/claw.c      |  6 ++----
>  drivers/s390/net/ctcm_main.c |  6 ++----
>  drivers/s390/net/lcs.c       |  6 ++----
>  drivers/s390/net/netiucv.c   | 12 ++++--------
>  4 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/s390/net/claw.c b/drivers/s390/net/claw.c
> index 213e54e..d609ca0 100644
> --- a/drivers/s390/net/claw.c
> +++ b/drivers/s390/net/claw.c
> @@ -109,10 +109,8 @@ static debug_info_t *claw_dbf_trace;
>  static void
>  claw_unregister_debug_facility(void)
>  {
> -	if (claw_dbf_setup)
> -		debug_unregister(claw_dbf_setup);
> -	if (claw_dbf_trace)
> -		debug_unregister(claw_dbf_trace);
> +	debug_unregister(claw_dbf_setup);
> +	debug_unregister(claw_dbf_trace);
>  }
> 
>  static int
> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index e056dd4..34dc0f3 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -1074,8 +1074,7 @@ static void ctcm_free_netdevice(struct net_device *dev)
>  	if (priv) {
>  		grp = priv->mpcg;
>  		if (grp) {
> -			if (grp->fsm)
> -				kfree_fsm(grp->fsm);
> +			kfree_fsm(grp->fsm);
>  			if (grp->xid_skb)
>  				dev_kfree_skb(grp->xid_skb);
>  			if (grp->rcvd_xid_skb)
> @@ -1672,8 +1671,7 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
>  		ctcm_free_netdevice(dev);
>  	}
> 
> -	if (priv->fsm)
> -		kfree_fsm(priv->fsm);
> +	kfree_fsm(priv->fsm);
> 
>  	ccw_device_set_offline(cgdev->cdev[1]);
>  	ccw_device_set_offline(cgdev->cdev[0]);
> diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
> index 0a7d87c..5dfa7dd 100644
> --- a/drivers/s390/net/lcs.c
> +++ b/drivers/s390/net/lcs.c
> @@ -88,10 +88,8 @@ static debug_info_t *lcs_dbf_trace;
>  static void
>  lcs_unregister_debug_facility(void)
>  {
> -	if (lcs_dbf_setup)
> -		debug_unregister(lcs_dbf_setup);
> -	if (lcs_dbf_trace)
> -		debug_unregister(lcs_dbf_trace);
> +	debug_unregister(lcs_dbf_setup);
> +	debug_unregister(lcs_dbf_trace);
>  }
> 
>  static int
> diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
> index 0a87809..bdcc3fe 100644
> --- a/drivers/s390/net/netiucv.c
> +++ b/drivers/s390/net/netiucv.c
> @@ -487,12 +487,9 @@ DEFINE_PER_CPU(char[256], iucv_dbf_txt_buf);
> 
>  static void iucv_unregister_dbf_views(void)
>  {
> -	if (iucv_dbf_setup)
> -		debug_unregister(iucv_dbf_setup);
> -	if (iucv_dbf_data)
> -		debug_unregister(iucv_dbf_data);
> -	if (iucv_dbf_trace)
> -		debug_unregister(iucv_dbf_trace);
> +	debug_unregister(iucv_dbf_setup);
> +	debug_unregister(iucv_dbf_data);
> +	debug_unregister(iucv_dbf_trace);
>  }
>  static int iucv_register_dbf_views(void)
>  {
> @@ -1975,8 +1972,7 @@ static void netiucv_free_netdevice(struct net_device *dev)
>  	if (privptr) {
>  		if (privptr->conn)
>  			netiucv_remove_connection(privptr->conn);
> -		if (privptr->fsm)
> -			kfree_fsm(privptr->fsm);
> +		kfree_fsm(privptr->fsm);
>  		privptr->conn = NULL; privptr->fsm = NULL;
>  		/* privptr gets freed by free_netdev() */
>  	}

Re: s390/net: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

> I agree with your proposed debug_unregister() changes, but not with your
> kfree_fsm() change.

Why do you want to keep an additional null pointer check before the call
of the kfree_fsm() function within the implementation of the
netiucv_free_netdevice() function?

Regards,
Markus

Re: s390/net: Deletion of unnecessary checks before two function calls

[Dan Carpenter]

On Mon, Nov 03, 2014 at 05:10:35PM +0100, SF Markus Elfring wrote:
> > I agree with your proposed debug_unregister() changes, but not with your
> > kfree_fsm() change.
> 
> Why do you want to keep an additional null pointer check before the call
> of the kfree_fsm() function within the implementation of the
> netiucv_free_netdevice() function?

Think about how long it takes you to figure this out what the bug is and
then remember that we have to spend that same amount of time multiplied
by the number of patches you have sent.

regards,
dan carpenter

Re: s390/net: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

> What do you want me to clarify?  Do you still not see the bug?

Would you like to point out that you notice the warning message
"fsm: kfree_fsm called with NULL argument" after my update suggestion?

How do you think about to share a bit more information about the
corresponding function call graph from your test system?

Regards,
Markus

[PATCH 1/1] s390/pci: Deletion of unnecessary checks before the function call "debug_unregister"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 22 Nov 2014 15:00:55 +0100

The debug_unregister() function performs also input parameter validation.
Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/pci/pci_debug.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
index eec598c..18403a7 100644
--- a/arch/s390/pci/pci_debug.c
+++ b/arch/s390/pci/pci_debug.c
@@ -158,10 +158,8 @@ int __init zpci_debug_init(void)
 
 void zpci_debug_exit(void)
 {
-	if (pci_debug_msg_id)
-		debug_unregister(pci_debug_msg_id);
-	if (pci_debug_err_id)
-		debug_unregister(pci_debug_err_id);
+	debug_unregister(pci_debug_msg_id);
+	debug_unregister(pci_debug_err_id);
 
 	debugfs_remove(debugfs_root);
 }
-- 
2.1.3

Re: [PATCH 1/1] s390/pci: Deletion of unnecessary checks before the function call "debug_unregister"

[Sebastian Ott]

On Sat, 22 Nov 2014, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 22 Nov 2014 15:00:55 +0100
> 
> The debug_unregister() function performs also input parameter validation.
> Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/pci/pci_debug.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
> index eec598c..18403a7 100644
> --- a/arch/s390/pci/pci_debug.c
> +++ b/arch/s390/pci/pci_debug.c
> @@ -158,10 +158,8 @@ int __init zpci_debug_init(void)
> 
>  void zpci_debug_exit(void)
>  {
> -	if (pci_debug_msg_id)
> -		debug_unregister(pci_debug_msg_id);
> -	if (pci_debug_err_id)
> -		debug_unregister(pci_debug_err_id);
> +	debug_unregister(pci_debug_msg_id);
> +	debug_unregister(pci_debug_err_id);
> 
>  	debugfs_remove(debugfs_root);
>  }

Applied.

Thanks,
Sebastian

[PATCH] s390/process: Delete an unnecessary check before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 24 Jun 2015 22:40:30 +0200

The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index dc5edc2..22e6448 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -81,8 +81,7 @@ void release_thread(struct task_struct *dead_task)
 
 void arch_release_task_struct(struct task_struct *tsk)
 {
-	if (tsk->thread.vxrs)
-		kfree(tsk->thread.vxrs);
+	kfree(tsk->thread.vxrs);
 }
 
 int copy_thread(unsigned long clone_flags, unsigned long new_stackp,
-- 
2.4.4

Re: [PATCH] s390/process: Delete an unnecessary check before the function call "kfree"

[walter harms]

Am 24.06.2015 22:48, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 24 Jun 2015 22:40:30 +0200
> 
> The kfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/kernel/process.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
> index dc5edc2..22e6448 100644
> --- a/arch/s390/kernel/process.c
> +++ b/arch/s390/kernel/process.c
> @@ -81,8 +81,7 @@ void release_thread(struct task_struct *dead_task)
>  
>  void arch_release_task_struct(struct task_struct *tsk)
>  {
> -	if (tsk->thread.vxrs)
> -		kfree(tsk->thread.vxrs);
> +	kfree(tsk->thread.vxrs);
>  }
>  
>  int copy_thread(unsigned long clone_flags, unsigned long new_stackp,

maybe the intention was to check tsk != NULL ?

re,
 wh

Re: [PATCH] s390/process: Delete an unnecessary check before the function call "kfree"

[Heiko Carstens]

On Thu, Jun 25, 2015 at 12:02:28PM +0200, walter harms wrote:
> 
> Am 24.06.2015 22:48, schrieb SF Markus Elfring:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 24 Jun 2015 22:40:30 +0200
> > 
> > The kfree() function tests whether its argument is NULL and then
> > returns immediately. Thus the test around the call is not needed.
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  arch/s390/kernel/process.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
> > index dc5edc2..22e6448 100644
> > --- a/arch/s390/kernel/process.c
> > +++ b/arch/s390/kernel/process.c
> > @@ -81,8 +81,7 @@ void release_thread(struct task_struct *dead_task)
> >  
> >  void arch_release_task_struct(struct task_struct *tsk)
> >  {
> > -	if (tsk->thread.vxrs)
> > -		kfree(tsk->thread.vxrs);
> > +	kfree(tsk->thread.vxrs);
> >  }

This code will be changed soon anyway. So it would be rather pointless
to apply your patch now. Thanks anyway!

> >  int copy_thread(unsigned long clone_flags, unsigned long new_stackp,
> maybe the intention was to check tsk != NULL ?

No.

[PATCH] SCSI-zfcp: Delete unnecessary checks before the function call "mempool_destroy"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Nov 2015 14:19:03 +0100

The mempool_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/scsi/zfcp_aux.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
index c00ac46..abc9c93 100644
--- a/drivers/s390/scsi/zfcp_aux.c
+++ b/drivers/s390/scsi/zfcp_aux.c
@@ -248,20 +248,13 @@ static int zfcp_allocate_low_mem_buffers(struct zfcp_adapter *adapter)
 
 static void zfcp_free_low_mem_buffers(struct zfcp_adapter *adapter)
 {
-	if (adapter->pool.erp_req)
-		mempool_destroy(adapter->pool.erp_req);
-	if (adapter->pool.scsi_req)
-		mempool_destroy(adapter->pool.scsi_req);
-	if (adapter->pool.scsi_abort)
-		mempool_destroy(adapter->pool.scsi_abort);
-	if (adapter->pool.qtcb_pool)
-		mempool_destroy(adapter->pool.qtcb_pool);
-	if (adapter->pool.status_read_req)
-		mempool_destroy(adapter->pool.status_read_req);
-	if (adapter->pool.sr_data)
-		mempool_destroy(adapter->pool.sr_data);
-	if (adapter->pool.gid_pn)
-		mempool_destroy(adapter->pool.gid_pn);
+	mempool_destroy(adapter->pool.erp_req);
+	mempool_destroy(adapter->pool.scsi_req);
+	mempool_destroy(adapter->pool.scsi_abort);
+	mempool_destroy(adapter->pool.qtcb_pool);
+	mempool_destroy(adapter->pool.status_read_req);
+	mempool_destroy(adapter->pool.sr_data);
+	mempool_destroy(adapter->pool.gid_pn);
 }
 
 /**
-- 
2.6.2

Re: [PATCH] SCSI-zfcp: Delete unnecessary checks before the function call "mempool_destroy"

[Benjamin Block]

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

Hej Markus,

On 14:23 Mon 16 Nov     , SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 16 Nov 2015 14:19:03 +0100
> 
> The mempool_destroy() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the calls is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/scsi/zfcp_aux.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
> index c00ac46..abc9c93 100644
> --- a/drivers/s390/scsi/zfcp_aux.c
> +++ b/drivers/s390/scsi/zfcp_aux.c
> @@ -248,20 +248,13 @@ static int zfcp_allocate_low_mem_buffers(struct zfcp_adapter *adapter)
> 
>  static void zfcp_free_low_mem_buffers(struct zfcp_adapter *adapter)
>  {
> -	if (adapter->pool.erp_req)
> -		mempool_destroy(adapter->pool.erp_req);
> -	if (adapter->pool.scsi_req)
> -		mempool_destroy(adapter->pool.scsi_req);
> -	if (adapter->pool.scsi_abort)
> -		mempool_destroy(adapter->pool.scsi_abort);
> -	if (adapter->pool.qtcb_pool)
> -		mempool_destroy(adapter->pool.qtcb_pool);
> -	if (adapter->pool.status_read_req)
> -		mempool_destroy(adapter->pool.status_read_req);
> -	if (adapter->pool.sr_data)
> -		mempool_destroy(adapter->pool.sr_data);
> -	if (adapter->pool.gid_pn)
> -		mempool_destroy(adapter->pool.gid_pn);
> +	mempool_destroy(adapter->pool.erp_req);
> +	mempool_destroy(adapter->pool.scsi_req);
> +	mempool_destroy(adapter->pool.scsi_abort);
> +	mempool_destroy(adapter->pool.qtcb_pool);
> +	mempool_destroy(adapter->pool.status_read_req);
> +	mempool_destroy(adapter->pool.sr_data);
> +	mempool_destroy(adapter->pool.gid_pn);
>  }
> 
>  /**
> -- 
> 2.6.2

Looks good to me, will have to wait though till Steffen is back around.

Reviewed-by: Benjamin Block <bblock@linux.vnet.ibm.com>


                                                    Beste Grüße / Best regards,
                                                      - Benjamin Block
-- 
Linux on z Systems Development         /         IBM Systems & Technology Group
		  IBM Deutschland Research & Development GmbH 
Vorsitz. AufsR.: Martina Koederitz     /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

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

Re: [PATCH] SCSI-zfcp: Delete unnecessary checks before the function call "mempool_destroy"

[Steffen Maier]

Markus, thanks, I queued it for next time I send features upstream.
Don't hold your breath, fixes will go first.

Benjamin, thanks for the review.

Will add an informational comment that this depends on 4e3ca3e033d1 
("mm/mempool: allow NULL `pool' pointer in mempool_destroy()").

On 12/04/2015 06:57 PM, Benjamin Block wrote:
> Hej Markus,
>
> On 14:23 Mon 16 Nov     , SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Mon, 16 Nov 2015 14:19:03 +0100
>>
>> The mempool_destroy() function tests whether its argument is NULL
>> and then returns immediately. Thus the test around the calls is not needed.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>   drivers/s390/scsi/zfcp_aux.c | 21 +++++++--------------
>>   1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
>> index c00ac46..abc9c93 100644
>> --- a/drivers/s390/scsi/zfcp_aux.c
>> +++ b/drivers/s390/scsi/zfcp_aux.c
>> @@ -248,20 +248,13 @@ static int zfcp_allocate_low_mem_buffers(struct zfcp_adapter *adapter)
>>
>>   static void zfcp_free_low_mem_buffers(struct zfcp_adapter *adapter)
>>   {
>> -	if (adapter->pool.erp_req)
>> -		mempool_destroy(adapter->pool.erp_req);
>> -	if (adapter->pool.scsi_req)
>> -		mempool_destroy(adapter->pool.scsi_req);
>> -	if (adapter->pool.scsi_abort)
>> -		mempool_destroy(adapter->pool.scsi_abort);
>> -	if (adapter->pool.qtcb_pool)
>> -		mempool_destroy(adapter->pool.qtcb_pool);
>> -	if (adapter->pool.status_read_req)
>> -		mempool_destroy(adapter->pool.status_read_req);
>> -	if (adapter->pool.sr_data)
>> -		mempool_destroy(adapter->pool.sr_data);
>> -	if (adapter->pool.gid_pn)
>> -		mempool_destroy(adapter->pool.gid_pn);
>> +	mempool_destroy(adapter->pool.erp_req);
>> +	mempool_destroy(adapter->pool.scsi_req);
>> +	mempool_destroy(adapter->pool.scsi_abort);
>> +	mempool_destroy(adapter->pool.qtcb_pool);
>> +	mempool_destroy(adapter->pool.status_read_req);
>> +	mempool_destroy(adapter->pool.sr_data);
>> +	mempool_destroy(adapter->pool.gid_pn);
>>   }
>>
>>   /**
>> --
>> 2.6.2
>
> Looks good to me, will have to wait though till Steffen is back around.
>
> Reviewed-by: Benjamin Block <bblock@linux.vnet.ibm.com>
>
>
>                                                      Beste Grüße / Best regards,
>                                                        - Benjamin Block
>

-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[PATCH] s390: Delete unnecessary checks before the function call "debug_unregister"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Nov 2015 14:45:40 +0100

The debug_unregister() function performs also input parameter validation.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/cio/chsc_sch.c      | 3 +--
 drivers/s390/cio/cio.c           | 9 +++------
 drivers/s390/cio/qdio_debug.c    | 6 ++----
 drivers/s390/crypto/zcrypt_api.c | 6 ++----
 4 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/cio/chsc_sch.c b/drivers/s390/cio/chsc_sch.c
index 213159d..378c571 100644
--- a/drivers/s390/cio/chsc_sch.c
+++ b/drivers/s390/cio/chsc_sch.c
@@ -185,8 +185,7 @@ static int __init chsc_init_dbfs(void)
 	debug_set_level(chsc_debug_log_id, 2);
 	return 0;
 out:
-	if (chsc_debug_msg_id)
-		debug_unregister(chsc_debug_msg_id);
+	debug_unregister(chsc_debug_msg_id);
 	return -ENOMEM;
 }
 
diff --git a/drivers/s390/cio/cio.c b/drivers/s390/cio/cio.c
index e0d0295..2d18205 100644
--- a/drivers/s390/cio/cio.c
+++ b/drivers/s390/cio/cio.c
@@ -76,12 +76,9 @@ static int __init cio_debug_init(void)
 	return 0;
 
 out_unregister:
-	if (cio_debug_msg_id)
-		debug_unregister(cio_debug_msg_id);
-	if (cio_debug_trace_id)
-		debug_unregister(cio_debug_trace_id);
-	if (cio_debug_crw_id)
-		debug_unregister(cio_debug_crw_id);
+	debug_unregister(cio_debug_msg_id);
+	debug_unregister(cio_debug_trace_id);
+	debug_unregister(cio_debug_crw_id);
 	return -1;
 }
 
diff --git a/drivers/s390/cio/qdio_debug.c b/drivers/s390/cio/qdio_debug.c
index f1f3baa..b6fc147 100644
--- a/drivers/s390/cio/qdio_debug.c
+++ b/drivers/s390/cio/qdio_debug.c
@@ -366,8 +366,6 @@ void qdio_debug_exit(void)
 {
 	qdio_clear_dbf_list();
 	debugfs_remove(debugfs_root);
-	if (qdio_dbf_setup)
-		debug_unregister(qdio_dbf_setup);
-	if (qdio_dbf_error)
-		debug_unregister(qdio_dbf_error);
+	debug_unregister(qdio_dbf_setup);
+	debug_unregister(qdio_dbf_error);
 }
diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index 9f8fa42..5d3d04c 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -1428,10 +1428,8 @@ int __init zcrypt_debug_init(void)
 void zcrypt_debug_exit(void)
 {
 	debugfs_remove(debugfs_root);
-	if (zcrypt_dbf_common)
-		debug_unregister(zcrypt_dbf_common);
-	if (zcrypt_dbf_devices)
-		debug_unregister(zcrypt_dbf_devices);
+	debug_unregister(zcrypt_dbf_common);
+	debug_unregister(zcrypt_dbf_devices);
 }
 
 /**
-- 
2.6.2

Re: [PATCH] s390: Delete unnecessary checks before the function call "debug_unregister"

[Heiko Carstens]

On Mon, Nov 16, 2015 at 03:15:17PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 16 Nov 2015 14:45:40 +0100
> 
> The debug_unregister() function performs also input parameter validation.
> Thus the test around the calls is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/cio/chsc_sch.c      | 3 +--
>  drivers/s390/cio/cio.c           | 9 +++------
>  drivers/s390/cio/qdio_debug.c    | 6 ++----
>  drivers/s390/crypto/zcrypt_api.c | 6 ++----
>  4 files changed, 8 insertions(+), 16 deletions(-)

Applied, thanks!

[PATCH] s390-ctcm: Delete unnecessary checks before the function call "channel_remove"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 17 Nov 2015 20:10:02 +0100

The channel_remove() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/s390/net/ctcm_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index 05c37d6..c3e2252 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -1677,11 +1677,8 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
 
 	ccw_device_set_offline(cgdev->cdev[1]);
 	ccw_device_set_offline(cgdev->cdev[0]);
-
-	if (priv->channel[CTCM_READ])
-		channel_remove(priv->channel[CTCM_READ]);
-	if (priv->channel[CTCM_WRITE])
-		channel_remove(priv->channel[CTCM_WRITE]);
+	channel_remove(priv->channel[CTCM_READ]);
+	channel_remove(priv->channel[CTCM_WRITE]);
 	priv->channel[CTCM_READ] = priv->channel[CTCM_WRITE] = NULL;
 
 	return 0;
-- 
2.6.2

Re: [PATCH] s390-ctcm: Delete unnecessary checks before the function call "channel_remove"

[Ursula Braun]

Applied to our local git. Thanks. It will be part of my next patch
submission for net-next.

Kind regards, Ursula Braun, IBM Germany

On Tue, 2015-11-17 at 20:20 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 17 Nov 2015 20:10:02 +0100
> 
> The channel_remove() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the calls is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/s390/net/ctcm_main.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index 05c37d6..c3e2252 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -1677,11 +1677,8 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
> 
>  	ccw_device_set_offline(cgdev->cdev[1]);
>  	ccw_device_set_offline(cgdev->cdev[0]);
> -
> -	if (priv->channel[CTCM_READ])
> -		channel_remove(priv->channel[CTCM_READ]);
> -	if (priv->channel[CTCM_WRITE])
> -		channel_remove(priv->channel[CTCM_WRITE]);
> +	channel_remove(priv->channel[CTCM_READ]);
> +	channel_remove(priv->channel[CTCM_WRITE]);
>  	priv->channel[CTCM_READ] = priv->channel[CTCM_WRITE] = NULL;
> 
>  	return 0;

[PATCH] s390/pci: Delete an unnecessary check before the function call "pci_dev_put"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Jul 2016 20:15:17 +0200

The pci_dev_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/pci/pci_event.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index fb2a9a5..c2b27ad 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -145,8 +145,7 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
 	default:
 		break;
 	}
-	if (pdev)
-		pci_dev_put(pdev);
+	pci_dev_put(pdev);
 }
 
 void zpci_event_availability(void *data)
-- 
2.9.1

Re: [PATCH] s390/pci: Delete an unnecessary check before the function call "pci_dev_put"

[Martin Schwidefsky]

On Sat, 16 Jul 2016 20:40:23 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Jul 2016 20:15:17 +0200
> 
> The pci_dev_put() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/s390/pci/pci_event.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index fb2a9a5..c2b27ad 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
> @@ -145,8 +145,7 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
>  	default:
>  		break;
>  	}
> -	if (pdev)
> -		pci_dev_put(pdev);
> +	pci_dev_put(pdev);
>  }
> 
>  void zpci_event_availability(void *data)

Applied,  thanks.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.