linux-kernel - Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <da7fa0ff-8264-47a4-6d40-1641d315be1c@ideasonboard.com>
Date:   Wed, 14 Oct 2020 12:52:02 +0100
From:   Kieran Bingham <kieran.bingham@...asonboard.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Sasha Levin <sashal@...nel.org>, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Wolfram Sang <wsa@...nel.org>, linux-i2c@...r.kernel.org
Subject: Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call
 i2c_acpi_install_space_handler() before i2c_acpi_register_devices()

Hi Hans,

On 14/10/2020 12:23, Hans de Goede wrote:
> Hi,
> 
> On 10/14/20 1:09 PM, Kieran Bingham wrote:
>> Hi Hans, Sasha,
>>
>> As mentioned on https://github.com/linux-surface/kernel/issues/63, I'm
>> afraid I've bisected a boot time issue on the Microsoft Surface Go 2 to
>> this commit on the stable 5.8 tree.
>>
>> The effect as reported there is that the boot process stalls just after
>> loading the usbhid module.
>>
>> Typing, or interacting with the Keyboard (Type Cover) at that point
>> appears to cause usb bus resets, but I don't know if that's a related
>> symptom or just an effect of some underlying root cause.
>>
>> I have been running a linux-media kernel on this device without issue.
>>
>> Is this commit in 5.9? I'll build a vanilla v5.9 kernel and see if it
>> occurs there too.
> 
> Yes the commit is in 5.9 too. Still would be interesting to see if 5.9 hits
> this issue too. I guess it will, but as I mentioned in:

Indeed, I've just tested vanilla 5.9 on a Surface Go 2 and that's broken
I'm afraid.

Back traces on 5.9 [0] show hung tasks on both the i915_driver_probe,
and the tps68470_gpio_probe. I suspect the i915 failure of course is the
reason a desktop environment can't be reached, however I was able to ssh
in at least ;-)

[0] https://paste.debian.net/1167081/


Given that this is now a mainline bug, I'll reply to the mainline patch
and suggest a revert there, rather than here on the stable patches.

As for fixing it ... I don't have enough visibility of the problem for
both this, and the issues that were fixed by this patch.

Let me know if there's anything you'd like me to investigate further, or
dig into specifically.

--
Kieran


> https://github.com/linux-surface/kernel/issues/63
> 
> I do not understand why this commit is causing this issue.
> 
> So I just checked and the whole acpidump is not using I2C
> opregion stuff at all:
> 
> [hans@x1 microsoft-surface-go2]$ ack GenericSerialBus *.dsl
> [hans@x1 microsoft-surface-go2]$
> 
> And there is only 1 _REG handler which is for the
> embedded-controller.
> 
> So this patch should not make a difference at all on the GO2,
> other then maybe a subtle timing difference somewhere ... ?
> 
> Regards,
> 
> Hans
> 
> 
>> On 21/09/2020 15:40, Sasha Levin wrote:
>>> From: Hans de Goede <hdegoede@...hat.com>
>>>
>>> [ Upstream commit 21653a4181ff292480599dad996a2b759ccf050f ]
>>>
>>> Some ACPI i2c-devices _STA method (which is used to detect if the device
>>> is present) use autodetection code which probes which device is present
>>> over i2c. This requires the I2C ACPI OpRegion handler to be registered
>>> before we enumerate i2c-clients under the i2c-adapter.
>>>
>>> This fixes the i2c touchpad on the Lenovo ThinkBook 14-IIL and
>>> ThinkBook 15 IIL not getting an i2c-client instantiated and thus not
>>> working.
>>>
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1842039
>>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>>> Reviewed-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
>>> Signed-off-by: Wolfram Sang <wsa@...nel.org>
>>> Signed-off-by: Sasha Levin <sashal@...nel.org>
>>> ---
>>>   drivers/i2c/i2c-core-base.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>>> index 4f09d4c318287..7031393c74806 100644
>>> --- a/drivers/i2c/i2c-core-base.c
>>> +++ b/drivers/i2c/i2c-core-base.c
>>> @@ -1336,8 +1336,8 @@ static int i2c_register_adapter(struct
>>> i2c_adapter *adap)
>>>         /* create pre-declared device nodes */
>>>       of_i2c_register_devices(adap);
>>> -    i2c_acpi_register_devices(adap);
>>>       i2c_acpi_install_space_handler(adap);
>>> +    i2c_acpi_register_devices(adap);
>>>         if (adap->nr < __i2c_first_dynamic_bus_num)
>>>           i2c_scan_static_board_info(adap);
>>>
>>
>>
> 

-- 
Regards
--
Kieran