acpi_ibm: add support for ThinkPad PrivacyGuard

[kamila_ksp.sk]

Summary

ThinkPad PrivacyGuard is a built-in toggleable privacy filter that restricts
viewing angles when on. It is an available on some new ThinkPad models such as
the X1 Carbon 7th gen (as an optional HW upgrade).
The privacy filter can be enabled/disabled via an ACPI call. This commit adds a
sysctl under dev.acpi_ibm that allows for getting and setting the PrivacyGuard
state.

Test Plan

Build and load the acpi_ibm module. There are 3 interesting cases:

1. If you aren't running a laptop with PrivacyGuard, this should have no effect.
2. If you have a laptop that has the option of PrivacyGuard but the hardware is not installed, this should have no effect.
3. If you have the PrivacyGuard HW, you should see a new sysctl (behaviour described below).

I have tested cases 1 and 3, but so far I have not been able to get access to
hardware for case 2. I attempt to detect this in the code though, and if it is
not correct, the worst thing that can happen is (hopefully) to end up with a
sysctl that does not do anything.

If you do have the hardware, this change will add the sysctl
dev.acpi_ibm.0.privacyguard, which can have values 0 or 1. This sysctl
correctly reflects the current state of the privacy filter: it is 1 iff your
neighbor cannot read this. (This is read from ACPI on every access, not stored
in a local variable. Thus, pressing the "PrivacyGuard toggle" hotkey, Fn+D on my
laptop, does the right thing.) If you write a 0 or 1 into the sysctl, the privacy
filter will be turned off or on respectively. Writing any other value returns
EINVAL.

I've tested this on my laptop (X1 Carbon 7th gen) on CURRENT and everything
seems to work as described.

[kamila_ksp.sk]

whitespace fixes

[kamila_ksp.sk]

I can't phabricator -_-

[cem]

LGTM modulo whitespace nit and ACPI_STATUS return to userspace in set.

sys/dev/acpi_support/acpi_ibm.c

1036–1038 ↗

