Remove unnecessary null pointer checks?

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 15:36:36 +0200

The variable "temp_storage" was eventually reassigned with a pointer. Thus omit the explicit initialisation at the beginning.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c +1 -1

index 024e22e..735aeb0 100644
@@ -60,7 +60,7 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr,
 		       void *input, void *output)
 {
 	int result = 0;
-	void *temp_storage = NULL;
+	void *temp_storage;
 
 	if (hwmgr == NULL || rt_table == NULL) {
 		printk(KERN_ERR "[ powerplay ] Invalid Parameter!\n");

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 16:23:21 +0200

Further update suggestions were taken into account after patches were applied from static source code analysis.

Markus Elfring (8):
Delete an unnecessary check before drm_gem_object_unreference_unlocked() Delete unnecessary checks before the function call "kfree"
One function call less in amdgpu_cgs_acpi_eval_object() after error detection Delete a variable in amdgpu_cgs_acpi_eval_object()
Delete an unnecessary variable initialisation in amdgpu_cgs_acpi_eval_object() Change assignment for a variable in amdgpu_cgs_acpi_eval_object() Change assignment for a buffer variable in phm_dispatch_table() Delete an unnecessary variable initialisation in phm_dispatch_table()

  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c            | 28 ++++++++++------------
  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c        |  4 +---
  .../gpu/drm/amd/powerplay/hwmgr/functiontables.c   | 12 ++++------
  3 files changed, 19 insertions(+), 25 deletions(-)

Am 16.07.2016 um 16:33 schrieb SF Markus Elfring:

Quoted 6 lines

From: Markus Elfring <[email protected]>
Date: Sat, 16 Jul 2016 16:23:21 +0200

Further update suggestions were taken into account after patches were applied from static source code analysis.

Small coding style nit pick on patch #7:

Review

Quoted 4 lines

(quoted diff) +2 -1

-       }
+       } else
+               temp_storage = NULL;

When an "if" has "{" and "}" the else should also use them even when it is only one line.

With that fixed the whole series is Reviewed-by: Christian König <[email protected]>, but as Walter Harms pointed out as well there are a couple of other things we could make more as well.

Regards,
Christian.

Quoted 14 lines

Markus Elfring (8):
Delete an unnecessary check before
drm_gem_object_unreference_unlocked()
Delete unnecessary checks before the function call "kfree" One function call less in amdgpu_cgs_acpi_eval_object() after error
detection
Delete a variable in amdgpu_cgs_acpi_eval_object()
Delete an unnecessary variable initialisation in
amdgpu_cgs_acpi_eval_object()
Change assignment for a variable in amdgpu_cgs_acpi_eval_object() Change assignment for a buffer variable in phm_dispatch_table() Delete an unnecessary variable initialisation in phm_dispatch_table()

Quoted 6 lines

  drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c            | 28
++++++++++------------
  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c        |  4 +---
  .../gpu/drm/amd/powerplay/hwmgr/functiontables.c   | 12 ++++------
  3 files changed, 19 insertions(+), 25 deletions(-)

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 11:15:28 +0200

The snd_info_free_entry() 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 <[email protected]>

---
 sound/pci/hda/patch_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/pci/hda/patch_hdmi.c +1 -1

index f852734..2f24338 100644
@@ -591,7 +591,7 @@ static int eld_proc_new(struct hdmi_spec_per_pin *per_pin, int index)
 
 static void eld_proc_free(struct hdmi_spec_per_pin *per_pin)
 {
-	if (!per_pin->codec->bus->shutdown && per_pin->proc_entry) {
+	if (!per_pin->codec->bus->shutdown) {
 		snd_info_free_entry(per_pin->proc_entry);
 		per_pin->proc_entry = NULL;
 	}

Signature · 1 line

2.4.4

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 14:59:04 +0200

The release_firmware() 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 <[email protected]>

---
 drivers/usb/serial/mxuport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/serial/mxuport.c +0 -0

index 460a406..92f7aee 100644

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 16:42:04 +0200

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/pci/host/pcie-iproc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/pci/host/pcie-iproc.c +0 -1

index d77481e..f875821 100644
@@ -239,12 +239,9 @@ err_rm_root_bus:
 	pci_remove_root_bus(bus);
 
 err_power_off_phy:
-	if (pcie->phy)

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 16:42:04 +0200

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/pci/host/pcie-iproc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/pci/host/pcie-iproc.c +2 -5

index d77481e..f875821 100644
@@ -239,12 +239,9 @@ err_rm_root_bus:
 	pci_remove_root_bus(bus);
 
 err_power_off_phy:
-	if (pcie->phy)
-		phy_power_off(pcie->phy);
+	phy_power_off(pcie->phy);
 err_exit_phy:
-	if (pcie->phy)
-		phy_exit(pcie->phy);
-
+	phy_exit(pcie->phy);
 	return ret;
 }
 EXPORT_SYMBOL(iproc_pcie_setup);

Signature · 1 line

2.4.4

Quoted 9 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 16:42:04 +0200

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Quoted 1 line

Signed-off-by Markus Elfring <[email protected]>

I haven't seen a followup to Ray's review, but in the interest of making progress, I updated and applied the patch as appended. I also reviewed other phy_*() calls under drivers/pci, and they all look OK (with no unnecessary tests for NULL).

This is on the pci/host-iproc branch for v4.3.

Quoted 25 lines

---
 drivers/pci/host/pcie-iproc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/pci/host/pcie-iproc.c +2 -5

index d77481e..f875821 100644
@@ -239,12 +239,9 @@ err_rm_root_bus:
 	pci_remove_root_bus(bus);
 
 err_power_off_phy:
-	if (pcie->phy)
-		phy_power_off(pcie->phy);
+	phy_power_off(pcie->phy);
 err_exit_phy:
-	if (pcie->phy)
-		phy_exit(pcie->phy);
-
+	phy_exit(pcie->phy);
 	return ret;
 }
 EXPORT_SYMBOL(iproc_pcie_setup);

