Hi Hans, On 14/02/2024 11.54, Hans de Goede wrote: > Hi Alexander, > > On 2/13/24 23:35, Alexander Kobel wrote: >> Hi Hans, all, >> >> after upgrading to 6.7.1 or 6.6.13 (LTS), my Thinkpad X1 Tablet doesn't suspend anymore. Or, rather, it suspends, but wakes again immediately. This happens regardless of whether the keyboard is attached or not, with all ACPI wakeup triggers disabled according to /proc/acpi/wakeup. >> I could identify the following commit as the culprit: >> >> 14c200b7ca46b9a9f4af9e81d258a58274320b6f >> platform/x86: intel-vbtn: Fix missing tablet-mode-switch events >> >> First, it's a suspiciously related patch going into both kernel versions. >> Second, unloading intel-vbtn resolves the problem; machine suspends as usual. >> Third, I tried modifying the patch. Commenting out the newly introduced >> >> /* Some devices need this to report further events */ >> acpi_evaluate_object(handle, "VBDL", NULL, NULL); >> >> resolves the problem on my machine. > > Thank you for reporting this and sorry about the regression. > > Also great that you've already pinpointed which exact patch > and which part of the patch is causing this problem. No worries, thanks for the swift reply. >> I understand that the change was in for a reason, but the deeper meaning of that statement eludes me; is it possible that my model is quirky for that one? >> FWIW: >> - SW_TABLET_MODE is properly updated about half a second after I attach/detach/fold the keyboard during suspend with that statement commented out > > Which input device is generating "SW_TABLET_MODE" events ? Are these > coming from a "Intel Virtual Switches" input device, or are these > generated by some other input device ? > > And I assume that you also have an "Intel Virtual Buttons" > input device ? Correct: % libinput list-devices | grep Intel Device: Intel Virtual Buttons Device: Intel Virtual Switches Device: Intel HID events Device: Intel HID 5 button array >> (but attaching/detaching/folding all wake the device, unless I also `echo Y > /sys/module/acpi/parameters/ec_no_wakeup` - but then I can't wake the tablet at all anymore). > > I assume that this changing the mode wakes up the 2-in-1 is a pre-existing > problem which also happens with older kernels ? Correct. >> - Folding the keyboard to the back of the device disables the keyboard. >> With that statement in (as in 6.7.1 upstream), SW_TABLET_MODE is set to 1 (correct), but reverts to 0 again after about a second (incorrect); the keyboard remains disabled. >> Without the statement, SW_TABLET_MODE remains on 1 until I flip back the keyboard to normal (expected behavior). > > Just to make sure that I understand things correctly, commenting out the new: > > /* Some devices need this to report further events */ > acpi_evaluate_object(handle, "VBDL", NULL, NULL); > > Call not only fixes suspend/resume mode but also fixes SW_TABLET_MODE > reverting to 0 after about 1 second while it should stay 1 ? Correct. N.B.: It still triggers wakeup when I flip or detach the keyboard. In order to prevent that, I have to both unload intel-vbtn entirely *and* disable the XHC wake-up trigger, if I didn't miss anything. > I guess this also answers my question from above if SW_TABLET_MODE > is reported by a "Intel Virtual Switches" device and the answer > to that would be yes, right ? Correct. > I have the feeling that we are just going to need to revert > the addition of this call then IOW your solution of commenting > it out is the right solution. > >> P.S.: I completely failed to find any explanation/definition of "VBDL" or "VGBS". If someone could >> point me to that, I might be able to experiment more on my own. > > The Intel VBTN interface is not documented anywhere AFAIK, so > the below is based on my understanding of things: > > "VBDL" is a method called to let the firmware know that the OS > has a vbtn driver and that it can start delivering events. > I have the feeling that on your X1 Tablet Gen2 it works as > a sort of init() function and it probably causes at least > 1 event (current tablet-mode event?) to be reported which > is causing the wakeup /me thinks. > > "VGBS" is used to ask the firmware for the initial status > (tablet or laptop mode) on initialization. The driver also > uses the presence of the "VGBS" method to determine if > the device supports tablet-mode-sw reporting or not. > > So the idea behind the patch causing your regression was > that maybe on some hw we need to call "VBDL" which enables > event reporting after every event to fix event reporting > sometimes stopping by re-enabling event reporting after > every event. This assumes that calling "VBDL" more then > once does not have any side effects, where in your case > it clearly seems to have several undesirable side-effects. > > So again the fix probably is to simply just drop the > newly added "VBDL" call. Thanks for taking the time for an explanation. Sounds like *not* needing the VBDL call should be default, and that rather the devices that need it are quirky. Although that doesn't mean that the X1 Tablet is fine, because the side effect of immediately resuming also seems rare; otherwise it'd have been spotted and reported earlier by others, I suppose. And, apparently, calling VBDL makes the X1 Tablet misreport the tablet mode state. Hm. Perhaps introducing a special handling of the Inspiron 7352 would be the way to go then? FWIW, I previously dumped a DSDT (for entirely unrelated reasons): https://raw.githubusercontent.com/akobel/linux-thinkpad-x1-tablet/main/x1-tablet-gen2/DSDT/dsl/dsdt.dsl Just in case it's useful in further investigation/comparison to the Dell... > Regards, > > Hans Cheers, Alex
