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

This is a discussion on [announce] new tree: "fix all build warnings, on all configs" - Kernel ; 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 ...

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2
Results 21 to 23 of 23

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

  1. Re: [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/

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

    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/

  3. 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")

    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
    >



+ Reply to Thread
Page 2 of 2 FirstFirst 1 2