[announce] new tree: "fix all build warnings, on all configs"

On Wed, 22 October 2008 11:47:59 +0200, Ingo Molnar wrote:
> * Jörn Engel wrote:
> > On Mon, 20 October 2008 21:21:10 +0200, Ingo Molnar wrote:
> > >
> > > /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
> > > #define fops_get(fops) \
> > > - (((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
> > > + (((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
> > > #define fops_put(fops) \
> > > - do { if (fops) module_put((fops)->owner); } while(0)
> > > + do { if (fops != NULL) module_put((fops)->owner); } while(0)
> >
> > This, I would argue, makes the code worse.
>
> In this specific case the issue is that the 'fops' parameter can
> occasionally be a constant pointer (turning the test into always-true)
> so the compiler is at least minimally correct at asking the "are you
> sure you want this" question - which we answer in the affirmative via
> the explicit NULL check. But these are really nuances.

#define fops_put(fops) do { \
if (fops != NULL) \
module_put((fops)->owner); \
} while 0

If the code was written like this, I wouldn't mind your patch at all.
But in the one-liner form, the ' != NULL' makes the difference between
fluent reading and having to actively sort out which piece belongs
where.

And I bet that if it hadn't been a macro but an inline function, you
and checkpatch would have complained about the formatting long ago.

Jörn

--
To announce that there must be no criticism of the President, or that we
are to stand by the President, right or wrong, is not only unpatriotic
and servile, but is morally treasonable to the American public.
-- Theodore Roosevelt, Kansas City Star, 1918
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Wednesday, 22 of October 2008, Len Brown wrote:
>
> > > drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used
>
> > >
> > > Shows that this code has an identity crisis due to a maze of #ifdefs, in
> > > the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.
> >
> > This case is invalid, because PM_SLEEP == SUSPEND || HIBERNATION !
>
> I though so too, but 2.6.27 appears to have added XEN to the mix:
>
> config PM_SLEEP
> bool
> depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE

Yeah, which breaks things. :-(

The patch below should fix it and BTW I think ACPI_SLEEP should not depend
on XEN_SAVE_RESTORE anyway, so this is a bug fix really (please consider as
..28 material, probably -stable too).

Thanks,
Rafael

---
From: Rafael J. Wysocki

ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings

Initially CONFIG_PM_SLEEP was defined as
CONFIG_SUSPEND || CONFIG_HIBERNATION and some ACPI code, most
importantly the code in drivers/acpi/main.c, was written with this
assumption. Currently, however, CONFIG_PM_SLEEP is also set when
CONFIG_XEN_SAVE_RESTORE is set.

This causes some compilation warnings to appear in
drivers/acpi/main.c if both CONFIG_SUSPEND and CONFIG_HIBERNATION
are unset and CONFIG_PM_SLEEP is set (this was impossible before).
To fix this problem, redefine CONFIG_ACPI_SLEEP do depend directly
on CONFIG_SUSPEND || CONFIG_HIBERNATION, as originally intended, and
use it instead of CONFIG_PM_SLEEP in drivers/acpi/main.c, wherever
appropriate.

Additionally, move the acpi_target_sleep_state definition from under
the #ifdef to prevent compilation from failing in some cases.

Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/Kconfig | 2 +-
drivers/acpi/sleep/main.c | 7 +++----
2 files changed, 4 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/acpi/Kconfig
================================================== =================
--- linux-2.6.orig/drivers/acpi/Kconfig
+++ linux-2.6/drivers/acpi/Kconfig
@@ -42,7 +42,7 @@ if ACPI

config ACPI_SLEEP
bool
- depends on PM_SLEEP
+ depends on SUSPEND || HIBERNATION
default y

config ACPI_PROCFS
Index: linux-2.6/drivers/acpi/sleep/main.c
================================================== =================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -23,6 +23,7 @@
#include "sleep.h"

u8 sleep_states[ACPI_S_STATE_COUNT];
+static u32 acpi_target_sleep_state = ACPI_STATE_S0;

static int acpi_sleep_prepare(u32 acpi_state)
{
@@ -45,9 +46,7 @@ static int acpi_sleep_prepare(u32 acpi_s
return 0;
}

-#ifdef CONFIG_PM_SLEEP
-static u32 acpi_target_sleep_state = ACPI_STATE_S0;
-
+#ifdef CONFIG_ACPI_SLEEP
/*
* ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
* user to request that behavior by using the 'acpi_old_suspend_ordering'
@@ -132,7 +131,7 @@ static void acpi_pm_end(void)
*/
acpi_target_sleep_state = ACPI_STATE_S0;
}
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_ACPI_SLEEP */

#ifdef CONFIG_SUSPEND
extern void do_suspend_lowlevel(void);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

applied.

thanks,
-Len

On Wed, 22 Oct 2008, Rafael J. Wysocki wrote:

> On Wednesday, 22 of October 2008, Len Brown wrote:
> >
> > > > drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used
> >
> > > >
> > > > Shows that this code has an identity crisis due to a maze of #ifdefs, in
> > > > the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.
> > >
> > > This case is invalid, because PM_SLEEP == SUSPEND || HIBERNATION !
> >
> > I though so too, but 2.6.27 appears to have added XEN to the mix:
> >
> > config PM_SLEEP
> > bool
> > depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
>
> Yeah, which breaks things. :-(
>
> The patch below should fix it and BTW I think ACPI_SLEEP should not depend
> on XEN_SAVE_RESTORE anyway, so this is a bug fix really (please consider as
> .28 material, probably -stable too).
>
> Thanks,
> Rafael
>
> ---
> From: Rafael J. Wysocki
>
> ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings
>
> Initially CONFIG_PM_SLEEP was defined as
> CONFIG_SUSPEND || CONFIG_HIBERNATION and some ACPI code, most
> importantly the code in drivers/acpi/main.c, was written with this
> assumption. Currently, however, CONFIG_PM_SLEEP is also set when
> CONFIG_XEN_SAVE_RESTORE is set.
>
> This causes some compilation warnings to appear in
> drivers/acpi/main.c if both CONFIG_SUSPEND and CONFIG_HIBERNATION
> are unset and CONFIG_PM_SLEEP is set (this was impossible before).
> To fix this problem, redefine CONFIG_ACPI_SLEEP do depend directly
> on CONFIG_SUSPEND || CONFIG_HIBERNATION, as originally intended, and
> use it instead of CONFIG_PM_SLEEP in drivers/acpi/main.c, wherever
> appropriate.
>
> Additionally, move the acpi_target_sleep_state definition from under
> the #ifdef to prevent compilation from failing in some cases.
>
> Signed-off-by: Rafael J. Wysocki
> ---
> drivers/acpi/Kconfig | 2 +-
> drivers/acpi/sleep/main.c | 7 +++----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> Index: linux-2.6/drivers/acpi/Kconfig
> ================================================== =================
> --- linux-2.6.orig/drivers/acpi/Kconfig
> +++ linux-2.6/drivers/acpi/Kconfig
> @@ -42,7 +42,7 @@ if ACPI
>
> config ACPI_SLEEP
> bool
> - depends on PM_SLEEP
> + depends on SUSPEND || HIBERNATION
> default y
>
> config ACPI_PROCFS
> Index: linux-2.6/drivers/acpi/sleep/main.c
> ================================================== =================
> --- linux-2.6.orig/drivers/acpi/sleep/main.c
> +++ linux-2.6/drivers/acpi/sleep/main.c
> @@ -23,6 +23,7 @@
> #include "sleep.h"
>
> u8 sleep_states[ACPI_S_STATE_COUNT];
> +static u32 acpi_target_sleep_state = ACPI_STATE_S0;
>
> static int acpi_sleep_prepare(u32 acpi_state)
> {
> @@ -45,9 +46,7 @@ static int acpi_sleep_prepare(u32 acpi_s
> return 0;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> -static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> -
> +#ifdef CONFIG_ACPI_SLEEP
> /*
> * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
> * user to request that behavior by using the 'acpi_old_suspend_ordering'
> @@ -132,7 +131,7 @@ static void acpi_pm_end(void)
> */
> acpi_target_sleep_state = ACPI_STATE_S0;
> }
> -#endif /* CONFIG_PM_SLEEP */
> +#endif /* CONFIG_ACPI_SLEEP */
>
> #ifdef CONFIG_SUSPEND
> extern void do_suspend_lowlevel(void);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

[announce] new tree: "fix all build warnings, on all configs" - Kernel

Thread: [announce] new tree: "fix all build warnings, on all configs"

LinkBack

Tools

Re: [announce] new tree: "fix all build warnings, on all configs"

[PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs")

Re: [PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs")