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

[Zev Weiss <zev@bewilderbeest.net>]

On Wed, Aug 10, 2022 at 12:33:43AM PDT, Guenter Roeck wrote:
>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.

True, that's a fair point.

>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.
>

nct6775-core.c currently has DEFAULT_SYMBOL_NAMESPACE #defined to 
HWMON_NCT6775 (and the platform and i2c drivers are using 
MODULE_IMPORT_NS(HWMON_NCT6775)), so I think that's already in place -- 
I guess we can just leave it as is with the patch that's now upstream 
then.


Zev