Re: [PATCH 1/1] Sony-laptop: Deletion of an unnecessary check before the function call "pci_dev_put"

Re: [PATCH 1/1] Sony-laptop: Deletion of an unnecessary check before the function call "pci_dev_put"

[Darren Hart]

On Mon, Nov 24, 2014 at 10:04:16PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 24 Nov 2014 22:00:21 +0100
> 
> 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>

Thanks Markus, queued.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[Darren Hart]

On Mon, Nov 24, 2014 at 08:40:22PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 24 Nov 2014 20:30:29 +0100
> 
> The backlight_device_unregister() 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>

Thanks Markus, queued.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 24 Nov 2014 20:30:29 +0100

The backlight_device_unregister() 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>
---
 drivers/platform/x86/asus-laptop.c    | 3 +--
 drivers/platform/x86/asus-wmi.c       | 3 +--
 drivers/platform/x86/eeepc-laptop.c   | 3 +--
 drivers/platform/x86/fujitsu-laptop.c | 6 ++----
 drivers/platform/x86/ideapad-laptop.c | 3 +--
 drivers/platform/x86/intel_oaktrail.c | 3 +--
 drivers/platform/x86/msi-wmi.c        | 3 +--
 drivers/platform/x86/sony-laptop.c    | 3 +--
 drivers/platform/x86/toshiba_acpi.c   | 3 +--
 9 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
index 7f4dc6f..11fac1a 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -843,8 +843,7 @@ static int asus_backlight_init(struct asus_laptop *asus)
 
 static void asus_backlight_exit(struct asus_laptop *asus)
 {
-	if (asus->backlight_device)
-		backlight_device_unregister(asus->backlight_device);
+	backlight_device_unregister(asus->backlight_device);
 	asus->backlight_device = NULL;
 }
 
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 21fc932..7543a56 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1308,8 +1308,7 @@ static int asus_wmi_backlight_init(struct asus_wmi *asus)
 
 static void asus_wmi_backlight_exit(struct asus_wmi *asus)
 {
-	if (asus->backlight_device)
-		backlight_device_unregister(asus->backlight_device);
+	backlight_device_unregister(asus->backlight_device);
 
 	asus->backlight_device = NULL;
 }
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index bd533c2..9bc7eb7 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -1158,8 +1158,7 @@ static int eeepc_backlight_init(struct eeepc_laptop *eeepc)
 
 static void eeepc_backlight_exit(struct eeepc_laptop *eeepc)
 {
-	if (eeepc->backlight_device)
-		backlight_device_unregister(eeepc->backlight_device);
+	backlight_device_unregister(eeepc->backlight_device);
 	eeepc->backlight_device = NULL;
 }
 
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 87aa28c..f1a670b 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -1147,8 +1147,7 @@ fail_hotkey1:
 fail_hotkey:
 	platform_driver_unregister(&fujitsupf_driver);
 fail_backlight:
-	if (fujitsu->bl_device)
-		backlight_device_unregister(fujitsu->bl_device);
+	backlight_device_unregister(fujitsu->bl_device);
 fail_sysfs_group:
 	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
 			   &fujitsupf_attribute_group);
@@ -1172,8 +1171,7 @@ static void __exit fujitsu_cleanup(void)
 
 	platform_driver_unregister(&fujitsupf_driver);
 
-	if (fujitsu->bl_device)
-		backlight_device_unregister(fujitsu->bl_device);
+	backlight_device_unregister(fujitsu->bl_device);
 
 	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
 			   &fujitsupf_attribute_group);
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index ed494f3..3163061 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -729,8 +729,7 @@ static int ideapad_backlight_init(struct ideapad_private *priv)
 
 static void ideapad_backlight_exit(struct ideapad_private *priv)
 {
-	if (priv->blightdev)
-		backlight_device_unregister(priv->blightdev);
+	backlight_device_unregister(priv->blightdev);
 	priv->blightdev = NULL;
 }
 
diff --git a/drivers/platform/x86/intel_oaktrail.c b/drivers/platform/x86/intel_oaktrail.c
index 4bc9604..0dd72cf 100644
--- a/drivers/platform/x86/intel_oaktrail.c
+++ b/drivers/platform/x86/intel_oaktrail.c
@@ -271,8 +271,7 @@ static int oaktrail_backlight_init(void)
 
 static void oaktrail_backlight_exit(void)
 {
-	if (oaktrail_bl_device)
-		backlight_device_unregister(oaktrail_bl_device);
+	backlight_device_unregister(oaktrail_bl_device);
 }
 
 static int oaktrail_probe(struct platform_device *pdev)
diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
index 70222f2..6d2bac0 100644
--- a/drivers/platform/x86/msi-wmi.c
+++ b/drivers/platform/x86/msi-wmi.c
@@ -354,8 +354,7 @@ static void __exit msi_wmi_exit(void)
 		sparse_keymap_free(msi_wmi_input_dev);
 		input_unregister_device(msi_wmi_input_dev);
 	}
-	if (backlight)
-		backlight_device_unregister(backlight);
+	backlight_device_unregister(backlight);
 }
 
 module_init(msi_wmi_init);
diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 26ad9ff..a48cdc7 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -3141,8 +3141,7 @@ static void sony_nc_backlight_setup(void)
 
 static void sony_nc_backlight_cleanup(void)
 {
-	if (sony_bl_props.dev)
-		backlight_device_unregister(sony_bl_props.dev);
+	backlight_device_unregister(sony_bl_props.dev);
 }
 
 static int sony_nc_add(struct acpi_device *device)
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index d0dce73..703a68a 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1647,8 +1647,7 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
 		rfkill_destroy(dev->bt_rfk);
 	}
 
-	if (dev->backlight_dev)
-		backlight_device_unregister(dev->backlight_dev);
+	backlight_device_unregister(dev->backlight_dev);
 
 	if (dev->illumination_supported)
 		led_classdev_unregister(&dev->led_dev);
-- 
2.1.3

[PATCH 1/1] Sony-laptop: Deletion of an unnecessary check before the function call "pci_dev_put"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 24 Nov 2014 22:00:21 +0100

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>
---
 drivers/platform/x86/sony-laptop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 26ad9ff..22b173d 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -3717,8 +3717,7 @@ static void sony_pic_detect_device_type(struct sony_pic_dev *dev)
 	dev->event_types = type2_events;
 
 out:
-	if (pcidev)
-		pci_dev_put(pcidev);
+	pci_dev_put(pcidev);
 
 	pr_info("detected Type%d model\n",
 		dev->model == SONYPI_DEVICE_TYPE1 ? 1 :
-- 
2.1.3

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[Anisse Astier]

Hi Markus,

Le lundi 24 novembre 2014, 20:40:22 SF Markus Elfring a écrit :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 24 Nov 2014 20:30:29 +0100
> 
> The backlight_device_unregister() 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.

What script was used ? Is it in scripts/coccinelle ?


> diff --git a/drivers/platform/x86/msi-wmi.c b/drivers/platform/x86/msi-wmi.c
> index 70222f2..6d2bac0 100644
> --- a/drivers/platform/x86/msi-wmi.c
> +++ b/drivers/platform/x86/msi-wmi.c
> @@ -354,8 +354,7 @@ static void __exit msi_wmi_exit(void)
>  		sparse_keymap_free(msi_wmi_input_dev);
>  		input_unregister_device(msi_wmi_input_dev);
>  	}
> -	if (backlight)
> -		backlight_device_unregister(backlight);
> +	backlight_device_unregister(backlight);
>  }
> 
>  module_init(msi_wmi_init);

For this part:
Acked-by: Anisse Astier <anisse@astier.eu>

Regards,

Anisse

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[SF Markus Elfring]

>> This issue was detected by using the Coccinelle software.
> 
> What script was used ?

A semantic patch approach which I published on the mailing lists in March
is in action on my software development system for a while.


> Is it in scripts/coccinelle ?

Not yet.

I hope that the involved update suggestions got sufficient positive feedback
to integrate five scripts there.

Regards,
Markus

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[Julia Lawall]

On Mon, 24 Nov 2014, SF Markus Elfring wrote:

> >> This issue was detected by using the Coccinelle software.
> >
> > What script was used ?
>
> A semantic patch approach which I published on the mailing lists in March
> is in action on my software development system for a while.
>
>
> > Is it in scripts/coccinelle ?
>
> Not yet.
>
> I hope that the involved update suggestions got sufficient positive feedback
> to integrate five scripts there.

The current scripts are very complicated, involving the interaction
between multiple scripts and a database, and I think they are not very
suitable for make coccicheck.  Also, the idea of removing the null checks
is not appropriate in all contexts.  Perhaps it could be possible to add
a script to the Linux kernel that handles a number of common cases for
which removing the null test is widely considered to be desirable.

julia

[PATCH] tc1100-wmi: Delete an unnecessary check before the function call "kfree"

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Jun 2015 21:21:16 +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>
---
 drivers/platform/x86/tc1100-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/tc1100-wmi.c b/drivers/platform/x86/tc1100-wmi.c
index e365425..89aa976 100644
--- a/drivers/platform/x86/tc1100-wmi.c
+++ b/drivers/platform/x86/tc1100-wmi.c
@@ -82,7 +82,7 @@ static int get_state(u32 *out, u8 instance)
 		tmp = 0;
 	}
 
