Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 66373 Build 63256: arc lint + arc unit
Event Timeline
| sys/dev/acpica/acpi.c | ||
|---|---|---|
| 174 | Indentation looks wonky here. I think the prevailing style is to tab until aligned with the function name, then add four spaces, so it'd be nice to match. | |
| 599 | Aren't you effectively breaking compatibility for these sysctls? After this change they report the generic power_stype names instead of ACPI sleep state names. | |
| sys/dev/acpica/acpi.c | ||
|---|---|---|
| 599 | Yeah I am. This is sort of the main reason why I'm unsure of these changes. We want to be able to tell OSPM to enter a generic "suspend" sleep type instead of an ACPI S-state upon pressing the power button, because this could mean either entering S3 or S0ix (which is not an ACPI state). | |
| 610 | TODO gotta fix this description | |
| sys/dev/acpica/acpi.c | ||
|---|---|---|
| 116 | Why are these not both bool or BOOLEAN? | |
| 599 | I think these sysctls are probably too widely used for such breakage to be a good idea. There are quite a few references to these sysctls on various forums. They're also documented in acpi.4, which needs to be updated here. Since they're taking a string as input, can we instead have sysctl handlers which still accept the old sleep state names as well as the new ones? And perhaps print a warning to the console if one is using the legacy names. | |
| sys/dev/acpica/acpi.c | ||
|---|---|---|
| 116 | oops, I'll make both bools | |
| 599 |
That sounds like a good solution. It still would be a breaking change if e.g. you were to attempt to set it to S1 and standby_state is set to S2, but that's probably not a big deal. | |
Use own sysctl handler for ACPI stuff so we can still support old S-states here (even though deprecated).
This looks ok to me. It would be best to land this with a man page update, whether that's here or in a separate commit.
| sys/dev/acpica/acpi.c | ||
|---|---|---|
| 4386 | Do you plan to update acpi.4 in this patch? | |