Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Hans de Goede]

Hi All,

While digging through Microsoft Surface Go ACPI tables to fix an unrelated
issue, I noticed that there is an intel-wmi-thunderbolt WMI device in the
ACPI tables and the intel-wmi-thunderbolt driver happily binds to this.
This is likely the result of copy paste programming of the ACPI tables.

This causes a /sys/bus/wmi/devices/.../force_power attribute to be created
and echoing to that executes ACPI code which ends up poking at things it
should not be poking at on the Surface Go.

The problem of having these "nonsense" WMI devices with the
intel-wmi-thunderbolt GUID is likely more wide-spread and ideally the
intel-wmi-thunderbolt would ignore these.

This makes me wonder if there is a way to see if there are any thunderbolt
controllers on the system at all ? (with as goal to make intel-wmi-thunderbolt
not bind if there are none)

Regards,

Hans

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Oliver Neukum]

On 25.10.21 15:48, Hans de Goede wrote:
> This makes me wonder if there is a way to see if there are any thunderbolt
> controllers on the system at all ? (with as goal to make intel-wmi-thunderbolt
> not bind if there are none)

Hi,

as far as I understand the driver and device exist precisely because the
thunderbolt does not exist on the bus will unused and will virtually be
hotplugged together with a device.

    Regards
        Oliver

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Hans de Goede]

Hi,

On 10/25/21 16:36, Mario Limonciello wrote:
> Surface Laptop go is this right?
> New Lightweight Surface Laptop Go – The Everyday, Everywhere Laptop – Microsoft Surface <https://www.microsoft.com/en-us/d/surface-laptop-go/94fc0bdgq7wv?activetab=pivot:techspecstab>
> IOW: "10th Gen Intel® Core™ i5 processor – 1035G1".
> 
> That should be Ice Lake according to ARK:
> Intel Core i51035G1 Processor 6M Cache up to 3.60 GHz Product Specifications <https://ark.intel.com/content/www/us/en/ark/products/196603/intel-core-i51035g1-processor-6m-cache-up-to-3-60-ghz.html>
> 
> ICL should have integrated TBT3.  The concept of the force power WMI attribute makes "most" sense when it comes to a GPIO getting toggled.

I'm not talking about the Surface Laptop Go, but about the "Surface Go"
which uses the classic Surface tablet with kickstand form-factor with
the following CPU: Intel(R) Pentium(R) CPU 4415Y

The model definitely does not have Thunderbolt.

>>This causes a /sys/bus/wmi/devices/.../force_power attribute to be created
> and echoing to that executes ACPI code which ends up poking at things it
> should not be poking at on the Surface Go.
> 
> 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.

Regards,

Hans



> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Hans de Goede <hdegoede@redhat.com>
> *Sent:* Monday, October 25, 2021 8:48
> *To:* Mika Westerberg <mika.westerberg@linux.intel.com>; Andreas Noever <andreas.noever@gmail.com>; Michael Jamet <michael.jamet@intel.com>; Yehezkel Bernat <YehezkelShB@gmail.com>; Mario Limonciello <mario.limonciello@outlook.com>
> *Cc:* linux-usb <linux-usb@vger.kernel.org>
> *Subject:* Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt
>  
> Hi All,
> 
> While digging through Microsoft Surface Go ACPI tables to fix an unrelated
> issue, I noticed that there is an intel-wmi-thunderbolt WMI device in the
> ACPI tables and the intel-wmi-thunderbolt driver happily binds to this.
> This is likely the result of copy paste programming of the ACPI tables.
> 
> This causes a /sys/bus/wmi/devices/.../force_power attribute to be created
> and echoing to that executes ACPI code which ends up poking at things it
> should not be poking at on the Surface Go.
> 
> The problem of having these "nonsense" WMI devices with the
> intel-wmi-thunderbolt GUID is likely more wide-spread and ideally the
> intel-wmi-thunderbolt would ignore these.
> 
> This makes me wonder if there is a way to see if there are any thunderbolt
> controllers on the system at all ? (with as goal to make intel-wmi-thunderbolt
> not bind if there are none)
> 
> Regards,
> 
> Hans
> 
> 

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Mika Westerberg]

On Mon, Oct 25, 2021 at 04:54:41PM +0200, Hans de Goede wrote:
> > 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?

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Hans de Goede]

Hi,

On 10/25/21 17:12, Mika Westerberg wrote:
> On Mon, Oct 25, 2021 at 04:54:41PM +0200, Hans de Goede wrote:
>>> 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

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Mika Westerberg]

