Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642241 > unrolled thread

[PATCH] mmc: sdhci-xenon: kill xenon_clean_phy()

Started byJisheng Zhang <jszhang@marvell.com>
First post2017-05-16 08:30 +0200
Last post2017-05-19 10:40 +0200
Articles 8 — 6 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Jisheng Zhang <jszhang@marvell.com> - 2017-05-16 08:30 +0200
    Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Andrew Lunn <andrew@lunn.ch> - 2017-05-16 14:30 +0200
      Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-16 14:50 +0200
      Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Jisheng Zhang <jszhang@marvell.com> - 2017-05-17 11:30 +0200
        Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Andrew Lunn <andrew@lunn.ch> - 2017-05-17 14:00 +0200
    Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Hu Ziji <huziji@marvell.com> - 2017-05-17 09:00 +0200
    Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Adrian Hunter <adrian.hunter@intel.com> - 2017-05-17 12:30 +0200
    Re: [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy() Ulf Hansson <ulf.hansson@linaro.org> - 2017-05-19 10:40 +0200

#1642241 — [PATCH] mmc: sdhci-xenon: kill xenon_clean_phy()

FromJisheng Zhang <jszhang@marvell.com>
Date2017-05-16 08:30 +0200
Subject[PATCH] mmc: sdhci-xenon: kill xenon_clean_phy()
Message-ID<tHE9A-22D-23@gated-at.bofh.it>
Currently, the xenon_clean_phy() is only used for freeing phy_params.
The phy_params is allocated by devm_kzalloc(), there's no need to free
is explicitly.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/mmc/host/sdhci-xenon-phy.c | 14 +-------------
 drivers/mmc/host/sdhci-xenon.c     |  6 +-----
 drivers/mmc/host/sdhci-xenon.h     |  1 -
 3 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
index 6356781f1cca..f7e26b031e76 100644
--- a/drivers/mmc/host/sdhci-xenon-phy.c
+++ b/drivers/mmc/host/sdhci-xenon-phy.c
@@ -787,14 +787,6 @@ int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
 	return ret;
 }
 
-void xenon_clean_phy(struct sdhci_host *host)
-{
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
-
-	kfree(priv->phy_params);
-}
-
 static int xenon_add_phy(struct device_node *np, struct sdhci_host *host,
 			 const char *phy_name)
 {
@@ -819,11 +811,7 @@ static int xenon_add_phy(struct device_node *np, struct sdhci_host *host,
 	if (ret)
 		return ret;
 
-	ret = xenon_emmc_phy_parse_param_dt(host, np, priv->phy_params);
-	if (ret)
-		xenon_clean_phy(host);
-
-	return ret;
+	return xenon_emmc_phy_parse_param_dt(host, np, priv->phy_params);
 }
 
 int xenon_phy_parse_dt(struct device_node *np, struct sdhci_host *host)
diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
index 67246655315b..bc1781bb070b 100644
--- a/drivers/mmc/host/sdhci-xenon.c
+++ b/drivers/mmc/host/sdhci-xenon.c
@@ -486,7 +486,7 @@ static int xenon_probe(struct platform_device *pdev)
 
 	err = xenon_sdhc_prepare(host);
 	if (err)
-		goto clean_phy_param;
+		goto err_clk;
 
 	err = sdhci_add_host(host);
 	if (err)
@@ -496,8 +496,6 @@ static int xenon_probe(struct platform_device *pdev)
 
 remove_sdhc:
 	xenon_sdhc_unprepare(host);
-clean_phy_param:
-	xenon_clean_phy(host);
 err_clk:
 	clk_disable_unprepare(pltfm_host->clk);
 free_pltfm:
@@ -510,8 +508,6 @@ static int xenon_remove(struct platform_device *pdev)
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 
-	xenon_clean_phy(host);
-
 	sdhci_remove_host(host, 0);
 
 	xenon_sdhc_unprepare(host);
diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
index 6e6523ea01ce..73debb42dc2f 100644
--- a/drivers/mmc/host/sdhci-xenon.h
+++ b/drivers/mmc/host/sdhci-xenon.h
@@ -93,7 +93,6 @@ struct xenon_priv {
 };
 
 int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios);
