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. > 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 ? > (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 ? > - 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 ? 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 ? 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. Regards, Hans
