Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1634079 > unrolled thread

Re: [GIT PULL] PCI fixes for v4.10

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL] PCI fixes for v4.10 Lukas Wunner <lukas@wunner.de> - 2017-05-02 04:00 +0200
    Re: [GIT PULL] PCI fixes for v4.10 Sinan Kaya <okaya@codeaurora.org> - 2017-05-02 04:50 +0200
      Re: [GIT PULL] PCI fixes for v4.10 Lukas Wunner <lukas@wunner.de> - 2017-05-02 12:50 +0200
        Re: [GIT PULL] PCI fixes for v4.10 Sinan Kaya <okaya@codeaurora.org> - 2017-05-02 16:20 +0200
        Re: [GIT PULL] PCI fixes for v4.10 Bjorn Helgaas <helgaas@kernel.org> - 2017-05-02 20:50 +0200
    Re: [GIT PULL] PCI fixes for v4.10 Lukas Wunner <lukas@wunner.de> - 2017-05-06 11:10 +0200

#1634079 — Re: [GIT PULL] PCI fixes for v4.10

Hi Ashok,

sorry for the delay, I was swamped with work in the first quarter and am
in the process of slowly going through my backlog...


On Thu, Feb 09, 2017 at 10:46:13AM -0800, Raj, Ashok wrote:
> I did a lspci and captured after a fresh reboot on the slot before 
> attempting a power off via sysfs.
> 
> lspci-at-poweron - values after a cold reset poweron.
> lspci-aft-poweroff - values after a echo 0 > power
> lspci-aft-poweron - values after a echo 1 > power
> 
> i would expect most or all of those values to stay same between poweron
> and after the next sysfs managed poweron. Would be worth taking a look and see 
> if we have any escapes.


(a) Correctable error (Receiver Error) after powering off => harmless.

(b) ASPM L1 enabled on boot, but disabled after powering off and back on
    => I believe Sinan is working on this (+cc).

(c) Autonomous Bandwidth Status is set after powering back on, but the
    speed and width is the same as on boot => harmless.

(d) PME Status is set after powering off and stays set after powering on.
    Weird.  Could you retry with the new pciehp runtime PM series I sent
    out a few hours ago?

Thanks!

Lukas

[toc] | [next] | [standalone]

#1634093

On 5/1/2017 9:54 PM, Lukas Wunner wrote:
> (b) ASPM L1 enabled on boot, but disabled after powering off and back on
>     => I believe Sinan is working on this (+cc).

The decision was made not to touch ASPM registers following hotplug insertion
unless pcie_aspm.policy=powersave is specified.

The discussion is here: https://lkml.org/lkml/2017/4/17/255

This was done to maintain existing behavior and not break things.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1634332

On Mon, May 01, 2017 at 10:41:20PM -0400, Sinan Kaya wrote:
> On 5/1/2017 9:54 PM, Lukas Wunner wrote:
> > (b) ASPM L1 enabled on boot, but disabled after powering off and back on
> >     => I believe Sinan is working on this (+cc).
> 
> The decision was made not to touch ASPM registers following hotplug insertion
> unless pcie_aspm.policy=powersave is specified.
> 
> The discussion is here: https://lkml.org/lkml/2017/4/17/255
> 
> This was done to maintain existing behavior and not break things.

Thanks for the reference, I hadn't followed the discussion in April
very closely, but I think the outcome of the discussion is unfortunate.

As can be seen in Ashok's tests, merely turning slot power off and back
on is sufficient to end up with a setting that draws more power.  That
may be equally surprising for users as the issues would be that we seek
to avoid with a "safety-first" ASPM policy.  In any case it seems
undesirable.

I hope this is not the end if it and would like to encourage you to
keep working on this.  Perhaps it is too simple to just define a
default policy, and what is really needed is a policy that adjusts
itself dynamically to specific devices or workloads, or that can be
influenced by device drivers.

Thanks for your efforts,

Lukas

[toc] | [prev] | [next] | [standalone]

#1634446

On 5/2/2017 6:49 AM, Lukas Wunner wrote:
> On Mon, May 01, 2017 at 10:41:20PM -0400, Sinan Kaya wrote:
>> On 5/1/2017 9:54 PM, Lukas Wunner wrote:
>>> (b) ASPM L1 enabled on boot, but disabled after powering off and back on
>>>     => I believe Sinan is working on this (+cc).
>>
>> The decision was made not to touch ASPM registers following hotplug insertion
>> unless pcie_aspm.policy=powersave is specified.
>>
>> The discussion is here: https://lkml.org/lkml/2017/4/17/255
>>
>> This was done to maintain existing behavior and not break things.
> 
> Thanks for the reference, I hadn't followed the discussion in April
> very closely, but I think the outcome of the discussion is unfortunate.
> 
> As can be seen in Ashok's tests, merely turning slot power off and back
> on is sufficient to end up with a setting that draws more power.  That
> may be equally surprising for users as the issues would be that we seek
> to avoid with a "safety-first" ASPM policy.  In any case it seems
> undesirable.
> 
> I hope this is not the end if it and would like to encourage you to
> keep working on this.  Perhaps it is too simple to just define a
> default policy, and what is really needed is a policy that adjusts
> itself dynamically to specific devices or workloads, or that can be
> influenced by device drivers.
> 