Hi,

On Tue, Oct 26, 2021 at 10:17:53AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/25/21 17:12, Mika Westerberg wrote:
> > On Mon, Oct 25, 2021 at 04:54:41PM +0200, Hans de Goede wrote:
> >>> 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.

Well, the force power thing is there just for this reason. It should
only be present on systems using ACPI assisted PCIe hotplug for
Thunderbolt devices. Apparantly some BIOS engineer forgot to remove it
on Surface :( I need to check if it is present on recent reference
BIOSes too. If it is then I'll report an internal sighting about this to
get it removed.

In theory we could also use a heuristic that if there is a TBT
controller present when the driver probes it should fail the probe or
so. Or even look for the PCI host bridge and if it got the PCIe hotplug
capability from the BIOS (through _OSC negotiation) we can assume this
system does not need the force power.

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Hans de Goede]

Hi,

On 10/26/21 10:53, Mika Westerberg wrote:
> Hi,
> 
> On Tue, Oct 26, 2021 at 10:17:53AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 10/25/21 17:12, Mika Westerberg wrote:
>>> On Mon, Oct 25, 2021 at 04:54:41PM +0200, Hans de Goede wrote:
>>>>> 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.
> 
> Well, the force power thing is there just for this reason. It should
> only be present on systems using ACPI assisted PCIe hotplug for
> Thunderbolt devices. Apparantly some BIOS engineer forgot to remove it
> on Surface :( I need to check if it is present on recent reference
> BIOSes too. If it is then I'll report an internal sighting about this to
> get it removed.
> 
> In theory we could also use a heuristic that if there is a TBT
> controller present when the driver probes it should fail the probe or
> so. Or even look for the PCI host bridge and if it got the PCIe hotplug
> capability from the BIOS (through _OSC negotiation) we can assume this
> system does not need the force power.

I think adding such heuristics might be a good thing to do, because
I suspect that this problem is much wider then just a couple of
surface devices.

One worry I have about this is probe ordering. We cannot assume the
entire PCI bus has been enumerated when the intel-wmi-thunderbolt's
probe() method runs. So that would mean doing something like
returning -EPROBE_DEFER if no thunderbolt controller is found and
then say 1 minute after boot return -ENODEV to get us of the
probe_deferal devices list...

IOW this is going to be ugly so for now I think a DMI list for the
devices where I want to make sure force_power does not poke the
GEXP device is best.

Regards,

Hans

Re: Disabling intel-wmi-thunderbolt on devices without Thunderbolt / detecting if a device has Thunderbolt

[Mika Westerberg]

On Tue, Oct 26, 2021 at 12:34:33PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/26/21 10:53, Mika Westerberg wrote:
> > Hi,
> > 
> > On Tue, Oct 26, 2021 at 10:17:53AM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 10/25/21 17:12, Mika Westerberg wrote:
> >>> On Mon, Oct 25, 2021 at 04:54:41PM +0200, Hans de Goede wrote:
> >>>>> 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.
> > 
> > Well, the force power thing is there just for this reason. It should
> > only be present on systems using ACPI assisted PCIe hotplug for
> > Thunderbolt devices. Apparantly some BIOS engineer forgot to remove it
> > on Surface :( I need to check if it is present on recent reference
> > BIOSes too. If it is then I'll report an internal sighting about this to
> > get it removed.
> > 
> > In theory we could also use a heuristic that if there is a TBT
> > controller present when the driver probes it should fail the probe or
> > so. Or even look for the PCI host bridge and if it got the PCIe hotplug
> > capability from the BIOS (through _OSC negotiation) we can assume this
> > system does not need the force power.
> 
> I think adding such heuristics might be a good thing to do, because
> I suspect that this problem is much wider then just a couple of
> surface devices.
> 
> One worry I have about this is probe ordering. We cannot assume the
> entire PCI bus has been enumerated when the intel-wmi-thunderbolt's
> probe() method runs. So that would mean doing something like
> returning -EPROBE_DEFER if no thunderbolt controller is found and
> then say 1 minute after boot return -ENODEV to get us of the
> probe_deferal devices list...

The whole PCI bus does not need to be enumerated - just the host bridge
which is typically pretty early.

> IOW this is going to be ugly so for now I think a DMI list for the
> devices where I want to make sure force_power does not poke the
> GEXP device is best.

I agree. We can look for the other option later if more devices with
this issue are found.