Signature · 1 line

2.4.4

commit 55b5e16332eb9ffc1cbaf975585f4521417ab427

Author: Markus Elfring <[email protected]>
Date:   Sun Jun 28 16:42:04 2015 +0200

PCI: iproc: Delete unnecessary checks before phy calls

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

[bhelgaas: also phy_init() and phy_power_on(), as Ray Jui suggested]

[bhelgaas: also remove tests in iproc_pcie_remove()]

Signed-off-by Markus Elfring <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

drivers/pci/host/pcie-iproc.c +13 -21

index d77481e..9a00dca 100644
@@ -191,19 +191,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	if (!pcie || !pcie->dev || !pcie->base)
 		return -EINVAL;
 
-	if (pcie->phy) {
-		ret = phy_init(pcie->phy);
-		if (ret) {
-			dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
-			return ret;
-		}
-
-		ret = phy_power_on(pcie->phy);
-		if (ret) {
-			dev_err(pcie->dev, "unable to power on PCIe PHY\n");
-			goto err_exit_phy;
-		}
+	ret = phy_init(pcie->phy);
+	if (ret) {
+		dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
+		return ret;
+	}
 
+	ret = phy_power_on(pcie->phy);
+	if (ret) {
+		dev_err(pcie->dev, "unable to power on PCIe PHY\n");
+		goto err_exit_phy;
 	}
 
 	iproc_pcie_reset(pcie);

@@ -239,12 +236,9 @@ err_rm_root_bus:
 	pci_remove_root_bus(bus);
 
 err_power_off_phy:
-	if (pcie->phy)
-		phy_power_off(pcie->phy);
+	phy_power_off(pcie->phy);
 err_exit_phy:
-	if (pcie->phy)
-		phy_exit(pcie->phy);
-
+	phy_exit(pcie->phy);
 	return ret;
 }
 EXPORT_SYMBOL(iproc_pcie_setup);

@@ -254,10 +248,8 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
 	pci_stop_root_bus(pcie->root_bus);
 	pci_remove_root_bus(pcie->root_bus);
 
-	if (pcie->phy) {
-		phy_power_off(pcie->phy);
-		phy_exit(pcie->phy);
-	}
+	phy_power_off(pcie->phy);
+	phy_exit(pcie->phy);
 
 	return 0;
 }

Hi Bjorn,

Quoted 10 lines · Bjorn Helgaas

Quoted 9 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 16:42:04 +0200

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Quoted 9 lines

Quoted 1 line

Signed-off-by Markus Elfring <[email protected]>

I haven't seen a followup to Ray's review, but in the interest of making progress, I updated and applied the patch as appended. I also reviewed other phy_*() calls under drivers/pci, and they all look OK (with no unnecessary tests for NULL).

This is on the pci/host-iproc branch for v4.3.

Hmmm....I searched my mailbox but cannot find an email with this patch (while I remember I reviewed and commented on the initial version of this patch). It must have gone into some sub-folder or deleted by me by accident. My bad.

Nevertheless,the current patch looks good to me!

Quoted 79 lines

commit 55b5e16332eb9ffc1cbaf975585f4521417ab427

Author: Markus Elfring <[email protected]>
Date:   Sun Jun 28 16:42:04 2015 +0200

