| Message ID | 20201203222922.1067522-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mediatek: mark PM functions as __maybe_unused | expand |
On Thu, 3 Dec 2020 at 23:29, Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > The #ifdef check for the suspend/resume functions is wrong: > > drivers/mmc/host/mtk-sd.c:2765:12: error: unused function 'msdc_suspend' [-Werror,-Wunused-function] > static int msdc_suspend(struct device *dev) > drivers/mmc/host/mtk-sd.c:2779:12: error: unused function 'msdc_resume' [-Werror,-Wunused-function] > static int msdc_resume(struct device *dev) > > Remove the #ifdef and mark all four as __maybe_unused to aovid the > problem. > > Fixes: c0a2074ac575 ("mmc: mediatek: Fix system suspend/resume support for CQHCI") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/mmc/host/mtk-sd.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > index d057fb11112a..de09c6347524 100644 > --- a/drivers/mmc/host/mtk-sd.c > +++ b/drivers/mmc/host/mtk-sd.c > @@ -2683,7 +2683,6 @@ static int msdc_drv_remove(struct platform_device *pdev) > return 0; > } > > -#ifdef CONFIG_PM > static void msdc_save_reg(struct msdc_host *host) Shouldn't msdc_save|restore_reg() be turned into "__maybe_unused" as well? > { > u32 tune_reg = host->dev_comp->pad_tune_reg; > @@ -2742,7 +2741,7 @@ static void msdc_restore_reg(struct msdc_host *host) > __msdc_enable_sdio_irq(host, 1); > } > > -static int msdc_runtime_suspend(struct device *dev) > +static int __maybe_unused msdc_runtime_suspend(struct device *dev) > { > struct mmc_host *mmc = dev_get_drvdata(dev); > struct msdc_host *host = mmc_priv(mmc); > @@ -2752,7 +2751,7 @@ static int msdc_runtime_suspend(struct device *dev) > return 0; > } > > -static int msdc_runtime_resume(struct device *dev) > +static int __maybe_unused msdc_runtime_resume(struct device *dev) > { > struct mmc_host *mmc = dev_get_drvdata(dev); > struct msdc_host *host = mmc_priv(mmc); > @@ -2762,7 +2761,7 @@ static int msdc_runtime_resume(struct device *dev) > return 0; > } > > -static int msdc_suspend(struct device *dev) > +static int __maybe_unused msdc_suspend(struct device *dev) > { > struct mmc_host *mmc = dev_get_drvdata(dev); > int ret; > @@ -2776,11 +2775,10 @@ static int msdc_suspend(struct device *dev) > return pm_runtime_force_suspend(dev); > } > > -static int msdc_resume(struct device *dev) > +static int __maybe_unused msdc_resume(struct device *dev) > { > return pm_runtime_force_resume(dev); > } > -#endif > > static const struct dev_pm_ops msdc_dev_pm_ops = { You may also change this to a __maybe_unused, as long as you also assign the .pm pointer in the mt_msdc_driver with pm_ptr(&msdc_dev_pm_ops). Ideally the compiler should drop these functions/datas entirely then. > SET_SYSTEM_SLEEP_PM_OPS(msdc_suspend, msdc_resume) > -- > 2.27.0 > Kind regards Uffe
On Fri, Dec 4, 2020 at 11:02 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Thu, 3 Dec 2020 at 23:29, Arnd Bergmann <arnd@kernel.org> wrote: > > -#ifdef CONFIG_PM > > static void msdc_save_reg(struct msdc_host *host) > > Shouldn't msdc_save|restore_reg() be turned into "__maybe_unused" as well? There is no need since the compiler can figure that out already when there is a reference to the function from dead code. > > > > -static int msdc_resume(struct device *dev) > > +static int __maybe_unused msdc_resume(struct device *dev) > > { > > return pm_runtime_force_resume(dev); > > } > > -#endif > > > > static const struct dev_pm_ops msdc_dev_pm_ops = { > > You may also change this to a __maybe_unused, as long as you also > assign the .pm pointer in the mt_msdc_driver with > pm_ptr(&msdc_dev_pm_ops). > > Ideally the compiler should drop these functions/datas entirely then. I don't see a lot of other instances of that yet, and it's fairly new. Maybe we should fix it before it gets propagated further. I would suggest we redefine pm_ptr like #define pm_ptr(_ptr) (IS_ENABLED(CONFIG_PM) ? (_ptr) : NULL) and remove the __maybe_unused annotations on those that we already have. This also has the effect of dropping the unused data from the object, but without having to an an #ifdef or __maybe_unused. Adding Paul and Rafael to Cc for clarification on this. Arnd
On Fri, 4 Dec 2020 at 15:14, Arnd Bergmann <arnd@kernel.org> wrote: > > On Fri, Dec 4, 2020 at 11:02 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 3 Dec 2020 at 23:29, Arnd Bergmann <arnd@kernel.org> wrote: > > > > -#ifdef CONFIG_PM > > > static void msdc_save_reg(struct msdc_host *host) > > > > Shouldn't msdc_save|restore_reg() be turned into "__maybe_unused" as well? > > There is no need since the compiler can figure that out already when there > is a reference to the function from dead code. Alright, thanks for clarifying. > > > > > > > -static int msdc_resume(struct device *dev) > > > +static int __maybe_unused msdc_resume(struct device *dev) > > > { > > > return pm_runtime_force_resume(dev); > > > } > > > -#endif > > > > > > static const struct dev_pm_ops msdc_dev_pm_ops = { > > > > You may also change this to a __maybe_unused, as long as you also > > assign the .pm pointer in the mt_msdc_driver with > > pm_ptr(&msdc_dev_pm_ops). > > > > Ideally the compiler should drop these functions/datas entirely then. > > I don't see a lot of other instances of that yet, and it's fairly new. > Maybe we should fix it before it gets propagated further. > > I would suggest we redefine pm_ptr like > > #define pm_ptr(_ptr) (IS_ENABLED(CONFIG_PM) ? (_ptr) : NULL) Why is this better than the original definition? > > and remove the __maybe_unused annotations on those that we > already have. This also has the effect of dropping the unused > data from the object, but without having to an an #ifdef or > __maybe_unused. I didn't quite get this (sorry it's Friday afternoon... getting tired), can you perhaps give a concrete example? That said, I have applied your patch for fixes, but let's try to sort out the above to make sure we are all on the same page. Kind regards Uffe
On Fri, Dec 4, 2020 at 3:38 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: . > > > > I don't see a lot of other instances of that yet, and it's fairly new. > > Maybe we should fix it before it gets propagated further. > > > > I would suggest we redefine pm_ptr like > > > > #define pm_ptr(_ptr) (IS_ENABLED(CONFIG_PM) ? (_ptr) : NULL) > > Why is this better than the original definition? It tells the compiler that the _ptr is referenced from here, so it does not warn about an unused symbol, but at the same time it still knows that it can discard it along with the functions referenced by it and should not emit any of that output. > > and remove the __maybe_unused annotations on those that we > > already have. This also has the effect of dropping the unused > > data from the object, but without having to an an #ifdef or > > __maybe_unused. > > I didn't quite get this (sorry it's Friday afternoon... getting > tired), can you perhaps give a concrete example? These work: a) static const struct dev_pm_ops __maybe_unused ops = { ... }; ... .ops = &ops, ... b) static const struct dev_pm_ops ops = { ... }; ... .ops = &ops, ... c) #ifdef CONFIG_PM static const struct dev_pm_ops ops = { ... }; #endif ... #ifdef CONFIG_PM .ops = ops, #endif ... d) static const struct dev_pm_ops __maybe_unused ops = { ... }; ... #ifdef CONFIG_PM .ops = ops, #endif ... e) static const struct dev_pm_ops ops = { ... }; ... .ops = IS_ENABLED(CONFIG_PM) ? ops : NULL, ... But these do not work: f) #ifdef CONFIG_PM static const struct dev_pm_ops ops = { ... }; #endif ... /* error: missing declaration for ops */ .ops = IS_ENABLED(CONFIG_PM) ? ops : NULL, ... g) static struct dev_pm_ops ops = { ... }; ... /* warning: unused variable */ #ifndef CONFIG_PM .ops = NULL, #else .ops = ops, #endif ... Arnd
Hi, Le ven. 4 déc. 2020 à 15:14, Arnd Bergmann <arnd@kernel.org> a écrit : > On Fri, Dec 4, 2020 at 11:02 AM Ulf Hansson <ulf.hansson@linaro.org> > wrote: >> On Thu, 3 Dec 2020 at 23:29, Arnd Bergmann <arnd@kernel.org> wrote: > >> > -#ifdef CONFIG_PM >> > static void msdc_save_reg(struct msdc_host *host) >> >> Shouldn't msdc_save|restore_reg() be turned into "__maybe_unused" >> as well? > > There is no need since the compiler can figure that out already when > there > is a reference to the function from dead code. > >> > >> > -static int msdc_resume(struct device *dev) >> > +static int __maybe_unused msdc_resume(struct device *dev) >> > { >> > return pm_runtime_force_resume(dev); >> > } >> > -#endif >> > >> > static const struct dev_pm_ops msdc_dev_pm_ops = { >> >> You may also change this to a __maybe_unused, as long as you also >> assign the .pm pointer in the mt_msdc_driver with >> pm_ptr(&msdc_dev_pm_ops). >> >> Ideally the compiler should drop these functions/datas entirely >> then. > > I don't see a lot of other instances of that yet, and it's fairly new. > Maybe we should fix it before it gets propagated further. > > I would suggest we redefine pm_ptr like > > #define pm_ptr(_ptr) (IS_ENABLED(CONFIG_PM) ? (_ptr) : NULL) > > and remove the __maybe_unused annotations on those that we > already have. This also has the effect of dropping the unused > data from the object, but without having to an an #ifdef or > __maybe_unused. > > Adding Paul and Rafael to Cc for clarification on this. I didn't think about that. That's smarter and much more elegant. Cheers, -Paul
Hi Arnd, Le ven. 4 déc. 2020 à 15:14, Arnd Bergmann <arnd@kernel.org> a écrit : > On Fri, Dec 4, 2020 at 11:02 AM Ulf Hansson <ulf.hansson@linaro.org> > wrote: >> On Thu, 3 Dec 2020 at 23:29, Arnd Bergmann <arnd@kernel.org> wrote: > >> > -#ifdef CONFIG_PM >> > static void msdc_save_reg(struct msdc_host *host) >> >> Shouldn't msdc_save|restore_reg() be turned into "__maybe_unused" >> as well? > > There is no need since the compiler can figure that out already when > there > is a reference to the function from dead code. > >> > >> > -static int msdc_resume(struct device *dev) >> > +static int __maybe_unused msdc_resume(struct device *dev) >> > { >> > return pm_runtime_force_resume(dev); >> > } >> > -#endif >> > >> > static const struct dev_pm_ops msdc_dev_pm_ops = { >> >> You may also change this to a __maybe_unused, as long as you also >> assign the .pm pointer in the mt_msdc_driver with >> pm_ptr(&msdc_dev_pm_ops). >> >> Ideally the compiler should drop these functions/datas entirely >> then. > > I don't see a lot of other instances of that yet, and it's fairly new. > Maybe we should fix it before it gets propagated further. > > I would suggest we redefine pm_ptr like > > #define pm_ptr(_ptr) (IS_ENABLED(CONFIG_PM) ? (_ptr) : NULL) By the way, as I'm ending up doing the same in a different context, I think it would be useful to have a IF_ENABLED() macro defined like this: #define IF_ENABLED(_cfg, _ptr) (IS_ENABLED(_cfg) ? (_ptr) : NULL) Then the pm_ptr(_ptr) macro could be defined like this: #define pm_ptr(_ptr) IF_ENABLED(CONFIG_PM, _ptr) Cheers, -Paul > and remove the __maybe_unused annotations on those that we > already have. This also has the effect of dropping the unused > data from the object, but without having to an an #ifdef or > __maybe_unused. > > Adding Paul and Rafael to Cc for clarification on this. > > Arnd
On Mon, Dec 7, 2020 at 1:33 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le ven. 4 déc. 2020 à 15:14, Arnd Bergmann <arnd@kernel.org> a écrit > By the way, as I'm ending up doing the same in a different context, I > think it would be useful to have a IF_ENABLED() macro defined like this: > > #define IF_ENABLED(_cfg, _ptr) (IS_ENABLED(_cfg) ? (_ptr) : NULL) > > Then the pm_ptr(_ptr) macro could be defined like this: > > #define pm_ptr(_ptr) IF_ENABLED(CONFIG_PM, _ptr) I like that. Do you just want to go ahead and start with adding IF_ENABLED() to your own branch then? Arnd
Le mar. 8 déc. 2020 à 15:04, Arnd Bergmann <arnd@kernel.org> a écrit : > On Mon, Dec 7, 2020 at 1:33 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> Le ven. 4 déc. 2020 à 15:14, Arnd Bergmann <arnd@kernel.org> a >> écrit > >> By the way, as I'm ending up doing the same in a different context, >> I >> think it would be useful to have a IF_ENABLED() macro defined like >> this: >> >> #define IF_ENABLED(_cfg, _ptr) (IS_ENABLED(_cfg) ? (_ptr) : NULL) >> >> Then the pm_ptr(_ptr) macro could be defined like this: >> >> #define pm_ptr(_ptr) IF_ENABLED(CONFIG_PM, _ptr) > > I like that. Do you just want to go ahead and start with adding > IF_ENABLED() to your own branch then? Sure. I'll send a patch later today and Cc you (and linux-arm?). Cheers, -Paul
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c index d057fb11112a..de09c6347524 100644 --- a/drivers/mmc/host/mtk-sd.c +++ b/drivers/mmc/host/mtk-sd.c @@ -2683,7 +2683,6 @@ static int msdc_drv_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM static void msdc_save_reg(struct msdc_host *host) { u32 tune_reg = host->dev_comp->pad_tune_reg; @@ -2742,7 +2741,7 @@ static void msdc_restore_reg(struct msdc_host *host) __msdc_enable_sdio_irq(host, 1); } -static int msdc_runtime_suspend(struct device *dev) +static int __maybe_unused msdc_runtime_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct msdc_host *host = mmc_priv(mmc); @@ -2752,7 +2751,7 @@ static int msdc_runtime_suspend(struct device *dev) return 0; } -static int msdc_runtime_resume(struct device *dev) +static int __maybe_unused msdc_runtime_resume(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); struct msdc_host *host = mmc_priv(mmc); @@ -2762,7 +2761,7 @@ static int msdc_runtime_resume(struct device *dev) return 0; } -static int msdc_suspend(struct device *dev) +static int __maybe_unused msdc_suspend(struct device *dev) { struct mmc_host *mmc = dev_get_drvdata(dev); int ret; @@ -2776,11 +2775,10 @@ static int msdc_suspend(struct device *dev) return pm_runtime_force_suspend(dev); } -static int msdc_resume(struct device *dev) +static int __maybe_unused msdc_resume(struct device *dev) { return pm_runtime_force_resume(dev); } -#endif static const struct dev_pm_ops msdc_dev_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(msdc_suspend, msdc_resume)