Bug 44161 addressed a problem relating to some Samsung laptops. This was resolved with a patch to drivers/acpi/ec.c in version 4.0 or thereabouts. However, this code added by this patch was removed from ec.c in 4.11, and the problem has returned, exactly as described in 44161. There is nothing more than I can add that was not covered in detain in 44161.
I don't know if it is actually solved in version 4.0 as not all of the commits have been upstreamed. Now could you give the following 3 patches a try: https://patchwork.kernel.org/patch/9931477/ https://patchwork.kernel.org/patch/9931481/ https://patchwork.kernel.org/patch/9931473/ Thanks Lv
And also tell me if applying only this patch could fully solve the issue: https://patchwork.kernel.org/patch/9931477/
> Bug 44161 addressed a problem relating to some Samsung laptops. This was > resolved with a patch to drivers/acpi/ec.c in version 4.0 or thereabouts. If you mean the CLEAR_ON_RESUME quirk? > However, this code added by this patch was removed from ec.c in 4.11, and the > problem has returned, exactly as described in 44161. Yes, it is removed because we are using 2 mechansims to solve the same issue: 1. Enable IRQ after noirq stage, if the GPE silicon is good, the event will be triggered again. 2. Polling EC GPE once after enabling the event. However, I did this in a safer way, only poll GPEs when an event is already noticed. It should work for most of the platforms requiring the quirk. https://patchwork.kernel.org/patch/9931477/ Just stop doing things in a safer way, but doing things aggressively. Thanks Lv
IMO, the quirk introduced in bug 44161 just reduces the reproduce ratio of the problem. The final solution in that bug is actually not upstreamed: https://bugzilla.kernel.org/attachment.cgi?id=155851 So comes out the approaches mentioned in comment 3. The approaches are closer to original quirk and safer for other platforms. The following 2 patches are the new form of the final solution: https://patchwork.kernel.org/patch/9931481/ https://patchwork.kernel.org/patch/9931473/ Thanks Lv
Ping... What's the blocking issue for the test? Thanks Lv
Created attachment 258389 [details] attachment-32255-0.html Do you want *me* to do this? I only reported the bug as an and user. I actually had a few lines of my code included in the kernel going back to about 2002, but I have not compiled a kernel for about a decade. I tried to apply the patches into the current Fedora kernel source (because my Samsung machine has Fedora installed) but they failed.n If no-one else can do it, I will try to figure out how to build and install a patched kernel, but it will take me a while. Problem is, the only affected machine is in constant use, and has a work-around using the program mentioned before and a hacked systemd script. On 11 September 2017 at 09:34, <bugzilla-daemon@bugzilla.kernel.org> wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=196833 > > --- Comment #5 from Lv Zheng (lv.zheng@intel.com) --- > Ping... > What's the blocking issue for the test? > > Thanks > Lv > > -- > You are receiving this mail because: > You reported the bug.
You should know, if you want the bug to be fixed, you need to provide validation support here. It's very simple to build and test a kernel. $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git $ apply the patches $ cd linux $ cp /boot/config-xxx (your current config) ./.config Then you can type "make help" to see the build targets for you. Simply you can: $ git make oldconfig $ git make -j64 $ sudo make modules_install install Reboot the kernel, you'll see the new kernel in your grub2 menu.
NOTE you are reporting a regression for a removed quirk as we have already root caused the regression and is able to provide a final solution as a replacement of the quirk (though may still not perfect). So it is not good to restore the quirk back and hide the root cause again. Please help to complete the validations. Thanks Lv
OK. Sorry for being stupid about this. I have the Samsung machine in my possession now for "maintenance" and will be trying to test the fixes in the next couple of days. Alistair
Great. Thanks for the help. :)
Let me comment more A. To correct my mistake: Sorry, I have a mistake: > $ git make oldconfig > $ git make -j64 > $ sudo make modules_install install should be: $ make oldconfig $ make -j64 $ sudo make modules_install install B. To know the replicdation ration of the issue: IMO, you should have very low reproduce ratio for the regression. As the following conditions should be met: 1. An EC events (causing SCI_EVT flagged) occurred between acpi_ec_unblock_transaction() and acpi_ec_resume(); 2. No EC transactions occurred between acpi_ec_unblock_transaction() and acpi_ec_resume(); 3. Samsung platforms. Only in this case, our original root cause fix is not sufficient to handle what was dealt by the removed quirk. And it's a very short period between acpi_ec_unblock_transaction() and acpi_ec_resume(), probably only several msecs. So let me ask: Could you let me know the reproduce ratio of the issue? B. To capture some bug related information: If you are not able to rebuild the kernel right now, then you may still able to provide useful information for us. If you have CONFIG_DYNAMIC_DEBUG enabled for your current running kernel, you still can help to offer some information. So Could help me to capture what's going on when the bug occurs using the following steps. 1. Boot the kernel with "dyndbg='file drivers/acpi/ec.c +p'" (I hope you needn't rebuild the kernel with CONFIG_DYNAMIC_DEBUG=y); 2. Perform a suspend/resume and confirm the bug is triggered; 3. Upload the dmesg output here. If you cannot do 2 due to lacking of CONFIG_DYNAMIC_DEBUG=y, you still need to learn kernel build, and stay in the kernel build directory: $ make menuconfig Find DYNAMIC_DEBUG, select it (you can type "\" to enter a search CUI to find its location), and exit the menuconfig CUI. $ make -j64 $ sudo make modules_install install D. To know if the better fix is working for you: attachment 258293 [details] should be a complete replacement of the removed quirk. If you can apply this fix on top of your kernel, rebuild the kernel and try it. You can help me to confirm if the bug is fully fixed by the attachment 258293 [details]. Thanks and best regards Lv
Created attachment 258575 [details] attachment-2201-0.html Hi, Lv I was just about to respond to you. I wanted to give the machine a good test before posting again. I compiled and installed the kernel with the three patches linked in comment 1 (9931477, 9931481, 9931473) <https://patchwork.kernel.org/patch/9931477/> a few days ago. I figured out the correct make and install sequence :-) (This takes me back years - very nostalgic - except that I could compile now using 8 cores rather than one or two, when I last did it!) Is 258293 the same as these three? [alistair@juno ~]$ uname -a Linux juno.ty-eurgain 4.14.0-rc1+ #1 SMP Wed Sep 20 00:00:48 BST 2017 x86_64 x86_64 x86_64 GNU/Linux I have suspended and resumed the machine many times (>30) and booted into Windows 10 and back again. I made sure that lots of PM events would occur when the machine was suspended (lid open/close, power on/off, battery discharge) which seems to be the trigger for the problem. There has been no instance of any problem with suspend or resume on either OS. I can also confirm that I have removed the user-space workaround that I had implemented. I also rebooted into the old Fedora kernel (with no userspace workaround) to confirm that the problem re-appeared. So it seems that this good behaviour is all down to the new kernel. I have not noticed any other problems arising from these changes. Note that this machine (Samsung Series 9 NP900X3B) has never automatically resumed on lid-open other than with the original Win7 factory install - the power button always has to be pressed. (Not just Linux - it failed to resume with a Win 7 re-install and with Win 10.) So, from my point of view, the bug is solved. Is there any other sort of testing that I can do or are there any logs that you would like me to post? Thanks for your help and thanks for getting me back into looking into the kernel again after so many years away! Alistair On 20 September 2017 at 02:02, <bugzilla-daemon@bugzilla.kernel.org> wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=196833 > > Lv Zheng (lv.zheng@intel.com) changed: > > What |Removed |Added > ------------------------------------------------------------ > ---------------- > Summary|[possible regression] |Change in 4.11 > |Change in 4.11 |drivers/acpi/ec.c > |drivers/acpi/ec.c |re-creates bug 44161 > |re-creates bug 44161 | > > --- Comment #11 from Lv Zheng (lv.zheng@intel.com) --- > Let me comment more > > A. To correct my mistake: > Sorry, I have a mistake: > > $ git make oldconfig > > $ git make -j64 > > $ sudo make modules_install install > should be: > $ make oldconfig > $ make -j64 > $ sudo make modules_install install > > B. To know the replicdation ration of the issue: > IMO, you should have very low reproduce ratio for the regression. > As the following conditions should be met: > 1. An EC events (causing SCI_EVT flagged) occurred between > acpi_ec_unblock_transaction() and acpi_ec_resume(); > 2. No EC transactions occurred between > acpi_ec_unblock_transaction() and acpi_ec_resume(); > 3. Samsung platforms. > Only in this case, our original root cause fix is not sufficient > to handle what was dealt by the removed quirk. > And it's a very short period between acpi_ec_unblock_transaction() > and acpi_ec_resume(), probably only several msecs. > So let me ask: > Could you let me know the reproduce ratio of the issue? > > B. To capture some bug related information: > If you are not able to rebuild the kernel right now, then you may > still able to provide useful information for us. If you have > CONFIG_DYNAMIC_DEBUG enabled for your current running kernel, you > still can help to offer some information. > So Could help me to capture what's going on when the bug occurs > using the following steps. > 1. Boot the kernel with "dyndbg='file drivers/acpi/ec.c +p'" > (I hope you needn't rebuild the kernel with CONFIG_DYNAMIC_DEBUG=y); > 2. Perform a suspend/resume and confirm the bug is triggered; > 3. Upload the dmesg output here. > If you cannot do 2 due to lacking of CONFIG_DYNAMIC_DEBUG=y, > you still need to learn kernel build, and stay in the kernel build > directory: > $ make menuconfig > Find DYNAMIC_DEBUG, select it (you can type "\" to enter a search CUI to > find > its location), and exit the menuconfig CUI. > $ make -j64 > $ sudo make modules_install install > > D. To know if the better fix is working for you: > attachment 258293 [details] should be a complete replacement of the removed > quirk. If you can apply this fix on top of your kernel, rebuild > the kernel and try it. You can help me to confirm if the bug is > fully fixed by the attachment 258293 [details]. > > Thanks and best regards > Lv > > -- > You are receiving this mail because: > You reported the bug.
Thanks for the confirmation. Your test is reasonable and match my theory. Thanks for the help. I'll mark this as resolved and close it after having the fixes upstreamed. Thanks Lv
Hi, You've tested against this commit: https://patchwork.kernel.org/patch/9931477/ Actually I'll split it into 2 fixes. 1. unconditionally re-checks SCI_EVT as attachment 258293 [details], and 2. enables EC event handling earlier. As far as I know, there are EC firmware taking very long time to complete its initialization, and hence the 2nd fix is risky as it can trigger timing issues inside of EC FW (race between EC FW initialization and the 1st ACPI EC transaction). I'd rather to make it a seperate patch so that the 1st one won't be affected by the possible regressions.
(In reply to Lv Zheng from comment #14) > Hi, > > You've tested against this commit: > https://patchwork.kernel.org/patch/9931477/ This patch actually contains two fixes, thus it won't be targeted for upstream kernel as we want to fix only one problem in one patch. > Actually I'll split it into 2 fixes. > 1. unconditionally re-checks SCI_EVT as attachment 258293 [details], please test this patch instead.
Yes, https://patchwork.kernel.org/patch/9931477/ actually contains a change useful for bug 196847 which should have a different root cause but similar sympton. Thus it's better to confirm if attachment 258293 [details] is the root cause fix for this issue.