* [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP @ 2020-10-29 7:41 Coiby Xu 2020-10-29 10:04 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Coiby Xu @ 2020-10-29 7:41 UTC (permalink / raw) To: Ingo Molnar, Darren Hart, Borislav Petkov, Thomas Gleixner Cc: Andy Shevchenko, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 PLATFORM DRIVERS - ARCH, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. Signed-off-by: Coiby Xu <coiby.xu@gmail.com> --- arch/x86/platform/olpc/olpc-xo15-sci.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/arch/x86/platform/olpc/olpc-xo15-sci.c b/arch/x86/platform/olpc/olpc-xo15-sci.c index 85f4638764d6..716eefd735a4 100644 --- a/arch/x86/platform/olpc/olpc-xo15-sci.c +++ b/arch/x86/platform/olpc/olpc-xo15-sci.c @@ -192,7 +192,6 @@ static int xo15_sci_remove(struct acpi_device *device) return 0; } -#ifdef CONFIG_PM_SLEEP static int xo15_sci_resume(struct device *dev) { /* Enable all EC events */ @@ -204,7 +203,6 @@ static int xo15_sci_resume(struct device *dev) return 0; } -#endif static SIMPLE_DEV_PM_OPS(xo15_sci_pm, NULL, xo15_sci_resume); -- 2.28.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP 2020-10-29 7:41 [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP Coiby Xu @ 2020-10-29 10:04 ` Hans de Goede 2020-10-29 10:59 ` Coiby Xu 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2020-10-29 10:04 UTC (permalink / raw) To: Coiby Xu, Ingo Molnar, Darren Hart, Borislav Petkov, Thomas Gleixner Cc: Andy Shevchenko, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 PLATFORM DRIVERS - ARCH, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) Hi, On 10/29/20 8:41 AM, Coiby Xu wrote: > SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. No it does not, when CONFIG_PM_SLEEP is not set then the SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses is a no-op, so nothing will reference xo15_sci_resume leading to a compiler warning when CONFIG_PM_SLEEP is not set. You could drop the ifdef and add __maybe_unused to the definition of xo15_sci_resume, but that feels like needless churn, best to just keep this as is IMHO. Also s/CONFIG_PM_CONFIG/CONFIG_PM_SLEEP/ in the commit message. Regards, Hans > > Signed-off-by: Coiby Xu <coiby.xu@gmail.com> > --- > arch/x86/platform/olpc/olpc-xo15-sci.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/x86/platform/olpc/olpc-xo15-sci.c b/arch/x86/platform/olpc/olpc-xo15-sci.c > index 85f4638764d6..716eefd735a4 100644 > --- a/arch/x86/platform/olpc/olpc-xo15-sci.c > +++ b/arch/x86/platform/olpc/olpc-xo15-sci.c > @@ -192,7 +192,6 @@ static int xo15_sci_remove(struct acpi_device *device) > return 0; > } > > -#ifdef CONFIG_PM_SLEEP > static int xo15_sci_resume(struct device *dev) > { > /* Enable all EC events */ > @@ -204,7 +203,6 @@ static int xo15_sci_resume(struct device *dev) > > return 0; > } > -#endif > > static SIMPLE_DEV_PM_OPS(xo15_sci_pm, NULL, xo15_sci_resume); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP 2020-10-29 10:04 ` Hans de Goede @ 2020-10-29 10:59 ` Coiby Xu 2020-10-29 11:09 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Coiby Xu @ 2020-10-29 10:59 UTC (permalink / raw) To: Hans de Goede Cc: Ingo Molnar, Darren Hart, Borislav Petkov, Thomas Gleixner, Andy Shevchenko, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 PLATFORM DRIVERS - ARCH, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) Hi Hans, Thank you for reviewing this patch! On Thu, Oct 29, 2020 at 11:04:36AM +0100, Hans de Goede wrote: >Hi, > >On 10/29/20 8:41 AM, Coiby Xu wrote: >> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. > >No it does not, when CONFIG_PM_SLEEP is not set then the >SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses >is a no-op, so nothing will reference xo15_sci_resume leading to >a compiler warning when CONFIG_PM_SLEEP is not set. > >You could drop the ifdef and add __maybe_unused to the definition >of xo15_sci_resume, but that feels like needless churn, best to >just keep this as is IMHO. > Actually, this is a tree-wide change by some semi-automation scripts. Thank you for pointing out the issue to prevent me from releasing another ~150 emails to flood other mailing lists. Currently there are 929 drivers has device PM callbacks, $ grep -rI "\.pm = &" --include=*.c ./|wc -l 929 I put all files having device PM callbacks into four categories based on weather a file has CONFIG_PM_SLEEP or PM macro like SET_SYSTEM_SLEEP_PM_OPS, here are the statistics, 1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213 2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19 3. have PM macro but not CONFIG_PM_SLEEP: 347 4. no PM macro or CONFIG_PM_SLEEP: 302 Some drivers which have PM macro but not CONFIG_PM_SLEEP like sound/x86/intel_hdmi_audio.c indeed use __maybe_unused to eliminate the compiling warning. In 2011, there's a patch proposing to remove ONFIG_PM altogether but an objection was turning CONFIG_PM on would increase the kernel size [1]. So __maybe_unused also have this issue. (I made a mistake when I thought PM macros like SIMPLE_DEV_PM_OPS didn't have this issue). What do you think? Btw, It's easy for me to add CONFIG_PM_SLEEP for those drivers have PM macro but not CONFIG_PM_SLEEP since I have already written the necessary automation scripts. [1] https://lists.linux-foundation.org/pipermail/linux-pm/2011-February/030215.html >Also s/CONFIG_PM_CONFIG/CONFIG_PM_SLEEP/ in the commit message. > Thank you for pointing out the typo. I've written some scripts to automate the whole process from changing code to submitting patches. Somehow there is still this issue. >Regards, > >Hans > > >> >> Signed-off-by: Coiby Xu <coiby.xu@gmail.com> >> --- >> arch/x86/platform/olpc/olpc-xo15-sci.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/arch/x86/platform/olpc/olpc-xo15-sci.c b/arch/x86/platform/olpc/olpc-xo15-sci.c >> index 85f4638764d6..716eefd735a4 100644 >> --- a/arch/x86/platform/olpc/olpc-xo15-sci.c >> +++ b/arch/x86/platform/olpc/olpc-xo15-sci.c >> @@ -192,7 +192,6 @@ static int xo15_sci_remove(struct acpi_device *device) >> return 0; >> } >> >> -#ifdef CONFIG_PM_SLEEP >> static int xo15_sci_resume(struct device *dev) >> { >> /* Enable all EC events */ >> @@ -204,7 +203,6 @@ static int xo15_sci_resume(struct device *dev) >> >> return 0; >> } >> -#endif >> >> static SIMPLE_DEV_PM_OPS(xo15_sci_pm, NULL, xo15_sci_resume); >> >> > -- Best regards, Coiby ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP 2020-10-29 10:59 ` Coiby Xu @ 2020-10-29 11:09 ` Hans de Goede 2020-10-29 14:16 ` Coiby Xu 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2020-10-29 11:09 UTC (permalink / raw) To: Coiby Xu Cc: Ingo Molnar, Darren Hart, Borislav Petkov, Thomas Gleixner, Andy Shevchenko, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 PLATFORM DRIVERS - ARCH, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) Hi, On 10/29/20 11:59 AM, Coiby Xu wrote: > Hi Hans, > > Thank you for reviewing this patch! > > On Thu, Oct 29, 2020 at 11:04:36AM +0100, Hans de Goede wrote: >> Hi, >> >> On 10/29/20 8:41 AM, Coiby Xu wrote: >>> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. >> >> No it does not, when CONFIG_PM_SLEEP is not set then the >> SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses >> is a no-op, so nothing will reference xo15_sci_resume leading to >> a compiler warning when CONFIG_PM_SLEEP is not set. >> >> You could drop the ifdef and add __maybe_unused to the definition >> of xo15_sci_resume, but that feels like needless churn, best to >> just keep this as is IMHO. >> > > Actually, this is a tree-wide change by some semi-automation scripts. > Thank you for pointing out the issue to prevent me from releasing > another ~150 emails to flood other mailing lists. > > Currently there are 929 drivers has device PM callbacks, > > $ grep -rI "\.pm = &" --include=*.c ./|wc -l > 929 > > I put all files having device PM callbacks into four categories > based on weather a file has CONFIG_PM_SLEEP or PM macro like > SET_SYSTEM_SLEEP_PM_OPS, here are the statistics, > 1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213 > 2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19 > 3. have PM macro but not CONFIG_PM_SLEEP: 347 > 4. no PM macro or CONFIG_PM_SLEEP: 302 > > Some drivers which have PM macro but not CONFIG_PM_SLEEP like > sound/x86/intel_hdmi_audio.c indeed use __maybe_unused to eliminate > the compiling warning. In 2011, there's a patch proposing to remove > ONFIG_PM altogether but an objection was turning CONFIG_PM on would > increase the kernel size [1]. So __maybe_unused also have this issue. I would expect the compiler to remove the unused function, it knows it is unused, that is why __maybe_unused is necessary to suppress the warning and compilers are pretty smart and agressive wrt remove unnecessary code these days. Regards, Hans >>> --- >>> arch/x86/platform/olpc/olpc-xo15-sci.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/arch/x86/platform/olpc/olpc-xo15-sci.c b/arch/x86/platform/olpc/olpc-xo15-sci.c >>> index 85f4638764d6..716eefd735a4 100644 >>> --- a/arch/x86/platform/olpc/olpc-xo15-sci.c >>> +++ b/arch/x86/platform/olpc/olpc-xo15-sci.c >>> @@ -192,7 +192,6 @@ static int xo15_sci_remove(struct acpi_device *device) >>> return 0; >>> } >>> >>> -#ifdef CONFIG_PM_SLEEP >>> static int xo15_sci_resume(struct device *dev) >>> { >>> /* Enable all EC events */ >>> @@ -204,7 +203,6 @@ static int xo15_sci_resume(struct device *dev) >>> >>> return 0; >>> } >>> -#endif >>> >>> static SIMPLE_DEV_PM_OPS(xo15_sci_pm, NULL, xo15_sci_resume); >>> >>> >> > > -- > Best regards, > Coiby > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP 2020-10-29 11:09 ` Hans de Goede @ 2020-10-29 14:16 ` Coiby Xu 2020-10-29 16:03 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Coiby Xu @ 2020-10-29 14:16 UTC (permalink / raw) To: Hans de Goede Cc: Ingo Molnar, Darren Hart, Borislav Petkov, Thomas Gleixner, Andy Shevchenko, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 PLATFORM DRIVERS - ARCH, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Thu, Oct 29, 2020 at 12:09:23PM +0100, Hans de Goede wrote: >Hi, > >On 10/29/20 11:59 AM, Coiby Xu wrote: >> Hi Hans, >> >> Thank you for reviewing this patch! >> >> On Thu, Oct 29, 2020 at 11:04:36AM +0100, Hans de Goede wrote: >>> Hi, >>> >>> On 10/29/20 8:41 AM, Coiby Xu wrote: >>>> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. >>> >>> No it does not, when CONFIG_PM_SLEEP is not set then the >>> SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses >>> is a no-op, so nothing will reference xo15_sci_resume leading to >>> a compiler warning when CONFIG_PM_SLEEP is not set. >>> >>> You could drop the ifdef and add __maybe_unused to the definition >>> of xo15_sci_resume, but that feels like needless churn, best to >>> just keep this as is IMHO. >>> >> >> Actually, this is a tree-wide change by some semi-automation scripts. >> Thank you for pointing out the issue to prevent me from releasing >> another ~150 emails to flood other mailing lists. >> >> Currently there are 929 drivers has device PM callbacks, >> >> $ grep -rI "\.pm = &" --include=*.c ./|wc -l >> 929 >> >> I put all files having device PM callbacks into four categories >> based on weather a file has CONFIG_PM_SLEEP or PM macro like >> SET_SYSTEM_SLEEP_PM_OPS, here are the statistics, >> 1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213 >> 2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19 >> 3. have PM macro but not CONFIG_PM_SLEEP: 347 >> 4. no PM macro or CONFIG_PM_SLEEP: 302 >> >> Some drivers which have PM macro but not CONFIG_PM_SLEEP like >> sound/x86/intel_hdmi_audio.c indeed use __maybe_unused to eliminate >> the compiling warning. In 2011, there's a patch proposing to remove >> ONFIG_PM altogether but an objection was turning CONFIG_PM on would >> increase the kernel size [1]. So __maybe_unused also have this issue. > >I would expect the compiler to remove the unused function, it knows >it is unused, that is why __maybe_unused is necessary to suppress >the warning and compilers are pretty smart and agressive wrt remove >unnecessary code these days. > Then __maybe_unused is a good solution and there's also convincing reason to prefer __maybe_unused over CONFIG_PM_SLEEP according to Arnd Bergmann [2], > > By and large, drivers handle this by using a CONFIG_PM_SLEEP ifdef. > > > > Unless you can make an extremely convincing argument why not to do > > so here, I'd like you to handle it that way instead. > > [adding linux-pm to Cc] > > The main reason is that everyone gets the #ifdef wrong, I run into > half a dozen new build regressions with linux-next every week on > average, the typical problems being: > > - testing CONFIG_PM_SLEEP instead of CONFIG_PM, leading to an unused > function warning > - testing CONFIG_PM instead of CONFIG_PM_SLEEP, leading to a build > failure > - calling a function outside of the #ifdef only from inside an > otherwise correct #ifdef, again leading to an unused function > warning > - causing a warning inside of the #ifdef but only testing if that > is disabled, leading to a problem if the macro is set (this is > rare these days for CONFIG_PM as that is normally enabled) > > Using __maybe_unused avoids all of the above. >Regards, > >Hans > [2] https://lore.kernel.org/patchwork/comment/919944/ -- Best regards, Coiby ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP 2020-10-29 14:16 ` Coiby Xu @ 2020-10-29 16:03 ` Hans de Goede 0 siblings, 0 replies; 6+ messages in thread From: Hans de Goede @ 2020-10-29 16:03 UTC (permalink / raw) To: Coiby Xu Cc: Ingo Molnar, Darren Hart, Borislav Petkov, Thomas Gleixner, Andy Shevchenko, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 PLATFORM DRIVERS - ARCH, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) Hi, On 10/29/20 3:16 PM, Coiby Xu wrote: > On Thu, Oct 29, 2020 at 12:09:23PM +0100, Hans de Goede wrote: >> Hi, >> >> On 10/29/20 11:59 AM, Coiby Xu wrote: >>> Hi Hans, >>> >>> Thank you for reviewing this patch! >>> >>> On Thu, Oct 29, 2020 at 11:04:36AM +0100, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 10/29/20 8:41 AM, Coiby Xu wrote: >>>>> SIMPLE_DEV_PM_OPS has already took good care of CONFIG_PM_CONFIG. >>>> >>>> No it does not, when CONFIG_PM_SLEEP is not set then the >>>> SET_SYSTEM_SLEEP_PM_OPS macro which SIMPLE_DEV_PM_OPS uses >>>> is a no-op, so nothing will reference xo15_sci_resume leading to >>>> a compiler warning when CONFIG_PM_SLEEP is not set. >>>> >>>> You could drop the ifdef and add __maybe_unused to the definition >>>> of xo15_sci_resume, but that feels like needless churn, best to >>>> just keep this as is IMHO. >>>> >>> >>> Actually, this is a tree-wide change by some semi-automation scripts. >>> Thank you for pointing out the issue to prevent me from releasing >>> another ~150 emails to flood other mailing lists. >>> >>> Currently there are 929 drivers has device PM callbacks, >>> >>> $ grep -rI "\.pm = &" --include=*.c ./|wc -l >>> 929 >>> >>> I put all files having device PM callbacks into four categories >>> based on weather a file has CONFIG_PM_SLEEP or PM macro like >>> SET_SYSTEM_SLEEP_PM_OPS, here are the statistics, >>>  1. have both CONFIG_PM_SLEEP and PM_OPS macro: 213 >>>  2. have CONFIG_PM_SLEEP but no PM_OPS macro: 19 >>>  3. have PM macro but not CONFIG_PM_SLEEP: 347 >>>  4. no PM macro or CONFIG_PM_SLEEP: 302 >>> >>> Some drivers which have PM macro but not CONFIG_PM_SLEEP like >>> sound/x86/intel_hdmi_audio.c indeed use __maybe_unused to eliminate >>> the compiling warning. In 2011, there's a patch proposing to remove >>> ONFIG_PM altogether but an objection was turning CONFIG_PM on would >>> increase the kernel size [1]. So __maybe_unused also have this issue. >> >> I would expect the compiler to remove the unused function, it knows >> it is unused, that is why __maybe_unused is necessary to suppress >> the warning and compilers are pretty smart and agressive wrt remove >> unnecessary code these days. >> > Then __maybe_unused is a good solution and there's also convincing > reason to prefer __maybe_unused over CONFIG_PM_SLEEP according to > Arnd Bergmann [2], Ok, I would be happy to merge a patch for this which drops the #ifdef-s and adds a __maybe_unused. Regards, Hans ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-29 16:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-29 7:41 [PATCH] power: supply: olpc_battery: remove unnecessary CONFIG_PM_SLEEP Coiby Xu 2020-10-29 10:04 ` Hans de Goede 2020-10-29 10:59 ` Coiby Xu 2020-10-29 11:09 ` Hans de Goede 2020-10-29 14:16 ` Coiby Xu 2020-10-29 16:03 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®