Bug 70311 - kernel 3.12+ breaks r8169 ethernet cards on suspend
Summary: kernel 3.12+ breaks r8169 ethernet cards on suspend
Status: ASSIGNED
Alias: None
Product: Drivers
Classification: Unclassified
Component: Network (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Srivatsa S. Bhat
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-08 21:35 UTC by Pierre Ossman
Modified: 2016-03-19 17:27 UTC (History)
7 users (show)

See Also:
Kernel Version: 3.12 and later
Subsystem:
Regression: No
Bisected commit-id:


Attachments
Patch to fix per-cpu initialization in powernow-k8 cpufreq driver (2.57 KB, patch)
2014-02-17 10:58 UTC, Srivatsa S. Bhat
Details | Diff

Description Pierre Ossman 2014-02-08 21:35:39 UTC
Originally filed with Red Hat:

https://bugzilla.redhat.com/show_bug.cgi?id=1054408

The short summary is that 3.11.9 worked fine on this machine. Upgrading to the 3.12 line causes my r8169 card to go nuts after a suspend. It even falls off the PCI bus after a reboot.

Also tried 3.13 today with the same effect.

Tried patching r8169.c with the v3.11..v3.12 diff, but it had no effect. Would like to avoid a full bisect.

Help? :/
Comment 1 Stanislaw Gruszka 2014-02-09 22:08:36 UTC
Here is mail thread about that bug:
https://lkml.org/lkml/2014/2/5/720
Bisection will be needed but perhaps some else will do it :-)
Comment 2 Pierre Ossman 2014-02-10 07:12:27 UTC
I've started, but it's tedious. Right now it's telling me that it is down to just cpufreq patches. Seems unlikely, but we'll see what this gives... :/
Comment 3 Francois Romieu 2014-02-13 17:09:16 UTC
(In reply to Pierre Ossman from comment #2)
> I've started, but it's tedious. Right now it's telling me that it is down to
> just cpufreq patches. Seems unlikely, but we'll see what this gives... :/

A patch has been made available in the thread that Stanislaw quoted. See:
http://marc.info/?l=linux-kernel&m=139211494420501

-- 
Ueimor
Comment 4 Pierre Ossman 2014-02-13 18:59:50 UTC
(In reply to Pierre Ossman from comment #2)
> I've started, but it's tedious. Right now it's telling me that it is down to
> just cpufreq patches. Seems unlikely, but we'll see what this gives... :/

Oddly enough, it is this commit that triggers the issue here:

commit 23d328994b548d6822b88fe7e1903652afc354e0
Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Date:   Tue Jul 30 04:23:56 2013 +0530

    cpufreq: Fix misplaced call to cpufreq_update_policy()
    
    The call to cpufreq_update_policy() is placed in the CPU hotplug callback
    of cpufreq_stats, which has a higher priority than the CPU hotplug callback
    of cpufreq-core. As a result, during CPU_ONLINE/CPU_ONLINE_FROZEN, we end up
    calling cpufreq_update_policy() *before* calling cpufreq_add_dev() !
    And for uninitialized CPUs, it just returns silently, not doing anything.
    
    To add to that, cpufreq_stats is not even the right place to call
    cpufreq_update_policy() to begin with. The cpufreq core ought to handle
    this in its own callback, from an elegance/relevance perspective.
    
    So move the invocation of cpufreq_update_policy() to cpufreq_cpu_callback,
    and place it *after* cpufreq_add_dev().
    
    Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
    Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since my system doesn't load cpufreq_stats, the only relevant change is the one in cpufreq.c. I've tested this:

3.11-rc4: OK
3.11-rc4 + 23d3289: FAIL
3.11-rc4 + 23d3289 + reverted cpufreq.c: OK
3.12.9: FAIL
3.12.9 + reverted cpufreq.c: OK

(In reply to Francois Romieu from comment #3)
> A patch has been made available in the thread that Stanislaw quoted. See:
> http://marc.info/?l=linux-kernel&m=139211494420501

No effect I'm afraid. I applied it on top of 3.12.9 and the system still screwed up the ethernet controller on resume.


So right now the only tangible connection I have for my problem is that cpufreq commit. I've added Rafael since he was involved in both that commit and the linked thread.
Comment 5 Pierre Ossman 2014-02-13 20:33:50 UTC
I turned on debug for cpufreq.c and compared dmesg output. In the failing case I get these extra lines going down:

 Disabling non-boot CPUs ...
 cpufreq: __cpufreq_remove_dev_prepare: unregistering CPU 1
 cpufreq: __cpufreq_governor for CPU 0, event 2
+Broke affinity for irq 16
+Broke affinity for irq 19
+Broke affinity for irq 22
 smpboot: CPU 1 is now offline

And these extra lines coming back up:

+cpufreq: updating policy for CPU 1
+cpufreq: Warning: CPU frequency out of sync: cpufreq and timing core thinks of 2800000, is 0 kHz.
+cpufreq: notification 0 of frequency transition to 0 kHz
+cpufreq: notification 0 of frequency transition to 0 kHz
+cpufreq: notification 1 of frequency transition to 0 kHz
+cpufreq: FREQ: 0 - CPU: 0
+cpufreq: notification 1 of frequency transition to 0 kHz
+cpufreq: FREQ: 0 - CPU: 1
+cpufreq: setting new policy for CPU 0: 1000000 - 2800000 kHz
+cpufreq: new min and max freqs are 1000000 - 2800000 kHz
+cpufreq: governor: change or update limits
+cpufreq: __cpufreq_governor for CPU 0, event 3
+cpufreq: target for CPU 0: 2800000 kHz, relation 1, requested 2800000 kHz
 CPU1 is up
 ACPI: Waking up from system sleep state S3

Those 0 kHz look a bit concerning?
Comment 6 Stanislaw Gruszka 2014-02-13 20:40:53 UTC
Adding new email address of Rafael, old one does not work any longer.
Comment 7 Pierre Ossman 2014-02-13 21:39:19 UTC
I think I might have found the bug. This code in cpufreq_update_policy() looks highly suspect:

        if (cpufreq_driver->get) {
                new_policy.cur = cpufreq_driver->get(cpu);
                if (!policy->cur) {
                        pr_debug("Driver did not initialize current freq");
                        policy->cur = new_policy.cur;
                } else {

I can't see cpufreq_driver have any connection to policy state, so it seems like whoever wrote that code simply confused policy and new_policy. Rewriting the code to this triggers that debug output and leaves me with a working system:

        if (cpufreq_driver->get) {
                new_policy.cur = cpufreq_driver->get(cpu);
                if (!new_policy.cur) {
                        pr_debug("Driver did not initialize current freq");
                        new_policy.cur = policy->cur;
                } else {

Someone more knowledgeable about cpufreq might want to have a look though. :)

As for why this is causing these problems, I have no idea. That 0 kHz being used in timeout calculations elsewhere?
Comment 8 Rafael J. Wysocki 2014-02-13 23:29:41 UTC
Srivatsa and Viresh are more familiar with that code, so reassigning.
Comment 9 Rafael J. Wysocki 2014-02-13 23:31:24 UTC
That said the patch from comment #7 actually looks correct to me, so why don't you send it to linux-pm@vger.kernel.org ?
Comment 10 Pierre Ossman 2014-02-14 06:33:54 UTC
(In reply to Rafael J. Wysocki from comment #9)
> That said the patch from comment #7 actually looks correct to me, so why
> don't you send it to linux-pm@vger.kernel.org ?

Done:

http://article.gmane.org/gmane.linux.power-management.general/42806
Comment 11 Viresh Kumar 2014-02-14 09:50:46 UTC
(In reply to Pierre Ossman from comment #10)
> (In reply to Rafael J. Wysocki from comment #9)
> > That said the patch from comment #7 actually looks correct to me, so why
> > don't you send it to linux-pm@vger.kernel.org ?
> 
> Done:
> 
> http://article.gmane.org/gmane.linux.power-management.general/42806

Hi,

That patch was sent as attachment and so couldn't review it there. But to move things forward, let me reply here:

Unlike Rafael, I don't agree your version of code is right :) .. Though I may be completely wrong here. This is what we are doing there (In the existing code):
- After s2r, we wake up and check what's the current frequency of system is?
- Then we check if the existing policy say current freq is zero, then simply overwrite it with new or actual current value.
- Else, check if current stored freq is same to new one or not, if not then call out-of-sync stuff..

And the change you are introducing keeps the currently stored freq as is, thinking there is no change in freq. Which is wrong. We must get a value from hardware for sure..

Now the important issue or the problem here is only one: Why does cpufreq_driver->get(cpu) return zero? That's the only things that is getting wrong here. I need to look in more detail to understand what's going on.. Can you please let me know some details of your system, processors ,freq, etc? May run cpufreq-info and give its output.
Comment 12 Pierre Ossman 2014-02-14 10:46:54 UTC
(In reply to Viresh Kumar from comment #11)
> Now the important issue or the problem here is only one: Why does
> cpufreq_driver->get(cpu) return zero? That's the only things that is getting
> wrong here. I need to look in more detail to understand what's going on..
> Can you please let me know some details of your system, processors ,freq,
> etc? May run cpufreq-info and give its output.

If I understand things correctly, cpufreq-info is replaced by cpupower, and this is its output:

[root@hugin ~]# cpupower frequency-info
analyzing CPU 0:
  driver: powernow-k8
  CPUs which run at the same hardware frequency: 0 1
  CPUs which need to have their frequency coordinated by software: 0 1
  maximum transition latency: 109 us.
  hardware limits: 1000 MHz - 2.80 GHz
  available frequency steps: 2.80 GHz, 2.60 GHz, 2.40 GHz, 2.20 GHz, 2.00 GHz, 1.80 GHz, 1000 MHz
  available cpufreq governors: conservative, userspace, powersave, ondemand, performance
  current policy: frequency should be within 1000 MHz and 2.80 GHz.
                  The governor "ondemand" may decide which speed to use
                  within this range.
  current CPU frequency is 1000 MHz (asserted by call to hardware).
  boost state support:
    Supported: no
    Active: no

This is an older CPU, but it has been running fine for years:

model name      : AMD Athlon(tm) 64 X2 Dual Core Processor 5600+

Motherboard:

[    0.000000] SMBIOS 2.5 present.
[    0.000000] DMI: MICRO-STAR INTERNATIONAL CO.,LTD MS-7368/K9AG Neo2 (MS-7368), BIOS V1.10 09/01/2009
Comment 13 Srivatsa S. Bhat 2014-02-14 10:51:53 UTC
(In reply to Pierre Ossman from comment #7)
> I think I might have found the bug. This code in cpufreq_update_policy()
> looks highly suspect:
> 
>         if (cpufreq_driver->get) {
>                 new_policy.cur = cpufreq_driver->get(cpu);
>                 if (!policy->cur) {
>                         pr_debug("Driver did not initialize current freq");
>                         policy->cur = new_policy.cur;
>                 } else {
> 
> I can't see cpufreq_driver have any connection to policy state, so it seems
> like whoever wrote that code simply confused policy and new_policy.
> Rewriting the code to this triggers that debug output and leaves me with a
> working system:
> 
>         if (cpufreq_driver->get) {
>                 new_policy.cur = cpufreq_driver->get(cpu);
>                 if (!new_policy.cur) {
>                         pr_debug("Driver did not initialize current freq");
>                         new_policy.cur = policy->cur;
>                 } else {
> 

The check for !<policy>.cur does imply that it was meant to check whether the
value that the driver returned from ->get() was sane. So, going by that,
your patch does make sense.

However, Viresh proposed a different solution on IRC, where we might be
able to completely get rid of the call to cpufreq_update_policy() from
the CPU hotplug notifier. I think his reasoning behind that solution
makes sense. I'm sure he'll post his fix soon. Let's see how that works.

Regards,
Srivatsa S. Bhat
Comment 14 Viresh Kumar 2014-02-14 11:05:56 UTC
On 14 February 2014 16:21,  <bugzilla-daemon@bugzilla.kernel.org> wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=70311
>
> --- Comment #13 from Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> ---
> (In reply to Pierre Ossman from comment #7)
>> I think I might have found the bug. This code in cpufreq_update_policy()
>> looks highly suspect:
>>
>>         if (cpufreq_driver->get) {
>>                 new_policy.cur = cpufreq_driver->get(cpu);
>>                 if (!policy->cur) {
>>                         pr_debug("Driver did not initialize current freq");
>>                         policy->cur = new_policy.cur;
>>                 } else {
>>
>> I can't see cpufreq_driver have any connection to policy state, so it seems
>> like whoever wrote that code simply confused policy and new_policy.
>> Rewriting the code to this triggers that debug output and leaves me with a
>> working system:
>>
>>         if (cpufreq_driver->get) {
>>                 new_policy.cur = cpufreq_driver->get(cpu);
>>                 if (!new_policy.cur) {
>>                         pr_debug("Driver did not initialize current freq");
>>                         new_policy.cur = policy->cur;
>>                 } else {
>>
>
> The check for !<policy>.cur does imply that it was meant to check whether the
> value that the driver returned from ->get() was sane. So, going by that,
> your patch does make sense.

Really? I mean, yes the !<policy>.cur check makes sense but not the code
after that.. i.e.

>>                 if (!new_policy.cur) {
>>                         pr_debug("Driver did not initialize current freq");
>>                         new_policy.cur = policy->cur;

We can't say that we switch back to the stored freq here, that might be wrong.
Freq might have changed underneath and that's what we are checking here.

> However, Viresh proposed a different solution on IRC, where we might be
> able to completely get rid of the call to cpufreq_update_policy() from
> the CPU hotplug notifier. I think his reasoning behind that solution
> makes sense. I'm sure he'll post his fix soon. Let's see how that works.

I have sent that already. I am sure it will get rid of the issues
Pierre is facing
but not in the right sense. We need to know why get() failed for him..
Comment 15 Pierre Ossman 2014-02-14 12:39:59 UTC
(In reply to Viresh Kumar from comment #14)
> On 14 February 2014 16:21,  <bugzilla-daemon@bugzilla.kernel.org> wrote:
> > However, Viresh proposed a different solution on IRC, where we might be
> > able to completely get rid of the call to cpufreq_update_policy() from
> > the CPU hotplug notifier. I think his reasoning behind that solution
> > makes sense. I'm sure he'll post his fix soon. Let's see how that works.
> 
> I have sent that already. I am sure it will get rid of the issues
> Pierre is facing
> but not in the right sense. We need to know why get() failed for him..

Which IRC channel/network?

And is there a link to said patch? I also need to get the ball rolling with Red Hat, even if we keep digging into the "get" problem.
Comment 16 Srivatsa S. Bhat 2014-02-17 10:58:20 UTC
Created attachment 126391 [details]
Patch to fix per-cpu initialization in powernow-k8 cpufreq driver

Hi Pierre,

Can you please try out this patch and let us know if it solves your problem?

Thanks!

Regards,
Srivatsa S. Bhat
Comment 17 Pierre Ossman 2014-02-17 19:40:40 UTC
(In reply to Srivatsa S. Bhat from comment #16)
> Created attachment 126391 [details]
> Patch to fix per-cpu initialization in powernow-k8 cpufreq driver
> 
> Hi Pierre,
> 
> Can you please try out this patch and let us know if it solves your problem?
> 

Seems to work fine. I reverted all the other tested fixes and applied just this patch on 3.12.9. I can't see any remaining issues at this point.

Thanks. :)
Comment 18 Pierre Ossman 2014-02-17 19:41:57 UTC
I guess one or more of these patches will show up on the stable branches?
Comment 19 Srivatsa S. Bhat 2014-02-18 09:04:50 UTC
(In reply to Pierre Ossman from comment #18)
> I guess one or more of these patches will show up on the stable branches?

Yes, I have added a "CC: stable" tag in the powernow-k8 driver fix.
So once the patch hits mainline, it will automatically get applied to
the relevant stable trees.

Thank you for all your testing efforts!

Regards,
Srivatsa S. Bhat

Note You need to log in before you can comment on or make changes to this bug.