| Message ID | 20170516061720.1755-1-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)