[PATCH] mmc-core: One check less in mmc_select_hs200() after error detection

[PATCH] mmc-core: One check less in mmc_select_hs200() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 29 Dec 2015 20:28:46 +0100

This issue was detected by using the Coccinelle software.

Move the jump label directly before the desired log statement
so that the variable "err" will not be checked once more
after it was determined that a call of the function
"__mmc_set_signal_voltage" or "__mmc_switch" failed.
Use the identifier "report_failure" instead of the label "err".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mmc/core/mmc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 549c56e..866f72b 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1256,7 +1256,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 
 	/* If fails try again during next card power cycle */
 	if (err)
-		goto err;
+		goto report_failure;
 
 	mmc_select_driver_type(card);
 
@@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 				   card->ext_csd.generic_cmd6_time,
 				   true, send_status, true);
 		if (err)
-			goto err;
+			goto report_failure;
 		old_timing = host->ios.timing;
 		mmc_set_timing(host, MMC_TIMING_MMC_HS200);
 		if (!send_status) {
@@ -1289,10 +1289,11 @@ static int mmc_select_hs200(struct mmc_card *card)
 				mmc_set_timing(host, old_timing);
 		}
 	}
-err:
-	if (err)
+	if (err) {
+report_failure:
 		pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
 		       __func__, err);
+	}
 	return err;
 }
 
-- 
2.6.3

[PATCH 0/2] mmc-host: Fine-tuning for one function

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 29 Dec 2015 21:54:14 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete unnecessary variable initialisations in sdricoh_init_mmc()
  Less checks in sdricoh_init_mmc() after error detection

 drivers/mmc/host/sdricoh_cs.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

-- 
2.6.3

[PATCH 1/2] mmc-sdricoh_cs: Delete unnecessary variable initialisations in sdricoh_init_mmc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 29 Dec 2015 21:11:45 +0100

These variables will eventually be set to an appropriate value a bit later.
* host
* iobase
* result