PCI: iproc: Delete unnecessary checks before phy calls

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

[bhelgaas: also phy_init() and phy_power_on(), as Ray Jui suggested]

[bhelgaas: also remove tests in iproc_pcie_remove()]

Signed-off-by Markus Elfring <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

drivers/pci/host/pcie-iproc.c +13 -21

index d77481e..9a00dca 100644
@@ -191,19 +191,16 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	if (!pcie || !pcie->dev || !pcie->base)
 		return -EINVAL;
 
-	if (pcie->phy) {
-		ret = phy_init(pcie->phy);
-		if (ret) {
-			dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
-			return ret;
-		}
-
-		ret = phy_power_on(pcie->phy);
-		if (ret) {
-			dev_err(pcie->dev, "unable to power on PCIe PHY\n");
-			goto err_exit_phy;
-		}
+	ret = phy_init(pcie->phy);
+	if (ret) {
+		dev_err(pcie->dev, "unable to initialize PCIe PHY\n");
+		return ret;
+	}
 
+	ret = phy_power_on(pcie->phy);
+	if (ret) {
+		dev_err(pcie->dev, "unable to power on PCIe PHY\n");
+		goto err_exit_phy;
 	}
 
 	iproc_pcie_reset(pcie);

@@ -239,12 +236,9 @@ err_rm_root_bus:
 	pci_remove_root_bus(bus);
 
 err_power_off_phy:
-	if (pcie->phy)
-		phy_power_off(pcie->phy);
+	phy_power_off(pcie->phy);
 err_exit_phy:
-	if (pcie->phy)
-		phy_exit(pcie->phy);
-
+	phy_exit(pcie->phy);
 	return ret;
 }
 EXPORT_SYMBOL(iproc_pcie_setup);

@@ -254,10 +248,8 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
 	pci_stop_root_bus(pcie->root_bus);
 	pci_remove_root_bus(pcie->root_bus);
 
-	if (pcie->phy) {
-		phy_power_off(pcie->phy);
-		phy_exit(pcie->phy);
-	}
+	phy_power_off(pcie->phy);
+	phy_exit(pcie->phy);
 
 	return 0;
 }

Quoted 23 lines · Ray Jui

Hi Bjorn,

Quoted 19 lines · Bjorn Helgaas

Quoted 10 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 16:42:04 +0200

The functions phy_exit() and phy_power_off() test whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

I haven't seen a followup to Ray's review, but in the interest of making progress, I updated and applied the patch as appended. I also reviewed other phy_*() calls under drivers/pci, and they all look OK (with no unnecessary tests for NULL).

This is on the pci/host-iproc branch for v4.3.

Quoted 6 lines

Hmmm....I searched my mailbox but cannot find an email with this patch (while I remember I reviewed and commented on the initial version of this patch). It must have gone into some sub-folder or deleted by me by accident. My bad.

Nevertheless,the current patch looks good to me!

Thanks for checking it out! Can I add your Reviewed-by to the patch below?

From: Markus Elfring <[email protected]>
Date: Sun, 28 Jun 2015 17:58:42 +0200

The kfree() function was called even if a previous memory allocation try failed.

This implementation detail could be improved by the introduction of another jump label.

Signed-off-by Markus Elfring <[email protected]>

---
  drivers/vfio/vfio_iommu_spapr_tce.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/vfio/vfio_iommu_spapr_tce.c +2 -2