-void xenon_clean_phy(struct sdhci_host *host);
 int xenon_phy_parse_dt(struct device_node *np,
 		       struct sdhci_host *host);
 void xenon_soc_pad_ctrl(struct sdhci_host *host,
-- 
2.11.0

[toc] | [next] | [standalone]

#1642488

FromAndrew Lunn <andrew@lunn.ch>
Date2017-05-16 14:30 +0200
Message-ID<tHJLX-5yL-1@gated-at.bofh.it>
In reply to#1642241
On Tue, May 16, 2017 at 02:17:20PM +0800, Jisheng Zhang wrote:
> Currently, the xenon_clean_phy() is only used for freeing phy_params.
> The phy_params is allocated by devm_kzalloc(), there's no need to free
> is explicitly.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/mmc/host/sdhci-xenon-phy.c | 14 +-------------
>  drivers/mmc/host/sdhci-xenon.c     |  6 +-----
>  drivers/mmc/host/sdhci-xenon.h     |  1 -
>  3 files changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
> index 6356781f1cca..f7e26b031e76 100644
> --- a/drivers/mmc/host/sdhci-xenon-phy.c
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> @@ -787,14 +787,6 @@ int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>  	return ret;
>  }
>  
> -void xenon_clean_phy(struct sdhci_host *host)
> -{
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -
> -	kfree(priv->phy_params);

Hi Jisheng

If the memory has been allocated using the devm_ API, calling kfree
here is wrong, and can lead to a double free.

So this patch should probably have a fixes: tag. Should it also goto
stable?

	Andrew

[toc] | [prev] | [next] | [standalone]

#1642535

FromRussell King - ARM Linux <linux@armlinux.org.uk>
Date2017-05-16 14:50 +0200
Message-ID<tHK5j-5F7-13@gated-at.bofh.it>
In reply to#1642488
On Tue, May 16, 2017 at 02:22:11PM +0200, Andrew Lunn wrote:
> If the memory has been allocated using the devm_ API, calling kfree
> here is wrong, and can lead to a double free.
> 
> So this patch should probably have a fixes: tag. Should it also goto
> stable?

No, sdhci-xenon has only hit mainline during the recent merge window,
so it's not in any kernel other than 4.12-rc1.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1643174

FromJisheng Zhang <jszhang@marvell.com>
Date2017-05-17 11:30 +0200
Message-ID<tI3rk-1fU-15@gated-at.bofh.it>
In reply to#1642488
On Tue, 16 May 2017 14:22:11 +0200 Andrew Lunn wrote:

> On Tue, May 16, 2017 at 02:17:20PM +0800, Jisheng Zhang wrote:
> > Currently, the xenon_clean_phy() is only used for freeing phy_params.
> > The phy_params is allocated by devm_kzalloc(), there's no need to free
> > is explicitly.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  drivers/mmc/host/sdhci-xenon-phy.c | 14 +-------------
> >  drivers/mmc/host/sdhci-xenon.c     |  6 +-----
> >  drivers/mmc/host/sdhci-xenon.h     |  1 -
> >  3 files changed, 2 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
> > index 6356781f1cca..f7e26b031e76 100644
> > --- a/drivers/mmc/host/sdhci-xenon-phy.c
> > +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> > @@ -787,14 +787,6 @@ int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
> >  	return ret;
> >  }
> >  
> > -void xenon_clean_phy(struct sdhci_host *host)
> > -{
> > -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > -
> > -	kfree(priv->phy_params);  
> 
> Hi Jisheng

Hi Andrew,

> 
> If the memory has been allocated using the devm_ API, calling kfree
> here is wrong, and can lead to a double free.

Yes. In fact, I found this issue by unbinding then binding the xenon host.


> 
> So this patch should probably have a fixes: tag. Should it also goto
> stable?

As pointed out by RMK, the sdhci-xenon driver is merged in current 4.12-rc1,
so we just need to apply this fix in later rcN.

Thanks,
Jisheng

[toc] | [prev] | [next] | [standalone]

#1643293

FromAndrew Lunn <andrew@lunn.ch>
Date2017-05-17 14:00 +0200
Message-ID<tI5Mt-2Bh-11@gated-at.bofh.it>
In reply to#1643174
> As pointed out by RMK, the sdhci-xenon driver is merged in current 4.12-rc1,
> so we just need to apply this fix in later rcN.

Yep. It helps the maintainer if you point this out in the patch, after
the ---.

    Andrew

[toc] | [prev] | [next] | [standalone]

#1643017

FromHu Ziji <huziji@marvell.com>
Date2017-05-17 09:00 +0200
Message-ID<tI16a-86C-23@gated-at.bofh.it>
In reply to#1642241
On 2017/5/16 14:17, Jisheng Zhang wrote:
> Currently, the xenon_clean_phy() is only used for freeing phy_params.
> The phy_params is allocated by devm_kzalloc(), there's no need to free
> is explicitly.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Thanks for the fix.

Acked-by Hu Ziji <huziji@marvell.com>

Best regards,
Hu Ziji

> ---
>  drivers/mmc/host/sdhci-xenon-phy.c | 14 +-------------
>  drivers/mmc/host/sdhci-xenon.c     |  6 +-----
>  drivers/mmc/host/sdhci-xenon.h     |  1 -
>  3 files changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
> index 6356781f1cca..f7e26b031e76 100644
> --- a/drivers/mmc/host/sdhci-xenon-phy.c
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> @@ -787,14 +787,6 @@ int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>  	return ret;
>  }
>  
> -void xenon_clean_phy(struct sdhci_host *host)
> -{
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -
> -	kfree(priv->phy_params);
> -}
> -
>  static int xenon_add_phy(struct device_node *np, struct sdhci_host *host,
>  			 const char *phy_name)
>  {
> @@ -819,11 +811,7 @@ static int xenon_add_phy(struct device_node *np, struct sdhci_host *host,
>  	if (ret)
>  		return ret;
>  
> -	ret = xenon_emmc_phy_parse_param_dt(host, np, priv->phy_params);
> -	if (ret)
> -		xenon_clean_phy(host);
> -
> -	return ret;
> +	return xenon_emmc_phy_parse_param_dt(host, np, priv->phy_params);
>  }
>  
>  int xenon_phy_parse_dt(struct device_node *np, struct sdhci_host *host)
> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> index 67246655315b..bc1781bb070b 100644
> --- a/drivers/mmc/host/sdhci-xenon.c
> +++ b/drivers/mmc/host/sdhci-xenon.c
> @@ -486,7 +486,7 @@ static int xenon_probe(struct platform_device *pdev)
>  
>  	err = xenon_sdhc_prepare(host);
>  	if (err)
> -		goto clean_phy_param;
> +		goto err_clk;
>  
>  	err = sdhci_add_host(host);
>  	if (err)
> @@ -496,8 +496,6 @@ static int xenon_probe(struct platform_device *pdev)
>  
>  remove_sdhc:
>  	xenon_sdhc_unprepare(host);
> -clean_phy_param:
> -	xenon_clean_phy(host);
>  err_clk:
>  	clk_disable_unprepare(pltfm_host->clk);
>  free_pltfm:
> @@ -510,8 +508,6 @@ static int xenon_remove(struct platform_device *pdev)
>  	struct sdhci_host *host = platform_get_drvdata(pdev);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  
> -	xenon_clean_phy(host);
> -
>  	sdhci_remove_host(host, 0);
>  
>  	xenon_sdhc_unprepare(host);
> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> index 6e6523ea01ce..73debb42dc2f 100644
> --- a/drivers/mmc/host/sdhci-xenon.h
> +++ b/drivers/mmc/host/sdhci-xenon.h
> @@ -93,7 +93,6 @@ struct xenon_priv {
>  };
>  
>  int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios);
> -void xenon_clean_phy(struct sdhci_host *host);
>  int xenon_phy_parse_dt(struct device_node *np,
>  		       struct sdhci_host *host);
>  void xenon_soc_pad_ctrl(struct sdhci_host *host,
>

[toc] | [prev] | [next] | [standalone]

#1643227

FromAdrian Hunter <adrian.hunter@intel.com>
Date2017-05-17 12:30 +0200
Message-ID<tI4nn-1Pa-13@gated-at.bofh.it>
In reply to#1642241
On 16/05/17 09:17, Jisheng Zhang wrote:
> Currently, the xenon_clean_phy() is only used for freeing phy_params.
> The phy_params is allocated by devm_kzalloc(), there's no need to free
> is explicitly.
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

[toc] | [prev] | [next] | [standalone]

#1645379

FromUlf Hansson <ulf.hansson@linaro.org>
Date2017-05-19 10:40 +0200
Message-ID<tILC3-7Vr-23@gated-at.bofh.it>
In reply to#1642241
On 16 May 2017 at 08:17, Jisheng Zhang <jszhang@marvell.com> wrote:
> Currently, the xenon_clean_phy() is only used for freeing phy_params.
> The phy_params is allocated by devm_kzalloc(), there's no need to free
> is explicitly.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Thanks, applied for fixes!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-xenon-phy.c | 14 +-------------
>  drivers/mmc/host/sdhci-xenon.c     |  6 +-----
>  drivers/mmc/host/sdhci-xenon.h     |  1 -
>  3 files changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c b/drivers/mmc/host/sdhci-xenon-phy.c
> index 6356781f1cca..f7e26b031e76 100644
> --- a/drivers/mmc/host/sdhci-xenon-phy.c
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> @@ -787,14 +787,6 @@ int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>         return ret;
>  }
>
> -void xenon_clean_phy(struct sdhci_host *host)
> -{
> -       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -       struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -
> -       kfree(priv->phy_params);
> -}
> -
>  static int xenon_add_phy(struct device_node *np, struct sdhci_host *host,
>                          const char *phy_name)
>  {
> @@ -819,11 +811,7 @@ static int xenon_add_phy(struct device_node *np, struct sdhci_host *host,
>         if (ret)
>                 return ret;
>
> -       ret = xenon_emmc_phy_parse_param_dt(host, np, priv->phy_params);
> -       if (ret)
> -               xenon_clean_phy(host);
> -
> -       return ret;
> +       return xenon_emmc_phy_parse_param_dt(host, np, priv->phy_params);
>  }
>
>  int xenon_phy_parse_dt(struct device_node *np, struct sdhci_host *host)
> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> index 67246655315b..bc1781bb070b 100644
> --- a/drivers/mmc/host/sdhci-xenon.c
> +++ b/drivers/mmc/host/sdhci-xenon.c
> @@ -486,7 +486,7 @@ static int xenon_probe(struct platform_device *pdev)
>
>         err = xenon_sdhc_prepare(host);
>         if (err)
> -               goto clean_phy_param;
> +               goto err_clk;
>
>         err = sdhci_add_host(host);
>         if (err)
> @@ -496,8 +496,6 @@ static int xenon_probe(struct platform_device *pdev)
>
>  remove_sdhc:
>         xenon_sdhc_unprepare(host);
> -clean_phy_param:
> -       xenon_clean_phy(host);
>  err_clk:
>         clk_disable_unprepare(pltfm_host->clk);
>  free_pltfm:
> @@ -510,8 +508,6 @@ static int xenon_remove(struct platform_device *pdev)
>         struct sdhci_host *host = platform_get_drvdata(pdev);
>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>
> -       xenon_clean_phy(host);
> -
>         sdhci_remove_host(host, 0);
>
>         xenon_sdhc_unprepare(host);
> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> index 6e6523ea01ce..73debb42dc2f 100644
> --- a/drivers/mmc/host/sdhci-xenon.h
> +++ b/drivers/mmc/host/sdhci-xenon.h
> @@ -93,7 +93,6 @@ struct xenon_priv {
>  };
>
>  int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios);
> -void xenon_clean_phy(struct sdhci_host *host);
>  int xenon_phy_parse_dt(struct device_node *np,
>                        struct sdhci_host *host);
>  void xenon_soc_pad_ctrl(struct sdhci_host *host,
> --
> 2.11.0
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web