Thus let us omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mmc/host/sdricoh_cs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index b7e3057..5e57d9f 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -398,10 +398,10 @@ static struct mmc_host_ops sdricoh_ops = {
 static int sdricoh_init_mmc(struct pci_dev *pci_dev,
 			    struct pcmcia_device *pcmcia_dev)
 {
-	int result = 0;
-	void __iomem *iobase = NULL;
+	int result;
+	void __iomem *iobase;
 	struct mmc_host *mmc = NULL;
-	struct sdricoh_host *host = NULL;
+	struct sdricoh_host *host;
 	struct device *dev = &pcmcia_dev->dev;
 	/* map iomem */
 	if (pci_resource_len(pci_dev, SDRICOH_PCI_REGION) !=
-- 
2.6.3

[PATCH 2/2] mmc-sdricoh_cs: Less checks in sdricoh_init_mmc() after, error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 29 Dec 2015 21:45:34 +0100

This issue was detected by using the Coccinelle software.

Two pointer checks could be repeated by the sdricoh_init_mmc() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* This implementation detail could be improved by adjustments
  for jump targets according to the Linux coding style convention.

* Drop an unnecessary initialisation for the variable "mmc" then.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mmc/host/sdricoh_cs.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdricoh_cs.c b/drivers/mmc/host/sdricoh_cs.c
index 5e57d9f..5ff26ab 100644
--- a/drivers/mmc/host/sdricoh_cs.c
+++ b/drivers/mmc/host/sdricoh_cs.c
@@ -400,7 +400,7 @@ static int sdricoh_init_mmc(struct pci_dev *pci_dev,
 {
 	int result;
 	void __iomem *iobase;
-	struct mmc_host *mmc = NULL;
+	struct mmc_host *mmc;
 	struct sdricoh_host *host;
 	struct device *dev = &pcmcia_dev->dev;
 	/* map iomem */
@@ -419,7 +419,7 @@ static int sdricoh_init_mmc(struct pci_dev *pci_dev,
 	if (readl(iobase + R104_VERSION) != 0x4000) {
 		dev_dbg(dev, "no supported mmc controller found\n");
 		result = -ENODEV;
-		goto err;
+		goto unmap_io;
 	}
 	/* allocate privdata */
 	mmc = pcmcia_dev->priv =
@@ -427,7 +427,7 @@ static int sdricoh_init_mmc(struct pci_dev *pci_dev,
 	if (!mmc) {
 		dev_err(dev, "mmc_alloc_host failed\n");
 		result = -ENOMEM;
-		goto err;
+		goto unmap_io;
 	}
 	host = mmc_priv(mmc);
 
@@ -451,8 +451,7 @@ static int sdricoh_init_mmc(struct pci_dev *pci_dev,
 	if (sdricoh_reset(host)) {
 		dev_dbg(dev, "could not reset\n");
 		result = -EIO;
-		goto err;
-
+		goto free_host;
 	}
 
 	result = mmc_add_host(mmc);
@@ -461,13 +460,10 @@ static int sdricoh_init_mmc(struct pci_dev *pci_dev,
 		dev_dbg(dev, "mmc host registered\n");
 		return 0;
 	}
-
-err:
-	if (iobase)
-		pci_iounmap(pci_dev, iobase);
-	if (mmc)
-		mmc_free_host(mmc);
-
+free_host:
+	mmc_free_host(mmc);
+unmap_io:
+	pci_iounmap(pci_dev, iobase);
 	return result;
 }
 
-- 
2.6.3

Re: [PATCH] mmc-core: One check less in mmc_select_hs200() after error detection

[Ulf Hansson]

On 29 December 2015 at 20:50, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 29 Dec 2015 20:28:46 +0100
>
> This issue was detected by using the Coccinelle software.
>
> Move the jump label directly before the desired log statement
> so that the variable "err" will not be checked once more
> after it was determined that a call of the function
> "__mmc_set_signal_voltage" or "__mmc_switch" failed.
> Use the identifier "report_failure" instead of the label "err".

I understand the report, but this unfortunate not the proper solution.

Instead, the "if (err)" check should be entirely removed from the err label.

To do that, mmc_select_hs200() should pre validate whether 4 or 8 -bit
bus is supported which then prevents starting to switch to hs200
unless really supported. Moreover it means mmc_select_bus_width()
shall return 0 to indicate success instead of as now, returning a
positive value or 0.

Kind regards
Uffe

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/mmc/core/mmc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 549c56e..866f72b 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1256,7 +1256,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>
>         /* If fails try again during next card power cycle */
>         if (err)
> -               goto err;
> +               goto report_failure;
>
>         mmc_select_driver_type(card);
>
> @@ -1276,7 +1276,7 @@ static int mmc_select_hs200(struct mmc_card *card)
>                                    card->ext_csd.generic_cmd6_time,
>                                    true, send_status, true);
>                 if (err)
> -                       goto err;
> +                       goto report_failure;
>                 old_timing = host->ios.timing;
>                 mmc_set_timing(host, MMC_TIMING_MMC_HS200);
>                 if (!send_status) {
> @@ -1289,10 +1289,11 @@ static int mmc_select_hs200(struct mmc_card *card)
>                                 mmc_set_timing(host, old_timing);
>                 }
>         }
> -err:
> -       if (err)
> +       if (err) {
> +report_failure:
>                 pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
>                        __func__, err);
> +       }
>         return err;
>  }
>
> --
> 2.6.3
>

Re: [PATCH 0/2] mmc-host: Fine-tuning for one function

[Ulf Hansson]

On 29 December 2015 at 21:57, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 29 Dec 2015 21:54:14 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
>   Delete unnecessary variable initialisations in sdricoh_init_mmc()
>   Less checks in sdricoh_init_mmc() after error detection
>
>  drivers/mmc/host/sdricoh_cs.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> --
> 2.6.3
>

Thanks, applied for next - with some minor change to the prefix of the
commit message header.

Kind regards
Uffe

Re: [PATCH 1/2] mmc-sdricoh_cs: Delete unnecessary variable initialisations in sdricoh_init_mmc()

[Sascha Sommer]

Hello,

Am Tue, 29 Dec 2015 22:00:35 +0100
schrieb SF Markus Elfring <elfring@users.sourceforge.net>:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 29 Dec 2015 21:11:45 +0100
> 
> These variables will eventually be set to an appropriate value a bit
> later.
> * host
> * iobase
> * result
> 
> Thus let us omit the explicit initialisation at the beginning.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Sascha Sommer <saschasommer@freenet.de>


Best regards

Sascha

Re: [PATCH 2/2] mmc-sdricoh_cs: Less checks in sdricoh_init_mmc() after, error detection

[Sascha Sommer]

Hello,

Am Tue, 29 Dec 2015 22:02:37 +0100
schrieb SF Markus Elfring <elfring@users.sourceforge.net>:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 29 Dec 2015 21:45:34 +0100
> 
> This issue was detected by using the Coccinelle software.
> 
> Two pointer checks could be repeated by the sdricoh_init_mmc()
> function during error handling even if the relevant properties can be
> determined for the involved variables before by source code analysis.
> 
> * This implementation detail could be improved by adjustments
>   for jump targets according to the Linux coding style convention.
> 
> * Drop an unnecessary initialisation for the variable "mmc" then.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---

Acked-by: Sascha Sommer <saschasommer@freenet.de>

Best regards

Sascha

[PATCH 0/2] mmc-block: Fine-tuning for mmc_blk_ioctl_copy_from_user()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 23:00:23 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Use memdup_user()
  Rename jump labels

 drivers/mmc/card/block.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

-- 
2.9.3

[PATCH 1/2] mmc-block: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 22:46:38 +0200

* Reuse existing functionality from memdup_user() instead of keeping
  duplicate source code.

  This issue was detected by using the Coccinelle software.

* Delete the integer variable "err" then because the pointer
  variable "idata" should be sufficient to handle return values alone
  in this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mmc/card/block.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 48a5dd7..6ce9492 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -337,22 +337,21 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
 	struct mmc_ioc_cmd __user *user)
 {
 	struct mmc_blk_ioc_data *idata;
-	int err;
 
 	idata = kmalloc(sizeof(*idata), GFP_KERNEL);
 	if (!idata) {
-		err = -ENOMEM;
+		idata = ERR_PTR(-ENOMEM);
 		goto out;
 	}
 
 	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
-		err = -EFAULT;
+		idata = ERR_PTR(-EFAULT);
 		goto idata_err;
 	}
 
 	idata->buf_bytes = (u64) idata->ic.blksz * idata->ic.blocks;
 	if (idata->buf_bytes > MMC_IOC_MAX_BYTES) {
-		err = -EOVERFLOW;
+		idata = ERR_PTR(-EOVERFLOW);
 		goto idata_err;
 	}
 
@@ -361,26 +360,19 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
 		return idata;
 	}
 
-	idata->buf = kmalloc(idata->buf_bytes, GFP_KERNEL);
-	if (!idata->buf) {
-		err = -ENOMEM;
+	idata->buf = memdup_user((void __user *)(unsigned long)
+				 idata->ic.data_ptr,
+				 idata->buf_bytes);
+	if (IS_ERR(idata->buf)) {
+		idata = (void *) idata->buf;
 		goto idata_err;
 	}
-
-	if (copy_from_user(idata->buf, (void __user *)(unsigned long)
-					idata->ic.data_ptr, idata->buf_bytes)) {
-		err = -EFAULT;
-		goto copy_err;
-	}
-
 	return idata;
 
-copy_err:
-	kfree(idata->buf);
 idata_err:
 	kfree(idata);
 out:
-	return ERR_PTR(err);
+	return idata;
 }
 
 static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
-- 
2.9.3

[PATCH 2/2] mmc-block: Rename jump labels in mmc_blk_ioctl_copy_from_user()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 22:52:50 +0200

Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mmc/card/block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 6ce9492..0d83c56 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -346,13 +346,13 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
 
 	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
 		idata = ERR_PTR(-EFAULT);
-		goto idata_err;
+		goto free_idata;
 	}
 
 	idata->buf_bytes = (u64) idata->ic.blksz * idata->ic.blocks;
 	if (idata->buf_bytes > MMC_IOC_MAX_BYTES) {
 		idata = ERR_PTR(-EOVERFLOW);
-		goto idata_err;
+		goto free_idata;
 	}
 
 	if (!idata->buf_bytes) {
@@ -365,11 +365,11 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
 				 idata->buf_bytes);
 	if (IS_ERR(idata->buf)) {
 		idata = (void *) idata->buf;
-		goto idata_err;
+		goto free_idata;
 	}
 	return idata;
 
-idata_err:
+free_idata:
 	kfree(idata);
 out:
 	return idata;
-- 
2.9.3

Re: [PATCH 1/2] mmc-block: Use memdup_user() rather than duplicating its implementation

[walter harms]

Am 19.08.2016 23:10, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 19 Aug 2016 22:46:38 +0200
> 
> * Reuse existing functionality from memdup_user() instead of keeping
>   duplicate source code.
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Delete the integer variable "err" then because the pointer
>   variable "idata" should be sufficient to handle return values alone
>   in this function.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/mmc/card/block.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 48a5dd7..6ce9492 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -337,22 +337,21 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
>  	struct mmc_ioc_cmd __user *user)
>  {
>  	struct mmc_blk_ioc_data *idata;
> -	int err;
>  
>  	idata = kmalloc(sizeof(*idata), GFP_KERNEL);
>  	if (!idata) {
> -		err = -ENOMEM;
> +		idata = ERR_PTR(-ENOMEM);
>  		goto out;
>  	}
>  
>  	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
> -		err = -EFAULT;
> +		idata = ERR_PTR(-EFAULT);
>  		goto idata_err;
>  	}
>  
>  	idata->buf_bytes = (u64) idata->ic.blksz * idata->ic.blocks;
>  	if (idata->buf_bytes > MMC_IOC_MAX_BYTES) {
> -		err = -EOVERFLOW;
> +		idata = ERR_PTR(-EOVERFLOW);
>  		goto idata_err;
>  	}
>  
> @@ -361,26 +360,19 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
>  		return idata;
>  	}
>  
> -	idata->buf = kmalloc(idata->buf_bytes, GFP_KERNEL);
> -	if (!idata->buf) {
> -		err = -ENOMEM;
> +	idata->buf = memdup_user((void __user *)(unsigned long)
> +				 idata->ic.data_ptr,
> +				 idata->buf_bytes);
> +	if (IS_ERR(idata->buf)) {
> +		idata = (void *) idata->buf;
>  		goto idata_err;
>  	}
> -
> -	if (copy_from_user(idata->buf, (void __user *)(unsigned long)
> -					idata->ic.data_ptr, idata->buf_bytes)) {
> -		err = -EFAULT;
> -		goto copy_err;
> -	}
> -
>  	return idata;
>  
> -copy_err:
> -	kfree(idata->buf);
>  idata_err:
>  	kfree(idata);
>  out:
> -	return ERR_PTR(err);
> +	return idata;
>  }


This looks strange, returning a freed pointer is a bad idea. I suggest a
idata=NULL after kfree().

re,
 wh

>  
>  static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,