| Message ID | 20160726123246.879426-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Arnd, On Tue, Jul 26, 2016 at 8:32 PM, Arnd Bergmann <arnd@arndb.de> wrote: > The driver has just gained a slightly incorrect pm-sleep implementation that causes > warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not: > > drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function] > static int sdhci_esdhc_resume(struct device *dev) > ^~~~~~~~~~~~~~~~~~ > drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function] > static int sdhci_esdhc_suspend(struct device *dev) > > This replaces the incorrect #ifdef with a __maybe_unused annotation that does > the right thing in all configurations and is more readable. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback") Thanks for the fix. Looks like the default kernel Makefile does not enable -Werror=unused-function wanrings, so did not expose it before. Acked-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Dong Aisheng > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 2bb326bbc34a..593e34053c4b 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > return 0; > } > > -#ifdef CONFIG_PM > -static int sdhci_esdhc_suspend(struct device *dev) > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev) > { > return sdhci_pltfm_suspend(dev); > } > > -static int sdhci_esdhc_resume(struct device *dev) > +static int __maybe_unused sdhci_esdhc_resume(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > > @@ -1309,7 +1308,7 @@ static int sdhci_esdhc_resume(struct device *dev) > return sdhci_pltfm_resume(dev); > } > > -static int sdhci_esdhc_runtime_suspend(struct device *dev) > +static int __maybe_unused sdhci_esdhc_runtime_suspend(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -1327,7 +1326,7 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev) > return ret; > } > > -static int sdhci_esdhc_runtime_resume(struct device *dev) > +static int __maybe_unused sdhci_esdhc_runtime_resume(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -1341,7 +1340,6 @@ static int sdhci_esdhc_runtime_resume(struct device *dev) > > return sdhci_runtime_resume_host(host); > } > -#endif > > static const struct dev_pm_ops sdhci_esdhc_pmops = { > SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume) > -- > 2.9.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 -- 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 Tuesday, July 26, 2016 9:41:50 PM CEST Dong Aisheng wrote: > > On Tue, Jul 26, 2016 at 8:32 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > The driver has just gained a slightly incorrect pm-sleep implementation that causes > > warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not: > > > > drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function] > > static int sdhci_esdhc_resume(struct device *dev) > > ^~~~~~~~~~~~~~~~~~ > > drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function] > > static int sdhci_esdhc_suspend(struct device *dev) > > > > This replaces the incorrect #ifdef with a __maybe_unused annotation that does > > the right thing in all configurations and is more readable. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback") > > Thanks for the fix. > Looks like the default kernel Makefile does not enable > -Werror=unused-function wanrings, > so did not expose it before. They are enabled by default as warnings, not errors. My test build setup uses -Werror to turn all warnings into errors so I can catch them more easily as failed builds. > Acked-by: Dong Aisheng <aisheng.dong@nxp.com> > Thanks, Arnd -- 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 26 July 2016 at 14:32, Arnd Bergmann <arnd@arndb.de> wrote: > The driver has just gained a slightly incorrect pm-sleep implementation that causes > warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not: > > drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function] > static int sdhci_esdhc_resume(struct device *dev) > ^~~~~~~~~~~~~~~~~~ > drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function] > static int sdhci_esdhc_suspend(struct device *dev) > > This replaces the incorrect #ifdef with a __maybe_unused annotation that does > the right thing in all configurations and is more readable. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback") > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > index 2bb326bbc34a..593e34053c4b 100644 > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > return 0; > } > > -#ifdef CONFIG_PM > -static int sdhci_esdhc_suspend(struct device *dev) > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev) Instead of using __maybe_unused, I prefer to change above "#ifdef CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP". I do realize that the runtime PM callbacks still requires #ifdef CONFIG_PM, so yes that's requires an extra "ifdef". Sure, it's more a matter of taste (and micro optimizations). > { > return sdhci_pltfm_suspend(dev); > } > > -static int sdhci_esdhc_resume(struct device *dev) > +static int __maybe_unused sdhci_esdhc_resume(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > > @@ -1309,7 +1308,7 @@ static int sdhci_esdhc_resume(struct device *dev) > return sdhci_pltfm_resume(dev); > } > > -static int sdhci_esdhc_runtime_suspend(struct device *dev) > +static int __maybe_unused sdhci_esdhc_runtime_suspend(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -1327,7 +1326,7 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev) > return ret; > } > > -static int sdhci_esdhc_runtime_resume(struct device *dev) > +static int __maybe_unused sdhci_esdhc_runtime_resume(struct device *dev) > { > struct sdhci_host *host = dev_get_drvdata(dev); > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > @@ -1341,7 +1340,6 @@ static int sdhci_esdhc_runtime_resume(struct device *dev) > > return sdhci_runtime_resume_host(host); > } > -#endif > > static const struct dev_pm_ops sdhci_esdhc_pmops = { > SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume) > -- > 2.9.0 > Kind regards Uffe -- 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 Tuesday, July 26, 2016 10:18:53 PM CEST Ulf Hansson wrote: > > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > > index 2bb326bbc34a..593e34053c4b 100644 > > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > > return 0; > > } > > > > -#ifdef CONFIG_PM > > -static int sdhci_esdhc_suspend(struct device *dev) > > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev) > > Instead of using __maybe_unused, I prefer to change above "#ifdef > CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP". > I do realize that the runtime PM callbacks still requires #ifdef > CONFIG_PM, so yes that's requires an extra "ifdef". > > Sure, it's more a matter of taste (and micro optimizations). I was hoping that we could eventually do a mass-conversion to __maybe_unused, as everybody seems to get the #ifdef wrong. Any specific reason for your preference? Arnd -- 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 26 July 2016 at 22:56, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday, July 26, 2016 10:18:53 PM CEST Ulf Hansson wrote: >> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c >> > index 2bb326bbc34a..593e34053c4b 100644 >> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c >> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c >> > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) >> > return 0; >> > } >> > >> > -#ifdef CONFIG_PM >> > -static int sdhci_esdhc_suspend(struct device *dev) >> > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev) >> >> Instead of using __maybe_unused, I prefer to change above "#ifdef >> CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP". >> I do realize that the runtime PM callbacks still requires #ifdef >> CONFIG_PM, so yes that's requires an extra "ifdef". >> >> Sure, it's more a matter of taste (and micro optimizations). > > I was hoping that we could eventually do a mass-conversion to > __maybe_unused, as everybody seems to get the #ifdef wrong. > > Any specific reason for your preference? Only that this is how I get used to do it - and that it becomes a bit more clear what is needed to support the various PM configurations. If you still insist on the "maybe_unused" option, that's okay as well. Kind regards Uffe -- 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 Wednesday, July 27, 2016 12:48:23 AM CEST Ulf Hansson wrote: > On 26 July 2016 at 22:56, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday, July 26, 2016 10:18:53 PM CEST Ulf Hansson wrote: > >> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c > >> > index 2bb326bbc34a..593e34053c4b 100644 > >> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c > >> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c > >> > @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) > >> > return 0; > >> > } > >> > > >> > -#ifdef CONFIG_PM > >> > -static int sdhci_esdhc_suspend(struct device *dev) > >> > +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev) > >> > >> Instead of using __maybe_unused, I prefer to change above "#ifdef > >> CONFIG_PMf" to "#ifdef CONFIG_PM_SLEEP". > >> I do realize that the runtime PM callbacks still requires #ifdef > >> CONFIG_PM, so yes that's requires an extra "ifdef". > >> > >> Sure, it's more a matter of taste (and micro optimizations). > > > > I was hoping that we could eventually do a mass-conversion to > > __maybe_unused, as everybody seems to get the #ifdef wrong. > > > > Any specific reason for your preference? > > Only that this is how I get used to do it - and that it becomes a bit > more clear what is needed to support the various PM configurations. > > If you still insist on the "maybe_unused" option, that's okay as well. I only have a mild preference for __maybe_unused because it's harder to get wrong. I have sent dozens of bugfixes for drivers that have incorrect #ifdef, and I'd prefer not having to send that many in the future, so I try to convert them to __maybe_unused whenever a problem shows up to slowly change the coding style in the kernel. Build time coverage is another point here, but that is also what caused problems in my patch since the declarations were hidden (I assume not intentionally). Arnd -- 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-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c index 2bb326bbc34a..593e34053c4b 100644 --- a/drivers/mmc/host/sdhci-esdhc-imx.c +++ b/drivers/mmc/host/sdhci-esdhc-imx.c @@ -1293,13 +1293,12 @@ static int sdhci_esdhc_imx_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM -static int sdhci_esdhc_suspend(struct device *dev) +static int __maybe_unused sdhci_esdhc_suspend(struct device *dev) { return sdhci_pltfm_suspend(dev); } -static int sdhci_esdhc_resume(struct device *dev) +static int __maybe_unused sdhci_esdhc_resume(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); @@ -1309,7 +1308,7 @@ static int sdhci_esdhc_resume(struct device *dev) return sdhci_pltfm_resume(dev); } -static int sdhci_esdhc_runtime_suspend(struct device *dev) +static int __maybe_unused sdhci_esdhc_runtime_suspend(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -1327,7 +1326,7 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev) return ret; } -static int sdhci_esdhc_runtime_resume(struct device *dev) +static int __maybe_unused sdhci_esdhc_runtime_resume(struct device *dev) { struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -1341,7 +1340,6 @@ static int sdhci_esdhc_runtime_resume(struct device *dev) return sdhci_runtime_resume_host(host); } -#endif static const struct dev_pm_ops sdhci_esdhc_pmops = { SET_SYSTEM_SLEEP_PM_OPS(sdhci_esdhc_suspend, sdhci_esdhc_resume)
The driver has just gained a slightly incorrect pm-sleep implementation that causes warnings when CONFIG_PM is set but CONFIG_PM_SLEEP is not: drivers/mmc/host/sdhci-esdhc-imx.c:1302:12: error: 'sdhci_esdhc_resume' defined but not used [-Werror=unused-function] static int sdhci_esdhc_resume(struct device *dev) ^~~~~~~~~~~~~~~~~~ drivers/mmc/host/sdhci-esdhc-imx.c:1297:12: error: 'sdhci_esdhc_suspend' defined but not used [-Werror=unused-function] static int sdhci_esdhc_suspend(struct device *dev) This replaces the incorrect #ifdef with a __maybe_unused annotation that does the right thing in all configurations and is more readable. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Fixes: b70d0b3b5b29 ("mmc: sdhci-esdhc-imx: add esdhc specific suspend resume callback") --- drivers/mmc/host/sdhci-esdhc-imx.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)