Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()
Hi all, On 10/14/20 1:23 PM, 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: 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 ... ? Thanks to Maximilian Luz sharp eyes this is explained now, despite the name of the i2c_acpi_install_space_handler() it also had a acpi_walk_dep_device_list() call hidden in there, so the "i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()" also moved that acpi_walk_dep_device_list() earlier. I've given Kieran a patch to test which in essence reverts the part where the acpi_walk_dep_device_list() call is also moved earlier and that fixes the Surface Go 2 not booting. I will submit this fix upstream right away and I'll also send a separate mail to Greg / stable@vger to see if Greg is willing to make an exception and at this to the stable series before it hits Linus' tree. Regards, Hans
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 >>> >>> [ 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 >>> Reviewed-by: Mika Westerberg >>> Signed-off-by: Wolfram Sang >>> Signed-off-by: Sasha Levin >>> --- >>> 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
Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()
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: 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 [ 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 Reviewed-by: Mika Westerberg Signed-off-by: Wolfram Sang Signed-off-by: Sasha Levin --- 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);
Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()
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. -- Regards Kieran On 21/09/2020 15:40, Sasha Levin wrote: > From: Hans de Goede > > [ 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 > Reviewed-by: Mika Westerberg > Signed-off-by: Wolfram Sang > Signed-off-by: Sasha Levin > --- > 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
[PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()
From: Hans de Goede [ 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 Reviewed-by: Mika Westerberg Signed-off-by: Wolfram Sang Signed-off-by: Sasha Levin --- 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); -- 2.25.1