Hi,

On 10/25/21 17:12, Mika Westerberg wrote:

On Mon, Oct 25, 2021 at 04:54:41PM +0200, Hans de Goede wrote:

quoted

quoted

Yes that's exactly what is supposed to happen that this attribute is made.
What exactly happens when you write into it?

The _SB.CGWR ACPI method gets called, with arguments coming from ACPI
settings stored in memory. Depending on those settings this function
either directly pokes some MMIO or tries to talk to an I2C GPIO
expander which is not present on the Surface Go, causing it to
MMIO poke an I2C controller which it should not touch.

In either case the AML code ends up poking stuff it should not touch
and the entire force_power sysfs attribute should simply not be
there on devices without thunderbolt.

That's right - it should not be there in the first place if there is no
Thunderbolt controller on that thing.

I guess most of the systems that have this actually do support
Thunderbolt so maybe we can work this around by quirking all the Surface
models in that driver?

I was hoping that we could avoid this, but yes if there is no easy /
clean way to detect if there are any Thunderbolt controllers on the
system then a DMI table is necessary.

My reason for looking into this is because force_power writes
may end up accessing the _SB.GEXP ACPI device which is present in
most DSDTs for devices with recent Intel Core CPUs (this device
seems to represent an I2C attached GPIO expander).

This _SB.GEXP ACPI device claims the MMIO region / PCI BAR of the
I2C4 controller but on the Surface the I2C4 bus is used for the
front camera sensor; and i2cdetect shows there is no GPIO-expander.

So I plan to add an exception for acpi_enforce_resources for the
Surface Go and Surface Go 2 so that Linux can use I2C4. To make sure
this is safe I audited all GEXP accesses in the DSDT.

And depending on the value of the FPAT value from the ACPI config
memory area writing force_power may access the GEXP device, so to
make sure this does not happen; and thus it is safe to add an
override for acpi_enforce_resources, we will need something akin
to a no_thunderbolt_dmi_ids DMI table inside intel-wmi-thunderbolt.

Regards,

Hans