I think our conclusion was to push PM decision to a userspace utility.
ASPM already has a knob in sysfs that can be adjusted at runtime. The
name of the file is policy. Same argument as the kernel command line
but it is writable. Different policies can be programmed into this
field after OS boot.

It is really the system power management entity that needs to decide
how system should behave. 

We just need to educate the userspace utility about the presence
of ASPM policy variable.

1. OS could still boot with the default parameters. 
2. Userspace utility sets the policy variable to powersave
3. User performs hotplug remove + insert
4. ASPM is enabled due to policy change.

I don't know anything about what this userspace utility is or if
it understands about PCIE ASPM at all. 

As an analogy, we can choose the level of power management in windows
through Power Policy.

control panel -> power options -> change advanced power options ->
PCI Express -> Link State Power Management

We want to have the same level of configuration.

> Thanks for your efforts,
> 
> Lukas
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1634577

On Tue, May 02, 2017 at 12:49:29PM +0200, Lukas Wunner wrote:
> On Mon, May 01, 2017 at 10:41:20PM -0400, Sinan Kaya wrote:
> > On 5/1/2017 9:54 PM, Lukas Wunner wrote:
> > > (b) ASPM L1 enabled on boot, but disabled after powering off and back on
> > >     => I believe Sinan is working on this (+cc).
> > 
> > The decision was made not to touch ASPM registers following hotplug insertion
> > unless pcie_aspm.policy=powersave is specified.
> > 
> > The discussion is here: https://lkml.org/lkml/2017/4/17/255
> > 
> > This was done to maintain existing behavior and not break things.
> 
> Thanks for the reference, I hadn't followed the discussion in April
> very closely, but I think the outcome of the discussion is unfortunate.
> 
> As can be seen in Ashok's tests, merely turning slot power off and back
> on is sufficient to end up with a setting that draws more power.  That
> may be equally surprising for users as the issues would be that we seek
> to avoid with a "safety-first" ASPM policy.  In any case it seems
> undesirable.
> 
> I hope this is not the end if it and would like to encourage you to
> keep working on this.  Perhaps it is too simple to just define a
> default policy, and what is really needed is a policy that adjusts
> itself dynamically to specific devices or workloads, or that can be
> influenced by device drivers.

It's not the end of the discussion.  If you have an alternate
proposal, we'd love to hear it, especially if you implement it.

[toc] | [prev] | [next] | [standalone]

#1636864

Hi Ashok,

On Wed, May 03, 2017 at 11:04:28AM -0700, Raj, Ashok wrote:
> attached is recent set of experiments with your patch series applied
> on 4.11 kernel.
> 
> Right after a reboot, poweroff causes an NMI. You can see the message on 
> 1.dmesg. 1.lspci is after reboot, and 2.lspci is after a poweroff/poweron
> sequence.
> 
> You can see there are other differences, like Corrected error enables, 
> and several others.. 

Thanks a lot for carrying out the tests!

It looks like powering the slot off and back on did work - so far so good.

The NMI seems to occur when powering off the slot.  The lspci output shows
that the Ethernet card in the slot signaled an Unsupported Request Error
which it handled as an Advisory Non-Fatal Error.  However the CPER is sent
by the hotplug port, not by the card in the slot.  It's unclear if the root
port is forwarding the error on behalf of the card in the slot or if it
experienced an error itself.  Please also send lspci output for the hotplug
port (0000:5d:00.0) to clarify this.  It would be ideal to also have lspci
output after the slot has been powered off and before it's powered on.

Unfortunately our CPER parser doesn't log the contents of the "AER info"
field.  It would contain the TLP that triggered the error.

Some ideas to understand what's going on:
- Could you test if the NMI also occurs without my pciehp runtime PM series,
  i.e. with a stock v4.11 kernel?
- Could you apply the small debug patch included below and re-test powering
  off/on?  It will log when the root port goes to D3hot or back to D0.
- If you test with a different card in the slot, do you also get an NMI?

There are a few oddities that caught my eye:
- The class code is logged with incorrect byte order.  I've sent out a
  patch yesterday, could you test if it fixes this?
- After calling pciehp_green_led_off(), there are *two* Command Complete
  interrupts from the slot?!
- The Ethernet card's Parity Error Response bit (#6 in Command Register)
  is unset after boot, but set after powering the slot off and back on?!

Thanks,

Lukas

-- >8 --
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 5b638c6..65f3b07 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -421,9 +421,10 @@ static int rpm_idle(struct device *dev, int rpmflags)
 
 	callback = RPM_GET_CALLBACK(dev, runtime_idle);
 
-	if (callback)
+	if (callback) {
+		dev_info(dev, "rpm_idle\n");
 		retval = __rpm_callback(callback, dev);
-
+	}
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
 
@@ -592,6 +593,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
 
 	dev_pm_enable_wake_irq_check(dev, true);
+	dev_info(dev, "rpm_suspend\n");
 	retval = rpm_callback(callback, dev);
 	if (retval)
 		goto fail;
@@ -814,6 +816,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
 	callback = RPM_GET_CALLBACK(dev, runtime_resume);
 
 	dev_pm_disable_wake_irq_check(dev);
+	dev_info(dev, "rpm_resume\n");
 	retval = rpm_callback(callback, dev);
 	if (retval) {
 		__update_runtime_status(dev, RPM_SUSPENDED);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web