(On Diff #67321)

Could be reduced to return (acpi_ibm_privacyguard_get() != -1); for conventional definitions of true and false. Not a big deal.

1266–1267 ↗

(On Diff #67321)

Inconsistent whitespace; these should either all be aligned, or none aligned.

1270–1271 ↗

(On Diff #67321)

Does the Get method ("GSSS"?) really take an argument? Do you have an ACPI DSDT dump of the two methods you can share?

1308 ↗

(On Diff #67321)

This just returns an ACPI_STATUS out to the sysctl layer, which directly returns the value out to userspace as far as I can tell. Something should translate it to an errno value instead; probably this function.

[kamila_ksp.sk]

@cem, thank you for the review! I don't really know anything about ACPI, so I really appreciate your useful comments.

sys/dev/acpi_support/acpi_ibm.c

1036–1038 ↗

(On Diff #67321)

I realised this, but there is code that does it like this above. But I agree that the shorter one is actually clearer, so I'll do that.

1270–1271 ↗

(On Diff #67321)

I'm not sure how to make a dump (I don't think the method shows up in the output of acpidump -dt...), but if I call it without an argument, I get a "warning: expected 1 argument, got 0" in dmesg (and it doesn't work). This way there is no warning, and the output does correspond to whether the privacyguard is on.

1308 ↗

(On Diff #67321)

Now that you mention it, that does seem to be true. However, all the other set functions in this file return the ACPI status, so that's what I did here too. Is that a "bad thing" that should be avoided in new code? If so, what error code should I return? I think that the ACPI call can only fail if the device isn't present, so would ENXIO or ENODEV be appropriate?

[kamila_ksp.sk]

• acpi_ibm: PrivacyGuard whitespace fixes

[kamila_ksp.sk]

• acpi_ibm: PrivacyGuard whitespace fixes

[kamila_ksp.sk]

• acpi_ibm: PrivacyGuard whitespace fixes

[cem]

Thanks, the updates look great. I'm still curious about the disassembly of the methods if you can get it, but I don't think that's a blocker issue.

sys/dev/acpi_support/acpi_ibm.c

1270–1271 ↗

(On Diff #67321)

It should show up in acpidump -dt. I'm not super familiar with how the tool works and how compiled ACPI methods are referenced in firmware, so it's possible I'm mistaken and acpidump won't be able to find it. My hypothesis is that the operating system needs to be able to enumerate it to invoke it, so if acpidump enumerates the same stuff, it should find it. Supposedly acpidump will dump both the primary DSDT as well as any SSDTs (secondary/additional DSDTs, essentially), per the manual page.

It does sound like the method is marked as taking one argument (i.e., Method (GSSS, 1, ...) { }), which is surprising to me. It might be interesting to see what the method does with the argument (Arg0), if anything.

This is mostly for my curiosity — clearly we should continue passing a single argument, and if zero works for the functionality, we should continue passing zero. It's hard to imagine the firmware doing something completely catastrophic when a Get function is passed zero, but who knows. My best guess is that the Method just ignores Arg0.

[cem]

Oh whoops I forgot about the error handling issue.

[cem]

sys/dev/acpi_support/acpi_ibm.c

1308 ↗

(On Diff #67321)

Yeah, I'd say it's a bad thing. E.g., here are the first few ACPI statuses and the errnos they line up with:

• AE_ERROR :: EPERM
• AE_NO_ACPI_TABLES :: ENOENT
• AE_NO_NAMESPACE :: ESRCH
• AE_NO_MEMORY :: EINTR
• AE_NOT_FOUND :: EIO
• AE_NOT_EXIST :: ENXIO
• AE_ALREADY_EXISTS :: E2BIG

Some line up ok, but it's clearly accidental rather intentional, and the lineup probably gets worse as we go down the list into more specific codes.

I think the appropriate long-term fix is likely to be:

• Modify the various routines that return an ACPI status to have type ACPI_STATUS rather than int
• Create (I can't believe this doesn't exist already) some kind of ACPI_STATUS to errno subroutine that can be shared with other code
• Convert ACPI_STATUS to errno in the caller, acpi_ibm_sysctl_set() (this could be a shared acpi_error_out: label / return)

But I don't think we should do that in this change; it's a preexisting issue.

Here's what I'd like for this revision:

• acpi_ibm_privacyguard_set returns ACPI_STATUS,
• case ACPI_IBM_METHOD_PRIVACYGUARD: in acpi_ibm_sysctl_set returns something like: return (ACPI_SUCCESS(status) ? 0 : ENODEV);

That way, this change does the right thing, but we don't have to rototill the entire file. Does that sound ok to you?

[kamila_ksp.sk]

sys/dev/acpi_support/acpi_ibm.c

1270–1271 ↗

(On Diff #67321)

I put the output of sudo acpidump -dt to https://kamila.is/coding/acpidump-x1-carbon-7th-gen-2020-01-26.txt. I'm not sure what exactly I'm looking for, but just searching for "GSSS" or "SSSS" does not show anything.

I do agree it is interesting (and it took me a while to figure this out), and I would really like to know where to find this information. Maybe someone else has an idea?

[kamila_ksp.sk]

sys/dev/acpi_support/acpi_ibm.c

1308 ↗

(On Diff #67321)

What you wrote makes a lot of sense, I've been thinking along similar lines. Thank you for writing this up, that makes it easier for someone to actually do it.

I have the feeling that the ACPI code could use some serious refactoring, but of course that's a difficult thing to do, as ideally such things would be tested on the affected hardware, and that's hard. Something to think about in the shower.

For this, I have done what you suggested.

[kamila_ksp.sk]

• acpi_ibm: PrivacyGuard: return a sensible error code

[cem]

Thanks! LGTM other than the remaining EINVAL <-> ACPI_STATUS conflict in the revision.

sys/dev/acpi_support/acpi_ibm.c

1305 ↗

(On Diff #67335)

This should be a valid ACPI_STATUS instead of EINVAL. AE_BAD_PARAMETER looks appropriate, maybe?

1270–1271 ↗

(On Diff #67321)

It looks like acpidump fails to parse your firmware and doesn't do any decompilation at all. Here's an example dump from a Bhyve guest, which contains a relatively small firmware compared to real devices: https://people.freebsd.org/~cem/bhyve-acpidump-dt-ex.txt

Note the bit starting at "AML/ASL+ Disassembler version 20191018 (64-bit version)"; that's where you'd expect to see GSSS and SSSS, as Method definitions.

[kamila_ksp.sk]

sys/dev/acpi_support/acpi_ibm.c

1305 ↗

(On Diff #67335)

Argh, sorry, missed that. But actually, this breaks the functionality: I want to return EINVAL to userspace when given an argument that isn't 0 or 1. I've opted for a "minimal diff" fix here for now: I duplicated the the "0 or 1" check in the acpi_ibm_sysctl_set call, in order to return EINVAL there.

(Another option would be to rever the int -> ACPI_STATUS change and completely ignore that problem for now. I'm not sure which one would be preferable.)

Given that this is kind of annoying, I think I will try making a patch for the "ACPI_STATUS to errno" conversion thing ASAP. But I think you'll agree that putting that into the same commit would not be great, so consider this a temporary fix (I hope this won't go the way temporary fixes usually go :-) )

1270–1271 ↗

(On Diff #67321)

That makes sense, thank you for the explanation. (As mentioned, I have no idea about ACPI...)

Why does it fail though? (if you're willing to spend time explaning...) Is it because the firmware contains something that acpidump doesn't recognise? What could that be?

[kamila_ksp.sk]

• acpi_ibm: PrivacyGuard: workaround ACPI_STATUS return type

[cem]

sys/dev/acpi_support/acpi_ibm.c

1305 ↗

(On Diff #67335)

Ah, right. Another option would be to just add a tiny version of the error conversion routine in sysctl_set; something like:

switch (acpi_ibm_privacyguard_set()) {
case AE_OK:
    return (0);
case AE_BAD_PARAMETER:
    return (EINVAL);
default:
    return (ENODEV);
}

The 2nd patch would just pull that out into a function somewhere more central and add other cases. I don't know if it makes more sense to convert callers in the 2nd patch or save it for a 3rd; either seems ok.

1270–1271 ↗

(On Diff #67321)

No problem! I'm pretty new to ACPI myself.

Why does it fail? My guess is that the last message is the "fatal" problem that causes acpidump to give up: "Firmware Error (ACPI): Failure creating named object [\_SB.PCI0.RP01.PXSX._DSM], AE_ALREADY_EXISTS (20200110/dswload-496)." I don't think the unresolved symbol errors are fatal, because my PC's firmware has 7 unresolved methods and acpidump handles it ok (it at least produces useful output).

Is that object actually a duplicate? Does it matter for decompilation? I don't really know. I think acpidump should probably print what it can as gracefully as possible rather than giving up in this scenario.

[cem]

sys/dev/acpi_support/acpi_ibm.c

902 ↗

(On Diff #67409)

style(9): return (EINVAL);

[kamila_ksp.sk]

• acpi_ibm: convert ACPI_STATUS exit code to errno for PrivacyGuard

[kamila_ksp.sk]

sys/dev/acpi_support/acpi_ibm.c

1305 ↗

(On Diff #67335)

Actually I think just doing a poor man's version of this now is the best option. Then the next patch would move that routine somewhere more central and do it for the whole file. See updated patch.

[cem]

LGTM other than the indentation

sys/dev/acpi_support/acpi_ibm.c

374–376 ↗

(On Diff #67444)

the indentation here looks wrong

[kamila_ksp.sk]

• acpi_ibm: another whitespace fix in my new code

[kamila_ksp.sk]

Hopefully it's OK now. If it is, can someone please commit it?

@cem: Thanks a ton for your help! Once this is in, I will get started on fixing the ACPI_STATUS -> errno thing for the whole file.

sys/dev/acpi_support/acpi_ibm.c

374–376 ↗

(On Diff #67444)

Sorry, I promise to set up clang-format for my next patch :-/

[philip]

Just doing a quick final test-build and I'll commit this.

Thank you!

[cem]

Updated patch LGTM :-). I see philip has already committed it, great!

sys/dev/acpi_support/acpi_ibm.c

374–376 ↗

(On Diff #67444)

No worries. Sadly, clang-format doesn't actually grok our style especially well.