lkml.org 
[lkml]   [2011]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/5] ACPI / PM: Move references to pm_flags into sleep.c
    On Tue, Feb 8, 2011 at 1:20 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
    >
    > If direct references to pm_flags are moved from bus.c to sleep.c,
    > CONFIG_ACPI will not need to depend on CONFIG_PM any more.

    The patch may _work_, but I really hate it. That function naming is insane:

    >  #ifdef CONFIG_ACPI_SLEEP
    >  #else
    > +static inline bool acpi_pm_enabled(void) { return true; }

    acpi_pm_enabled() returns true if ACPI_SLEEP is _not_ enabled? That's
    just crazy.

    ... followed by more crazy:

    > +bool acpi_pm_enabled(void)
    > +{
    > +       if (!(pm_flags & PM_APM)) {
    > +               pm_flags |= PM_ACPI;
    > +               return true;
    > +       }
    > +       return false;
    > +}

    IOW, that function doesn't do anything _remotely_ like what the naming
    indicates.

    Any sane person would expect that a function called
    'acpi_pm_enabled()' would return true if ACPI PM was enabled, and
    false otherwise. But it's not what it does at all. Instead, what it
    does is to say "if APM isn't enabled, let's enable ACPI and return
    true". Except then for the non-ACPI sleep case, we just return true
    regardless, which still looks damn odd, wouldn't you say?

    That isn't good. Maybe it all does what you want it to do, but from a
    code readability standpoint, it's just one honking big "WTF?". Please
    don't do that. Call it something else. Make the naming actually follow
    what the semantics are. Appropriate naming should also make it
    sensible to return "true" when ACPI_SLEEP is disabled, and should make
    the caller look sane.

    Now, I don't know what that particular naming might be, but maybe it
    would be about APM being enabled. Which is what the caller actually
    seems to care about and talks about for the failure case. Maybe you
    need separate functions for the "is APM enabled" case for the naming
    to make sense. Hmm?

    Linus
    --
    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/

    \
     
     \ /
      Last update: 2011-02-09 00:43    [W:0.024 / U:30.104 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site