index 50ddfac..2523075 100644
@@ -1200,7 +1200,7 @@ static int tce_iommu_attach_group(void *iommu_data,
  	tcegrp = kzalloc(sizeof(*tcegrp), GFP_KERNEL);
  	if (!tcegrp) {
  		ret = -ENOMEM;
-		goto unlock_exit;
+		goto unlock_container;
  	}
 
  	if (!table_group->ops || !table_group->ops->take_ownership ||

@@ -1217,7 +1217,7 @@ static int tce_iommu_attach_group(void *iommu_data,
  unlock_exit:
  	if (ret)
  		kfree(tcegrp);
-
+unlock_container:
  	mutex_unlock(&container->lock);
 
  	return ret;

tcegrp will be NULL and kfree() can handle this just fine

(is not it the whole point of this patchset
- remove the check and just call kfree() even if the pointer is NULL?).

And if you wanted another label,

than the existing one should have been renamed to "free_exit" or "free_unlock_exit" and new one would be "unlock_exit".

Quoted 1 line

tcegrp will be NULL and kfree() can handle this just fine

The affected function did not show this API knowledge, did it?

Quoted 4 lines

(is not it the whole point of this patchset
- remove the check and just call kfree() even if the pointer is NULL?).

Partly, yes.

Quoted 1 line

And if you wanted another label,

I suggest this to improve corresponding exception handling.

Quoted 2 lines

than the existing one should have been renamed to "free_exit" or "free_unlock_exit" and new one would be "unlock_exit".

I chose a smaller change at this place.

I am not familiar enough with other called functions there at the moment. Are the remaining goto statements also update candidates?

Quoted 4 lines

Quoted 2 lines

than the existing one should have been renamed to "free_exit" or "free_unlock_exit" and new one would be "unlock_exit".

I chose a smaller change at this place.

I'd just drop this patch.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

than the existing one should have been renamed to "free_exit" or "free_unlock_exit" and new one would be "unlock_exit".

I chose a smaller change at this place.

I'd just drop this patch.

How do you think about to improve the affected jump labels a bit more there?

From: Markus Elfring <[email protected]>
Date: Mon, 29 Jun 2015 12:22:24 +0200

The vfree() 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 <[email protected]>

From: Markus Elfring <[email protected]>
Date: Thu, 2 Jul 2015 16:30:24 +0200

The free_percpu() 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 <[email protected]>

From: Markus Elfring <[email protected]>
Date: Thu, 2 Jul 2015 17:58:21 +0200

The module_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 <[email protected]>

From: Markus Elfring <[email protected]>
Date: Thu, 2 Jul 2015 18:38:12 +0200

The module_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 <[email protected]>

From: Markus Elfring <[email protected]>
Date: Sun, 5 Jul 2015 21:55:10 +0200

The drm_property_unreference_blob() 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 <[email protected]>

---
 drivers/gpu/drm/drm_crtc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/gpu/drm/drm_crtc.c +1 -3

index 2d57fc5..6e4c8b0 100644
@@ -4469,9 +4469,7 @@ static int drm_property_replace_global_blob(struct
 drm_device *dev,
                        goto err_created;
        }
 
-       if (old_blob)
-               drm_property_unreference_blob(old_blob);
-
+       drm_property_unreference_blob(old_blob);
        *replace = new_blob;
 

        return 0;

--
2.4.5

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 7 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Sun, 5 Jul 2015 21:55:10 +0200

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

Quoted 4 lines

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

Reviewed-by Zhao Junwang <[email protected]>

From: Markus Elfring <[email protected]>
Date: Fri, 6 Nov 2015 12:03:46 +0100

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

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c        | 9 +++------
 drivers/gpu/drm/drm_atomic_helper.c | 5 ++---
 2 files changed, 5 insertions(+), 9 deletions(-)

drivers/gpu/drm/drm_atomic.c +3 -6

index 7bb3845..d65dc31 100644
@@ -316,8 +316,7 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 	if (mode && memcmp(&state->mode, mode, sizeof(*mode)) = 0)
 		return 0;
 
-	if (state->mode_blob)
-		drm_property_unreference_blob(state->mode_blob);
+	drm_property_unreference_blob(state->mode_blob);
 	state->mode_blob = NULL;
 
 	if (mode) {

@@ -363,8 +362,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 	if (blob = state->mode_blob)
 		return 0;
 
-	if (state->mode_blob)
-		drm_property_unreference_blob(state->mode_blob);
+	drm_property_unreference_blob(state->mode_blob);
 	state->mode_blob = NULL;
 
 	if (blob) {

@@ -419,8 +417,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_property_blob *mode >  			drm_property_lookup_blob(dev, val);
 		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
-		if (mode)
-			drm_property_unreference_blob(mode);
+		drm_property_unreference_blob(mode);
 		return ret;
 	}
 	else if (crtc->funcs->atomic_set_property)

drivers/gpu/drm/drm_atomic_helper.c +2 -3

index 0c6f621..9870c70 100644
@@ -2173,7 +2173,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
  */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
 {
-	if (crtc->state && crtc->state->mode_blob)
+	if (crtc->state)
 		drm_property_unreference_blob(crtc->state->mode_blob);
 	kfree(crtc->state);
 	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);

@@ -2241,8 +2241,7 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
 void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 					    struct drm_crtc_state *state)
 {
-	if (state->mode_blob)
-		drm_property_unreference_blob(state->mode_blob);
+	drm_property_unreference_blob(state->mode_blob);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
 

Signature · 1 line

2.6.2

From: Markus Elfring <[email protected]>
Date: Wed, 20 Jul 2016 17:54:32 +0200

The drm_property_unreference_blob() 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 <[email protected]>

---
 drivers/gpu/drm/drm_atomic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/gpu/drm/drm_atomic.c +0 -0

index 3cee084..8d2f111 100644

drivers/gpu/drm/drm_atomic.c +0 -1

        *blob = new_blob;
--

2.9.2

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Markus Elfring <[email protected]>
Date: Sun, 5 Jul 2015 22:45:23 +0200

The drm_fbdev_cma_restore_mode() 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 <[email protected]>

---
 drivers/gpu/drm/imx/imx-drm-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/gpu/drm/imx/imx-drm-core.c +0 -0

index 74f505b..9172c0e 100644

From: Markus Elfring <[email protected]>
Date: Wed, 8 Jul 2015 21:26:02 +0200

The kobject_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 <[email protected]>

---
 sound/hda/hdac_sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

sound/hda/hdac_sysfs.c +1 -2

index 0a6ce3b..089b35f 100644
@@ -321,8 +321,7 @@ static void widget_tree_free(struct hdac_device *codec)
 			free_widget_node(*p, &widget_node_group);
 		kfree(tree->nodes);
 	}
-	if (tree->root)
-		kobject_put(tree->root);
+	kobject_put(tree->root);
 	kfree(tree);
 	codec->widgets = NULL;
 }

Signature · 1 line

2.4.5

From: Markus Elfring <[email protected]>
Date: Wed, 8 Jul 2015 22:12:25 +0200

The gpiod_put() function performs also input parameter validation by forwarding its single input pointer to the gpiod_free() function. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by Markus Elfring <[email protected]>

Quoted 9 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 8 Jul 2015 22:12:25 +0200

The gpiod_put() function performs also input parameter validation by forwarding its single input pointer to the gpiod_free() function. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Quoted 1 line

Signed-off-by Markus Elfring <[email protected]>

As Dan correctly pointed out, this is not as straightforward as it might seem on a firsr sight, because there is a WARN_ON() that might start triggering in case of !ihid->desc.

Adding Benjamin. I am not applying this without his Ack.

Quoted 6 lines

Quoted 6 lines

The gpiod_put() function performs also input parameter validation by forwarding its single input pointer to the gpiod_free() function. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Quoted 8 lines

Quoted 1 line

Signed-off-by Markus Elfring <[email protected]>

As Dan correctly pointed out, this is not as straightforward as it might seem on a firsr sight, because there is a WARN_ON() that might start triggering in case of !ihid->desc.

Adding Benjamin. I am not applying this without his Ack.

I think the gpiod case is the exception rather than the common rule (most i2c-hid device we saw until recently were using irqs, not gpios). So if I understand correctly, removing the check on ihid->desc would raise a warning for most devices. This is IMO not a good thing, so I would say NACK.

Mika might have a different opinion though.

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

The gpiod_put() function performs also input parameter validation by forwarding its single input pointer to the gpiod_free() function. Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

Quoted 8 lines

Quoted 1 line

Signed-off-by Markus Elfring <[email protected]>

As Dan correctly pointed out, this is not as straightforward as it might seem on a firsr sight, because there is a WARN_ON() that might start triggering in case of !ihid->desc.

Adding Benjamin. I am not applying this without his Ack.

Quoted 7 lines

I think the gpiod case is the exception rather than the common rule (most i2c-hid device we saw until recently were using irqs, not gpios). So if I understand correctly, removing the check on ihid->desc would raise a warning for most devices. This is IMO not a good thing, so I would say NACK.

Mika might have a different opinion though.

The proposed update candidates are contained in the source file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708.

* i2c_hid_remove() function:
Can it be tolerated here that the pointer "ihid->desc" might be eventually null?

* i2c_hid_probe() function:
Is this implementation structured in such a way that a pointer for valid data will be usually passed for "ihid->desc" if the statements after the jump label "err" will be reached?

Quoted 11 lines

The proposed update candidates are contained in the source file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708.

* i2c_hid_remove() function:
Can it be tolerated here that the pointer "ihid->desc" might be eventually null?

* i2c_hid_probe() function:
Is this implementation structured in such a way that a pointer for valid data will be usually passed for "ihid->desc" if the statements after the jump label "err" will be reached?

Again, in both case it is completely normal to have "ihid->desc = NULL" given that this field is only retrieved in case of an ACPI device which does not declares an IRQ but a GPIO. Most ACPI devices I saw are using a simple IRQ, and the OF instantiations of the driver will definitively have ihid->desc null. So I do not want to have a warning for most of i2c-hid devices out there (because I will have to explain that this is completely normal again and again).

Quoted 12 lines

Quoted 11 lines

The proposed update candidates are contained in the source file "drivers/hid/i2c-hid/i2c-hid.c" from Linux next-20150708.

* i2c_hid_remove() function:
Can it be tolerated here that the pointer "ihid->desc" might be eventually null?

* i2c_hid_probe() function:
Is this implementation structured in such a way that a pointer for valid data will be usually passed for "ihid->desc" if the statements after the jump label "err" will be reached?

Quoted 7 lines

Again, in both case it is completely normal to have "ihid->desc = NULL" given that this field is only retrieved in case of an ACPI device which does not declares an IRQ but a GPIO. Most ACPI devices I saw are using a simple IRQ, and the OF instantiations of the driver will definitively have ihid->desc null. So I do not want to have a warning for most of i2c-hid devices out there (because I will have to explain that this is completely normal again and again).

Would it make sense to annotate checks before such function calls as "unlikely" then?

From: Markus Elfring <[email protected]>
Date: Thu, 9 Jul 2015 09:52:24 +0200

The unregister_sysctl_table() 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 <[email protected]>

---
 drivers/cdrom/cdrom.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/cdrom/cdrom.c +0 -0

index 5d28a45..e43e84d 100644

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 18:18:37 +0100

The irlmp_unregister_service() 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 <[email protected]>

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 19:20:34 +0100

The batadv_softif_vlan_free_ref() 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 <[email protected]>

---
 net/batman-adv/translation-table.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 19:20:34 +0100

The batadv_softif_vlan_free_ref() 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 <[email protected]>

---
 net/batman-adv/translation-table.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 20:41:02 +0100

Let us split a check for a condition at the beginning of the batadv_is_ap_isolated() function so that a direct return can be performed in this function if the variable "vlan" contained a null pointer.

Signed-off-by Markus Elfring <[email protected]>

---
 net/batman-adv/translation-table.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 20:41:02 +0100

Let us split a check for a condition at the beginning of the batadv_is_ap_isolated() function so that a direct return can be performed in this function if the variable "vlan" contained a null pointer.

Signed-off-by Markus Elfring <[email protected]>

---
 net/batman-adv/translation-table.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 21:10:51 +0100

The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
Let us avoid this double check by reordering a function call sequence and the better selection of jump targets.

Signed-off-by Markus Elfring <[email protected]>

---
 net/batman-adv/translation-table.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

net/batman-adv/translation-table.c +5 -8

index 965a004..3ac32d9 100644
@@ -3323,27 +3323,24 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst,
 		return false;
 
 	if (!atomic_read(&vlan->ap_isolation))
-		goto out;
+		goto vlan_free;
 
 	tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid);
 	if (!tt_local_entry)
-		goto out;
+		goto vlan_free;
 
 	tt_global_entry = batadv_tt_global_hash_find(bat_priv, src, vid);
 	if (!tt_global_entry)
-		goto out;
+		goto local_entry_free;
 
-	if (!_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
-		goto out;
-
-	ret = true;
+	if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
+		ret = true;
 
-out:

Quoted 6 lines

net/batman-adv/translation-table.c +4 -1

-out:
+	batadv_tt_global_entry_free_ref(tt_global_entry);
+local_entry_free:
+	batadv_tt_local_entry_free_ref(tt_local_entry);
+vlan_free:
 	batadv_softif_vlan_free_ref(vlan);

net/batman-adv/translation-table.c bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst, · line 3344

Quoted 5 lines

net/batman-adv/translation-table.c +0 -4

-	if (tt_global_entry)
-		batadv_tt_global_entry_free_ref(tt_global_entry);
-	if (tt_local_entry)
-		batadv_tt_local_entry_free_ref(tt_local_entry);
 	return ret;

if you really want to make this codestyle change, I'd suggest you to go through the whole batman-adv code and apply the same change where needed.

It does not make sense to change the codestyle in one spot only.

On top of that, by going through the batman-adv code you might agree that the current style is actually not a bad idea.

Quoted 2 lines

On top of that, by going through the batman-adv code you might agree that the current style is actually not a bad idea.

I got the impression that the current Linux coding style convention disagrees around the affected jump label selection to some degree, doesn't it?

From: Markus Elfring <[email protected]>
Date: Fri, 11 Mar 2016 13:10:20 +0100

The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.

* Avoid this double check by reordering a function call sequence and the better selection of jump targets.

* Omit the initialisation for these variables at the beginning then.

Signed-off-by Markus Elfring <[email protected]>

From: SF Markus Elfring <[email protected]>
Date: Fri, 11 Mar 2016 13:40:56 +0100

Quoted 6 lines

From: Markus Elfring <[email protected]>
Date: Fri, 11 Mar 2016 13:10:20 +0100

The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.

Quoted 6 lines

* Avoid this double check by reordering a function call sequence and the better selection of jump targets.

* Omit the initialisation for these variables at the beginning then.

Signed-off-by Markus Elfring <[email protected]>

I am assuming Antonio will take this in via his tree.

From: Markus Elfring <[email protected]>
Date: Fri, 11 Mar 2016 13:10:20 +0100

The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.

* Avoid this double check by reordering a function call sequence and the better selection of jump targets.

* Omit the initialisation for these variables at the beginning then.

Signed-off-by Markus Elfring <[email protected]>

---

From: Markus Elfring <[email protected]>
Date: Fri, 11 Mar 2016 13:10:20 +0100

The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.

* Avoid this double check by reordering a function call sequence and the better selection of jump targets.

* Omit the initialisation for these variables at the beginning then.

Signed-off-by Markus Elfring <[email protected]>

Reviewed-by Sven Eckelmann <[email protected]>

---
 net/batman-adv/translation-table.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

From: Markus Elfring <[email protected]>
Date: Tue, 3 Nov 2015 21:10:51 +0100

The variables "tt_local_entry" and "tt_global_entry" were eventually checked again despite of a corresponding null pointer test before.
Let us avoid this double check by reordering a function call sequence and the better selection of jump targets.

Signed-off-by Markus Elfring <[email protected]>

---
 net/batman-adv/translation-table.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

net/batman-adv/translation-table.c +5 -8

index 965a004..3ac32d9 100644
@@ -3323,27 +3323,24 @@ bool batadv_is_ap_isolated(struct batadv_priv *bat_priv, u8 *src, u8 *dst,
 		return false;
 
 	if (!atomic_read(&vlan->ap_isolation))
-		goto out;
+		goto vlan_free;
 
 	tt_local_entry = batadv_tt_local_hash_find(bat_priv, dst, vid);
 	if (!tt_local_entry)
-		goto out;
+		goto vlan_free;
 
 	tt_global_entry = batadv_tt_global_hash_find(bat_priv, src, vid);
 	if (!tt_global_entry)
-		goto out;
+		goto local_entry_free;
 
-	if (!_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
-		goto out;
-
-	ret = true;
+	if (_batadv_is_ap_isolated(tt_local_entry, tt_global_entry))
+		ret = true;
 
-out:

From: Markus Elfring <[email protected]>
Date: Wed, 4 Nov 2015 20:00:42 +0100

The iput() 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 <[email protected]>

---
 fs/ubifs/recovery.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/ubifs/recovery.c +1 -2

index 2a1f0bc..586d593 100644
@@ -1331,8 +1331,7 @@ void ubifs_destroy_size_tree(struct ubifs_info *c)
        struct size_entry *e, *n;
 
        rbtree_postorder_for_each_entry_safe(e, n, &c->size_tree, rb) {
-               if (e->inode)
-                       iput(e->inode);
+               iput(e->inode);
                kfree(e);
        }
 

From: Markus Elfring <[email protected]>
Date: Thu, 5 Nov 2015 12:48:58 +0100

The functions "kfree" and "kobject_put" were called in a few cases by the function "class_register_type" during error handling even if the passed variable contained a null pointer.

This implementation detail could be improved by the adjustment of jump targets.

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Quoted 10 lines

From: Markus Elfring <[email protected]>
Date: Thu, 5 Nov 2015 12:48:58 +0100

The functions "kfree" and "kobject_put" were called in a few cases by the function "class_register_type" during error handling even if the passed variable contained a null pointer.

This implementation detail could be improved by the adjustment of jump targets.

Quoted 4 lines

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Does not apply to my staging-next branch :(

Am 25.11.2015 um 17:39 schrieb Greg Kroah-Hartman:

Quoted 11 lines · SF Markus Elfring

Quoted 10 lines

From: Markus Elfring <[email protected]>
Date: Thu, 5 Nov 2015 12:48:58 +0100

The functions "kfree" and "kobject_put" were called in a few cases by the function "class_register_type" during error handling even if the passed variable contained a null pointer.

This implementation detail could be improved by the adjustment of jump targets.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Markus Elfring <[email protected]>

---
 drivers/staging/lustre/lustre/obdclass/genops.c | 26 +++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Does not apply to my staging-next branch :(

I get also a result like the following together with the software "Linux next-20151126". ;-)

elfring@Sonne:~/Projekte/Linux/next-patched> LANG=C git apply ~/Projekte/Bau/Linux/scripts/Coccinelle/deletions1/next/20151102/Flicken/0003-staging-lustre-Less-function-calls-in-class_register.patch
error: patch failed: drivers/staging/lustre/lustre/obdclass/genops.c:214
error: drivers/staging/lustre/lustre/obdclass/genops.c: patch does not apply

Do you try this update suggestion out without integrating the corresponding previous update suggestion "Delete unnecessary checks before two function calls" where I proposed to remove extra checks before a few calls of the function "kobject_put" (which seems to matter for the patch hunk in the shown error message)?
https://lkml.org/lkml/2015/11/5/276
https://www.mail-archive.com/[email protected]/msg1013635.html

Would you like to reject the first update step from this patch series so that I need to adapt my approach to your software design decision?

Quoted 6 lines

Do you try this update suggestion out without integrating the corresponding previous update suggestion "Delete unnecessary checks before two function calls" where I proposed to remove extra checks before a few calls of the function "kobject_put" (which seems to matter for the patch hunk in the shown error message)?
https://lkml.org/lkml/2015/11/5/276
https://www.mail-archive.com/[email protected]/msg1013635.html

I guess so, I don't remember, I don't see any patches from you earlier in my "todo" mbox.

Quoted 2 lines

Would you like to reject the first update step from this patch series so that I need to adapt my approach to your software design decision?

I have no idea what you are talking about. I have no recolection of previous patches or conversations about your patches.

Please stop sending these patches for drivers/staging.

You are welcome to send them for other subsystems

which I don't care about.

-----Original Message-----
From: SF Markus Elfring [mailto:elfring(a)users.sourceforge.net]
Sent: Thursday, November 05, 2015 7:23 AM
To: Len Brown; Zheng, Lv; Wysocki, Rafael J; Moore, Robert; linux- acpi(a)vger.kernel.org; devel(a)acpica.org
Cc: LKML; kernel-janitors(a)vger.kernel.org; Julia Lawall
Subject: [PATCH] ACPICA: Delete unnecessary checks before the function call "acpi_ut_strupr"

From: Markus Elfring <elfring(a)users.sourceforge.net>
Date: Thu, 5 Nov 2015 16:12:32 +0100

The acpi_ut_strupr() 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(a)users.sourceforge.net>

---
 drivers/acpi/acpica/dbcmds.c  | 9 ++-------
drivers/acpi/acpica/dbinput.c | 5 +----
 2 files changed, 3 insertions(+), 11 deletions(-)

drivers/acpi/acpica/dbcmds.c +2 -7

index 30414b3..7dcc95a 100644
@@ -1131,13 +1131,8 @@ void acpi_db_trace(char *enable_arg, char
 *method_arg, char *once_arg)
 	u32 debug_layer = 0;
 	u32 flags = 0;
 
-	if (enable_arg) {
-		acpi_ut_strupr(enable_arg);
-	}
-
-	if (once_arg) {
-		acpi_ut_strupr(once_arg);
-	}
+	acpi_ut_strupr(enable_arg);
+	acpi_ut_strupr(once_arg);
 
 	if (method_arg) {

drivers/acpi/acpica/dbcmds.c +0 -0

 		if (acpi_db_trace_method_name) {

drivers/acpi/acpica/dbinput.c +1 -5

index 0480254..aa1dcee 100644
@@ -622,10 +622,7 @@ static u32 acpi_db_get_line(char *input_buffer)
 	}
 
 	/* Uppercase the actual command */
-
-	if (acpi_gbl_db_args[0]) {
-		acpi_ut_strupr(acpi_gbl_db_args[0]);
-	}
+	acpi_ut_strupr(acpi_gbl_db_args[0]);
 
 	count = i;
 	if (count) {
--

2.6.2

From: Markus Elfring <[email protected]>
Date: Thu, 5 Nov 2015 17:10:22 +0100

The pinctrl_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 <[email protected]>

---
  drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c | 3 +--
  drivers/pinctrl/pinctrl-single.c          | 5 +----
  2 files changed, 2 insertions(+), 6 deletions(-)

drivers/pinctrl/bcm/pinctrl-cygnus-gpio.c +1 -2

index 12a48f4..663ad0a 100644
@@ -638,8 +638,7 @@ static int cygnus_gpio_register_pinconf(struct cygnus_gpio *chip)
 
  static void cygnus_gpio_unregister_pinconf(struct cygnus_gpio *chip)
  {
-	if (chip->pctl)
-		pinctrl_unregister(chip->pctl);
+	pinctrl_unregister(chip->pctl);
  }
 
  struct cygnus_gpio_data {

drivers/pinctrl/pinctrl-single.c +0 -0

index ef04b96..d24e5f1 100644
@@ -1484,10 +1484,7 @@ static void pcs_irq_free(struct pcs_device *pcs)
  static void pcs_free_resources(struct pcs_device *pcs)

From: Markus Elfring <[email protected]>
Date: Thu, 5 Nov 2015 17:10:22 +0100

The pinctrl_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 <[email protected]>