-	if (result.length > 0 && result.pointer)
+	if (result.length > 0)
 		kfree(result.pointer);
 
 	switch (instance) {
-- 
2.4.4

Re: [PATCH] tc1100-wmi: Delete an unnecessary check before the function call "kfree"

[Darren Hart]

On Fri, Jun 26, 2015 at 09:28:08PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 26 Jun 2015 21:21:16 +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>

Queued, thanks.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[Darren Hart]

On Thu, Nov 27, 2014 at 07:13:10PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 24 Nov 2014, SF Markus Elfring wrote:
> 
> > >> This issue was detected by using the Coccinelle software.
> > >
> > > What script was used ?
> >
> > A semantic patch approach which I published on the mailing lists in March
> > is in action on my software development system for a while.
> >
> >
> > > Is it in scripts/coccinelle ?
> >
> > Not yet.
> >
> > I hope that the involved update suggestions got sufficient positive feedback
> > to integrate five scripts there.
> 
> The current scripts are very complicated, involving the interaction
> between multiple scripts and a database, and I think they are not very
> suitable for make coccicheck.  Also, the idea of removing the null checks
> is not appropriate in all contexts.  Perhaps it could be possible to add
> a script to the Linux kernel that handles a number of common cases for
> which removing the null test is widely considered to be desirable.
> 
> julia
> 

Julia, do you have any particular objection to this specific patch? I didn't see
a reason to prevent it going in.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[Julia Lawall]

On Fri, 26 Jun 2015, Darren Hart wrote:

> On Thu, Nov 27, 2014 at 07:13:10PM +0100, Julia Lawall wrote:
> > 
> > 
> > On Mon, 24 Nov 2014, SF Markus Elfring wrote:
> > 
> > > >> This issue was detected by using the Coccinelle software.
> > > >
> > > > What script was used ?
> > >
> > > A semantic patch approach which I published on the mailing lists in March
> > > is in action on my software development system for a while.
> > >
> > >
> > > > Is it in scripts/coccinelle ?
> > >
> > > Not yet.
> > >
> > > I hope that the involved update suggestions got sufficient positive feedback
> > > to integrate five scripts there.
> > 
> > The current scripts are very complicated, involving the interaction
> > between multiple scripts and a database, and I think they are not very
> > suitable for make coccicheck.  Also, the idea of removing the null checks
> > is not appropriate in all contexts.  Perhaps it could be possible to add
> > a script to the Linux kernel that handles a number of common cases for
> > which removing the null test is widely considered to be desirable.
> > 
> > julia
> > 
> 
> Julia, do you have any particular objection to this specific patch? I didn't see
> a reason to prevent it going in.

Sorry if I was unclear.  If there is no problem with the current patch, I 
have no objection to it.  I don't think that the semantic patch that 
caused this patch to be generated is suitable for inclusion in the Linux 
kernel.

julia

Re: platform: x86: Deletion of checks before backlight_device_unregister()

[SF Markus Elfring]

> Julia, do you have any particular objection to this specific patch?
> I didn't see a reason to prevent it going in.

Thanks for your interest around this concrete update suggestion.

* Would you like to distinguish the consequences a bit more
  for results from the application of the semantic patch language?

* Where do you prefer to store special extensions for
  Coccinelle's script collection

Regards,
Markus

Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()

[Dan Carpenter]

On Fri, Jun 26, 2015 at 04:06:55PM -0700, Darren Hart wrote:
> Julia, do you have any particular objection to this specific patch? I didn't see
> a reason to prevent it going in.

I hate these patches...

We're saying "these functions have sanity checks so let's pass nonsense
values to them, it's fine."  It makes the code harder to understand.
There is no way for a human being to remember the complete list of
functions with sanity checks and which don't have sanity checks.

Markus has introduced quite a few bugs as well (people have so far
managed to catch his bugs before they were committed).  He so far has
resisted any suggestion that he should manually review his patches
before sending them.

Btw do have a scripts/coccinelle/free/ifnullfree.cocci which removes
checks before kfree, debugfs_remove, debugfs_remove_recursive, and
usb_free_urb.

regards,
dan carpenter

Re: platform: x86: Deletion of checks before backlight_device_unregister()

[SF Markus Elfring]

> There is no way for a human being to remember the complete list of
> functions with sanity checks and which don't have sanity checks.

I understand also this software development challenge.


> Markus has introduced quite a few bugs as well

I have only found other opinions about specific update suggestions.
Which of such "bugs" are real mistakes?
Are you looking for a better consensus?


> (people have so far managed to catch his bugs before they were committed).

Would you like to refer to any concrete software developers?


> He so far has resisted any suggestion that he should manually review
> his patches before sending them.

I am performing source code review to some degree.
My approach does partly not fit to your expectations.

Regards,
Markus