Port-amd64 archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: New ACPI power resource code for testing



Comments on code:

May I suggest that you change the comment for acpi_power_init() from

        * Must be called before attachment of ACPI device nodes.

to

        * Called from acpi framework before attachment of ACPI
        * device nodes

In acpi_power_register() the DPRINTF(()) hardwires the text "acpi0" in the message; might be better to grab this from the current device's parent? Same for several other DPRINTF(())s...

In acpi_power_switch(), since variable "on" is a boolean, why compare it to another boolean?

        if (on != false) ...
   vs
        if (on) ...

(Similar comment for the call to acpi_power_set() in the patches to acpi_tz.c ...)


Comments on design:

As you've currently set things up, the user MUST include

        acpipower* at acpi?

in their config file if they have acpi at all - even if their system does not have any acpipower resources. (Otherwise, the call to acpi_power_init() will refer to an undefined symbol.) Either this requirement should be removed, or it should be explicitly documented.

(Would it be valid to use RUN_ONCE(9) here? And then call this from the attach routine?)

Unfortunately, even if you resolve the above issue, there's another dependency with acpitz. Is it conceivable that an acpitz can be fully functional without needing the acpipower device? (I'm thinking of passive-only situations.) If so, then the dependency should be removed.




On Mon, 1 Mar 2010, Jukka Ruohonen wrote:

Hi.

I have (re)written the ACPI power resource code.

This tries to overcome some bugs and limitations in the old code. To name
few issues, the old code lacks locking, never removes references to power
resources, does not support retrieving the power state, does not support
some newer control methods, contains some small bugs, among other thing.
(This is not to say that the old code is bad, and Michael Smith's name still
appears in the copyright.) The new code also tries to act more as a
small-scale "framework".

Currently ACPI power resources are used only by acpitz(4). But as power
resources hopefully some day play a part in the overall x86 power management
scheme(s), now would be a good time to consider the issue.

Please test if you have time. How?

        1. Drop the code ("acpi_power.c") to "src/sys/dev/acpi".[1]
        2. Apply a patch ("acpi_power.diff").[2]
        3. Add "acpipower* at acpinodebus?" to your kernel configuration file.

On some laptops acpitz(4) uses these resources to make a switch to "active
cooling", so possible regressions should be visible there (fan does not turn
on/off, etc.). Otherwise the code should just, well, sit and idle.

Comments are welcome too, of course.

- Jukka.

PS. Thanks to Paul Goyette for early review.

[1] ftp://ftp.NetBSD.org/pub/NetBSD/misc/jruoho/acpi_power.c
[2] ftp://ftp.NetBSD.org/pub/NetBSD/misc/jruoho/acpi_power.diff



-------------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:      |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul at whooppee.com   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |                          | pgoyette at netbsd.org  |
-------------------------------------------------------------------------


Home | Main Index | Thread Index | Old Index