Re: PROBLEM: NCT6775: suspend doesn't work after updating to Linux 5.19

[Zev Weiss <zev@bewilderbeest.net>]

On Wed, Aug 10, 2022 at 01:22:04AM PDT, Zoltán Kővágó wrote:
>On 2022-08-10 03:39, Zev Weiss wrote:
>>On Tue, Aug 09, 2022 at 04:50:20PM PDT, Zoltán Kővágó wrote:
>>>On 2022-08-10 00:34, Zev Weiss wrote:
>>>>On Tue, Aug 09, 2022 at 02:28:20PM PDT, Zoltán Kővágó wrote:
>>>>>On 2022-08-09 22:56, Zev Weiss wrote:
>>>>>>On Tue, Aug 09, 2022 at 01:27:48PM PDT, Zoltán Kővágó wrote:
>>>>>>>Hi,
>>>>>>>
>>>>>>>[1.] One line summary of the problem:
>>>>>>>NCT6775: suspend doesn't work after updating to Linux 5.19
>>>>>>>
>>>>>>>[2.] Full description of the problem/report:
>>>>>>>After updating my kernel from 5.18.11 to 5.19, I've 
>>>>>>>noticed that resuming after suspend no longer works: fans 
>>>>>>>start up, then about a second later, the computer just 
>>>>>>>shuts down, leaving the USB ports powered up (normally it 
>>>>>>>turns them off on shutdown). The screens don't turn on 
>>>>>>>during this timeframe, so I can't see any useful log 
>>>>>>>messages.
>>>>>>>
>>>>>>>Bisecting between 5.18 (where it still worked) and 5.19 
>>>>>>>lead me to commit c3963bc0a0cf9ecb205a9d4976eb92b6df2fa3fd 
>>>>>>>"hwmon: (nct6775) Split core and platform driver" which 
>>>>>>>looks like a refactor commit, but apparently it broke 
>>>>>>>something.
>>>>>>>
>>>>>>
>>>>>>Hi Zoltán,
>>>>>>
>>>>>>Thanks for the thorough bug report.  You're right that that 
>>>>>>commit was essentially just a refactor, though there was one 
>>>>>>slight change to the nct6775_suspend() function introduced 
>>>>>>during the review process that may perhaps have had some 
>>>>>>subtle unintended side-effects.
>>>>>>
>>>>>>Can you test the following patch and report if it resolves 
>>>>>>the problem?
>>>>>>
>>>>>>
>>>>>>Thanks,
>>>>>>Zev
>>>>>
>>>>>Hi Zev,
>>>>>
>>>>>Thanks for the quick reply. Yes, it looks like your patch does 
>>>>>solve the problem (I've applied it on top of 5.19 (after 
>>>>>fighting with my mail client for a while) and suspended a few 
>>>>>times, it's working so far).
>>>>>
>>>>>Regards,
>>>>>Zoltan
>>>>
>>>>Great, thanks.
>>>>
>>>>Guenter, it looks like nct6775_suspend() really does in fact 
>>>>need to use nct6775_update_device() instead of 
>>>>dev_get_drvdata(), though it's not immediately obvious to me 
>>>>why.  Though given that the bulk of of the body of 
>>>>nct6775_update_device() is inside an 'if' block that might not 
>>>>necessarily execute every time, I also wonder if it might be 
>>>>vulnerable to exhibiting the same problem depending on timing.
>>>>
>>>>Zoltán, if you could try another experiment to try to gather 
>>>>some data on that -- with the patch from my previous email still 
>>>>applied, could you try suspending via:
>>>>
>>>>    $ cat 
>>>>/sys/bus/platform/drivers/nct6775/nct6775.*/hwmon/hwmon*/*_input 
>>>>&& echo mem > /sys/power/state
>>>
>>>Tried it, three times in fact, and it worked fine every time. 
>>>Looking at the dmesg, though, it looks like it needs a bit more 
>>>than 1.5 sec to suspend. Where's that 1.5 sec limit defined? I 
>>>will try to increase it tomorrow.
>>>
>>
>>The 1.5 second duration comes from this line in nct6775_update_device():
>>
>>   if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
>>
>>Each 'HZ' represents one second, so HZ + HZ / 2 = 1.5 sec; if you 
>>want to lengthen it you could do e.g. 10 * HZ or something instead.
>
>Tried that, this isn't the problem.
>
>>
>>Though as Guenter noted, one other possibility is that with the 
>>previous (buggy) version nct6775_update_device() might never have 
>>gotten called at all -- do you know if that might be the case on 
>>your system?  (i.e. do you have any userspace monitoring program or 
>>the like that would have been reading from the nct6775 device's 
>>sensors?)  If something like that never ran between the driver 
>>getting loaded and the system suspending, that might also 
>>(partially) explain things; to test that out you could revert to the 
>>old buggy code and see if the suspend problem still occurs if you 
>>explicitly run
>>
>>   $ cat /sys/bus/platform/drivers/nct6775/nct6775.*/hwmon/hwmon*/*_input
>>
>>(or just 'sensors' if you've got the lm-sensors package installed).  
>>That will
>>ensure that nct6775_update_device() gets called before the suspend 
>>operation, which could help narrow things down further.
>
>Yup, this was it. It looks like I remembered it wrong and my 
>monitoring widget in the end only used k10temp and not nct6798, so I 
>could very easily suspend without any reads from nct6775 before (and 
>that widget itself even only ran when I had an X session).
>
>Regards,
>Zoltan

Great, glad that's confirmed.  The fix is now in Linus's tree and should 
presumably get picked up in the -stable tree for 5.19.1.

Thanks for the bug report & testing!


Zev