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

["Zoltán Kővágó" <dirty.ice.hu@gmail.com>]

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