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

[Guenter Roeck <linux@roeck-us.net>]

On 8/9/22 23:03, Zev Weiss wrote:
> On Tue, Aug 09, 2022 at 08:36:24PM PDT, Guenter Roeck wrote:
>> On 8/9/22 20:00, Armin Wolf wrote:
>>> Am 10.08.22 um 03:39 schrieb Zev Weiss:
>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>> In case you are running a Debian-based distribution, please note that Debian unconditionally executes sensors on startup, something
>>> i discovered when the dell-smm-hwmon driver was causing issues on my machine.
>>>
>>> So you may need to temporarily disable lm-sensors.service and/or /etc/init.d/lm-sensors for testing.
>>>
> 
> I'm not sure offhand what it does in that area, but Zoltán's initial bug report indicated that the problem occurred on Gentoo, FWIW.
> 
>>
>> The assumption was that the sensors command was _not_ executed prior
>> to suspend and that this triggers the problem. If it was executed at least
>> once and the crash on resume still happens, something else must be going
>> on. Either case, I think it is best to apply the patch suggested by Zev.
>> We can try to figure out the exact cause of the problem later. Zev, can
>> you send me a formal patch ? I would like to apply and send it to Linus
>> as soon as possible.
>>
> 
> Ack, patch sent.
> 
>  From what we've seen so far though, it seems likely (if for no other reason than that if this _isn't_ the cause there must be something truly weird going on) that the problem stems from nct6775_update_device() simply having never been called at all.  If we can confirm that that's the case, should we perhaps just add an unconditional call to nct6775_update_device() at the end of nct6775_probe(), and revert the patch I just sent so as to avoid the single-consumer symbol export?
> 

I thought about calling it in probe, but I don't really like it because
it would slow down boot. Let's introduce a namespace (say, NCT6775) instead.
Change all EXPORT_SYMBOL_GPL(x) to EXPORT_SYMBOL_NS_GPL(x, NCT6775) and
add MODULE_IMPORT_NS(NCT6775) to nct6775-platform.c and nct6775-i2c.c.
This will ensure that the exported symbols can only be called from the
nct6775 code.

Thanks,
Guenter