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

Groups > linux.kernel > #1576985 > unrolled thread

[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

  [GIT PULL] PCI fixes for v4.10 Bjorn Helgaas <helgaas@kernel.org> - 2017-02-08 20:50 +0100
    Re: [GIT PULL] PCI fixes for v4.10 Lukas Wunner <lukas@wunner.de> - 2017-02-09 05:40 +0100
      Re: [GIT PULL] PCI fixes for v4.10 Bjorn Helgaas <helgaas@kernel.org> - 2017-02-09 19:20 +0100
        Re: [GIT PULL] PCI fixes for v4.10 "Raj, Ashok" <ashok.raj@intel.com> - 2017-02-09 19:30 +0100
          Re: [GIT PULL] PCI fixes for v4.10 "Raj, Ashok" <ashok.raj@intel.com> - 2017-02-09 19:50 +0100
        Re: [GIT PULL] PCI fixes for v4.10 Bjorn Helgaas <helgaas@kernel.org> - 2017-02-09 21:20 +0100
          Re: [GIT PULL] PCI fixes for v4.10 "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-10 14:20 +0100
          Re: [GIT PULL] PCI fixes for v4.10 Yinghai Lu <yinghai@kernel.org> - 2017-02-11 03:50 +0100
            Re: [GIT PULL] PCI fixes for v4.10 Yinghai Lu <yinghai@kernel.org> - 2017-02-11 08:20 +0100
            Re: [GIT PULL] PCI fixes for v4.10 Lukas Wunner <lukas@wunner.de> - 2017-02-12 20:10 +0100
              Re: [GIT PULL] PCI fixes for v4.10 "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-13 13:20 +0100

#1576985 — [GIT PULL] PCI fixes for v4.10

[+cc, sorry for the duplicate]

PCI fixes:

  - check MSI affinity vs. number of vectors to avoid memory corruption
  - drop runtime power management for PCIe hotplug ports for now to avoid
    regressing hotplug via sysfs


The following changes since commit 030305d69fc6963c16003f50d7e8d74b02d0a143:

  PCI/ASPM: Handle PCI-to-PCIe bridges as roots of PCIe hierarchies (2017-01-27 15:00:45 -0600)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git tags/pci-v4.10-fixes-3

for you to fetch changes up to d98e0929071e7ef63d35c1838b0ad0805ae366dd:

  Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports" (2017-02-03 08:53:51 -0600)

----------------------------------------------------------------
pci-v4.10-fixes-3

----------------------------------------------------------------
Bjorn Helgaas (1):
      Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports"

Christoph Hellwig (1):
      PCI/MSI: Don't apply affinity if there aren't enough vectors left

 drivers/pci/hotplug/pciehp_ctrl.c |  6 ------
 drivers/pci/msi.c                 | 10 ++++++++++
 drivers/pci/pci.c                 | 12 ++++++------
 3 files changed, 16 insertions(+), 12 deletions(-)

[toc] | [next] | [standalone]

#1577305

On Wed, Feb 08, 2017 at 01:22:56PM -0600, Bjorn Helgaas wrote:
> Bjorn Helgaas (1):
>       Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports"

What's the rationale for reverting this?

You've received patches to fix the issue on both affected machines,
so a revert seems unnecessary:

https://patchwork.kernel.org/patch/9557113/
https://patchwork.kernel.org/patch/9562007/

Lukas

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

#1577840

[+cc Ashok, Keith]

On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote:
> On Wed, Feb 08, 2017 at 01:22:56PM -0600, Bjorn Helgaas wrote:
> > Bjorn Helgaas (1):
> >       Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports"
> 
> What's the rationale for reverting this?
> 
> You've received patches to fix the issue on both affected machines,
> so a revert seems unnecessary:
> 
> https://patchwork.kernel.org/patch/9557113/
> https://patchwork.kernel.org/patch/9562007/

I don't think we've gotten to the root cause of the problem yet,
and I don't want to throw in fixes at the last minute without a better
understanding of it.

PCIe hotplug hardware is not very complicated, it hasn't changed in
many years, and at least for the Intel hardware in question, is
generally pretty well-tested with Windows.  So I want to be careful
about asserting that this new piece of hardware is broken.

I think pciehp is unnecessarily complicated, and we do have known
synchronization issues with it, e.g., [1] [2].  It seems possible that
if we poked a little deeper, we would find that the hardware is
actually working correctly and the real problem is in pciehp.

That's why I've been trying to have a conversation about how we
interpret the spec and how we could remove PM and pciehp from the
picture and experiment directly with setpci.

[1] https://lkml.kernel.org/r/1481317564-18045-1-git-send-email-ashok.raj@intel.com
[2] https://bugzilla.kernel.org/show_bug.cgi?id=117561

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

#1577852

Thanks Bjorn,

With the fixes below, managed add remove via sysfs seems to work on my
SKX system. 

I'm not familiar with runtime PM aspects, just started looking into it 
after this one. There seems some interactions with ASPM and how we handle 
devices that support ARI for e.g.

For hotplug, we have a good set of tests to check coverage. That matrix
might need to be expanded for runtime PM interactions.

Cheers,
Ashok

On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote:
> [+cc Ashok, Keith]
> 
> > 
> > https://patchwork.kernel.org/patch/9557113/
> > https://patchwork.kernel.org/patch/9562007/
> 
> I don't think we've gotten to the root cause of the problem yet,
> and I don't want to throw in fixes at the last minute without a better
> understanding of it.
> 
> PCIe hotplug hardware is not very complicated, it hasn't changed in
> many years, and at least for the Intel hardware in question, is
> generally pretty well-tested with Windows.  So I want to be careful
> about asserting that this new piece of hardware is broken.
> 
> I think pciehp is unnecessarily complicated, and we do have known
> synchronization issues with it, e.g., [1] [2].  It seems possible that
> if we poked a little deeper, we would find that the hardware is
> actually working correctly and the real problem is in pciehp.
> 
> That's why I've been trying to have a conversation about how we
> interpret the spec and how we could remove PM and pciehp from the
> picture and experiment directly with setpci.
> 
> [1] https://lkml.kernel.org/r/1481317564-18045-1-git-send-email-ashok.raj@intel.com
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=117561

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

#1577864

[Multipart message — attachments visible in raw view] — view raw

Hi Bjorn/Lucas

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.

Cheers,
Ashok

On Thu, Feb 09, 2017 at 10:23:28AM -0800, Raj, Ashok wrote:
> Thanks Bjorn,
> 
> With the fixes below, managed add remove via sysfs seems to work on my
> SKX system. 
> 
> I'm not familiar with runtime PM aspects, just started looking into it 
> after this one. There seems some interactions with ASPM and how we handle 
> devices that support ARI for e.g.
> 
> For hotplug, we have a good set of tests to check coverage. That matrix
> might need to be expanded for runtime PM interactions.
> 
> Cheers,
> Ashok
> 

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

#1577928

On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote:
> [+cc Ashok, Keith]
> 
> On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote:
> > On Wed, Feb 08, 2017 at 01:22:56PM -0600, Bjorn Helgaas wrote:
> > > Bjorn Helgaas (1):
> > >       Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports"
> > 
> > What's the rationale for reverting this?
> > 
> > You've received patches to fix the issue on both affected machines,
> > so a revert seems unnecessary:
> > 
> > https://patchwork.kernel.org/patch/9557113/
> > https://patchwork.kernel.org/patch/9562007/
> 
> I don't think we've gotten to the root cause of the problem yet,
> and I don't want to throw in fixes at the last minute without a better
> understanding of it.
> 
> PCIe hotplug hardware is not very complicated, it hasn't changed in
> many years, and at least for the Intel hardware in question, is
> generally pretty well-tested with Windows.  So I want to be careful
> about asserting that this new piece of hardware is broken.

I apologize: I had quirks on the brain, but neither of the patches
above is device-specific.  So neither is claiming broken hardware.

However, 9557113 claims we get unwanted PME interrupts if the slot is
occupied when we suspend to D3hot.  This is what I want to explore
further, because that hardware behavior doesn't really make sense to
me.

9562007 apparently fixes something, but at this point it's a debugging
patch (no changelog or signed-off-by) so not a candidate for tossing
into v4.10 at this late date.

Bjorn

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

#1578439

On Thursday, February 09, 2017 02:11:54 PM Bjorn Helgaas wrote:
> On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote:
> > [+cc Ashok, Keith]
> > 
> > On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote:
> > > On Wed, Feb 08, 2017 at 01:22:56PM -0600, Bjorn Helgaas wrote:
> > > > Bjorn Helgaas (1):
> > > >       Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports"
> > > 
> > > What's the rationale for reverting this?
> > > 
> > > You've received patches to fix the issue on both affected machines,
> > > so a revert seems unnecessary:
> > > 
> > > https://patchwork.kernel.org/patch/9557113/
> > > https://patchwork.kernel.org/patch/9562007/
> > 
> > I don't think we've gotten to the root cause of the problem yet,
> > and I don't want to throw in fixes at the last minute without a better
> > understanding of it.
> > 
> > PCIe hotplug hardware is not very complicated, it hasn't changed in
> > many years, and at least for the Intel hardware in question, is
> > generally pretty well-tested with Windows.  So I want to be careful
> > about asserting that this new piece of hardware is broken.
> 
> I apologize: I had quirks on the brain, but neither of the patches
> above is device-specific.  So neither is claiming broken hardware.
> 
> However, 9557113 claims we get unwanted PME interrupts if the slot is
> occupied when we suspend to D3hot.  This is what I want to explore
> further, because that hardware behavior doesn't really make sense to
> me.
> 
> 9562007 apparently fixes something, but at this point it's a debugging
> patch (no changelog or signed-off-by) so not a candidate for tossing
> into v4.10 at this late date.

Right.

FWIW, my view on this is that it woundn't really hurt to allow Thunderbolt
ports only to use runtime PM for the time being while the more general
(PCIe hotplug ports) case is under investigation.

That shouldn't made the code in question excessively complex and the
Thunderbolt special case should be easy enough to get rid of when we
know what to do in general here.

OTOH, the users of the systems with Thunderbolt would benefit from
reduced energy consumption in the meantime.

Thanks,
Rafael

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

#1578924

[Multipart message — attachments visible in raw view] — view raw

On Thu, Feb 9, 2017 at 12:11 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote:
>> [+cc Ashok, Keith]
>>
>> On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote:
>> > On Wed, Feb 08, 2017 at 01:22:56PM -0600, Bjorn Helgaas wrote:
>> > > Bjorn Helgaas (1):
>> > >       Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports"
>> >
>> > What's the rationale for reverting this?
>> >
>> > You've received patches to fix the issue on both affected machines,
>> > so a revert seems unnecessary:
>> >
>> > https://patchwork.kernel.org/patch/9557113/
>> > https://patchwork.kernel.org/patch/9562007/
>>
>> I don't think we've gotten to the root cause of the problem yet,
>> and I don't want to throw in fixes at the last minute without a better
>> understanding of it.
>>
>> PCIe hotplug hardware is not very complicated, it hasn't changed in
>> many years, and at least for the Intel hardware in question, is
>> generally pretty well-tested with Windows.  So I want to be careful
>> about asserting that this new piece of hardware is broken.
>
> I apologize: I had quirks on the brain, but neither of the patches
> above is device-specific.  So neither is claiming broken hardware.
>
> However, 9557113 claims we get unwanted PME interrupts if the slot is
> occupied when we suspend to D3hot.  This is what I want to explore
> further, because that hardware behavior doesn't really make sense to
> me.
>
> 9562007 apparently fixes something, but at this point it's a debugging
> patch (no changelog or signed-off-by) so not a candidate for tossing
> into v4.10 at this late date.

Agreed. It should need more test coverage.

Found more problems.

Actually we don't need 9557113.
as even with that, we still saw link up when power off slots with some cards.

please check updated version of 9562007, that fix power on/off link up problem.

Ashok,

Can ask your QA guys check only attached patch and commit 68db9bc ?

Thanks

Yinghai

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

#1578947

[Multipart message — attachments visible in raw view] — view raw

On Fri, Feb 10, 2017 at 6:39 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> Ashok,
>
> Can ask your QA guys check only attached patch and commit 68db9bc ?

more clean patches: split that into two small patches.

Thanks

Yinghai

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

#1579265

On Fri, Feb 10, 2017 at 06:39:16PM -0800, Yinghai Lu wrote:
> On Thu, Feb 9, 2017 at 12:11 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote:
> > > On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote:
> > > > https://patchwork.kernel.org/patch/9557113/
> > > > https://patchwork.kernel.org/patch/9562007/
> >
> > I apologize: I had quirks on the brain, but neither of the patches
> > above is device-specific.  So neither is claiming broken hardware.
> >
> > However, 9557113 claims we get unwanted PME interrupts if the slot is
> > occupied when we suspend to D3hot.  This is what I want to explore
> > further, because that hardware behavior doesn't really make sense to
> > me.
> >
> > 9562007 apparently fixes something, but at this point it's a debugging
> > patch (no changelog or signed-off-by) so not a candidate for tossing
> > into v4.10 at this late date.
> 
> Agreed. It should need more test coverage.  Found more problems.
> 
> Actually we don't need 9557113 as even with that, we still saw link up
> when power off slots with some cards.
> 
> please check updated version of 9562007, that fix power on/off link up
> problem.

Thank you for debugging this further.  The patch I've submitted today
reinstates runtime PM for hotplug ports but constrains it to those on
a Thunderbolt daisy chain.  The patch allows enabling the feature on
other hardware by booting with pcie_port_pm=force.

A few things to keep in mind:

* On Thunderbolt hotplug ports, interrupts are sent even if the port
  is in D3hot, which as Bjorn has pointed out contradicts the PCI PM
  spec r1.2, table 5-4.  This may be caused by liberal interpretation
  of the spec by Intel when designing the Thunderbolt controllers,
  or perhaps Thunderbolt controllers simply do not possess a "real",
  fully-fledged PCIe switch.  I let the hotplug ports go to D3hot,
  expecting them to continue delivering interrupts but YMMV.

* You've reported that the hotplug port must be in D0 to enable and
  disable power on the slot.  I think this is not required by the spec.
  Thunderbolt hotplug ports do not support power control.  My suspicion
  is that the ports on your machine must remain in D0 as long as the
  slot is occupied, i.e. they must not runtime suspend to D3hot.  Can
  this happen?  Yes.  I release the runtime PM ref once a slot has been
  enabled or disabled.  The device remains runtime active as long as it
  has active children.  If all children runtime suspend, the port will
  go to D3hot, which might cause trouble if this implies that slot power
  is turned off.  To test this you need a card whose Linux driver supports
  runtime PM (e.g. Nvidia GPU, boot with nouveau.runpm=1).

* If the hotplug slot has runtime suspended to D3hot and there are ports
  above it that also runtime suspend to D3hot, its config space is no
  longer accessible and in-band interrupts won't come through.  A side-band
  signaling method such as PME WAKE# is required to deliver interrupts from
  this state.  Also, the hotplug_slot_ops defined for pciehp will have to
  be augmented with calls to pm_runtime_get_sync() and pm_runtime_put()
  to wake the parent of the hotplug port so that config space is accessible
  when interacting with the slot via sysfs.

* If pciehp_poll_mode is used, it may be necessary to call
  pm_runtime_forbid(). (Or alternatively runtime resume it whenever config
  space is polled, but that seems silly.)


> --- linux-2.6.orig/drivers/pci/hotplug/pciehp_ctrl.c
> +++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -89,17 +89,17 @@ static int board_added(struct slot *p_sl
>  	struct controller *ctrl = p_slot->ctrl;
>  	struct pci_bus *parent = ctrl->pcie->port->subordinate;
>  
> +	pm_runtime_get_sync(&ctrl->pcie->port->dev);
>  	if (POWER_CTRL(ctrl)) {
>  		/* Power on slot */
>  		retval = pciehp_power_on_slot(p_slot);
>  		if (retval)
> -			return retval;
> +			goto err_exit;
>  	}
>  
>  	pciehp_green_led_blink(p_slot);
>  
>  	/* Check link training status */
> -	pm_runtime_get_sync(&ctrl->pcie->port->dev);
>  	retval = pciehp_check_link_status(ctrl);
>  	if (retval) {
>  		ctrl_err(ctrl, "Failed to check link status\n");

Well, it may be simpler to just move the pm_runtime_get_sync() / _put()
to the caller of board_added() and remove_board().  That way it's not
necessary to insert a pm_runtime_put() into every error path.  The
patch I've submitted today does exactly that.

In fact, v2 of my Thunderbolt runtime PM series, posted in May 2016,
already did that:
http://www.spinics.net/lists/linux-pci/msg51153.html

But for v3 I decided to move the pm_runtime_get_sync() / _put() down
the call stack into board_added() and remove_board() to make more
precise exactly which operations require the hotplug port to be in D0.
Guess that wasn't a good idea. :-(

Thanks,

Lukas

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

#1579672

On Sunday, February 12, 2017 08:05:02 PM Lukas Wunner wrote:
> On Fri, Feb 10, 2017 at 06:39:16PM -0800, Yinghai Lu wrote:
> > On Thu, Feb 9, 2017 at 12:11 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote:
> > > > On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote:
> > > > > https://patchwork.kernel.org/patch/9557113/
> > > > > https://patchwork.kernel.org/patch/9562007/
> > >
> > > I apologize: I had quirks on the brain, but neither of the patches
> > > above is device-specific.  So neither is claiming broken hardware.
> > >
> > > However, 9557113 claims we get unwanted PME interrupts if the slot is
> > > occupied when we suspend to D3hot.  This is what I want to explore
> > > further, because that hardware behavior doesn't really make sense to
> > > me.
> > >
> > > 9562007 apparently fixes something, but at this point it's a debugging
> > > patch (no changelog or signed-off-by) so not a candidate for tossing
> > > into v4.10 at this late date.
> > 
> > Agreed. It should need more test coverage.  Found more problems.
> > 
> > Actually we don't need 9557113 as even with that, we still saw link up
> > when power off slots with some cards.
> > 
> > please check updated version of 9562007, that fix power on/off link up
> > problem.
> 
> Thank you for debugging this further.  The patch I've submitted today
> reinstates runtime PM for hotplug ports but constrains it to those on
> a Thunderbolt daisy chain.  The patch allows enabling the feature on
> other hardware by booting with pcie_port_pm=force.
> 
> A few things to keep in mind:
> 
> * On Thunderbolt hotplug ports, interrupts are sent even if the port
>   is in D3hot, which as Bjorn has pointed out contradicts the PCI PM
>   spec r1.2, table 5-4.  This may be caused by liberal interpretation
>   of the spec by Intel when designing the Thunderbolt controllers,
>   or perhaps Thunderbolt controllers simply do not possess a "real",
>   fully-fledged PCIe switch.  I let the hotplug ports go to D3hot,
>   expecting them to continue delivering interrupts but YMMV.
> 
> * You've reported that the hotplug port must be in D0 to enable and
>   disable power on the slot.  I think this is not required by the spec.
>   Thunderbolt hotplug ports do not support power control.  My suspicion
>   is that the ports on your machine must remain in D0 as long as the
>   slot is occupied, i.e. they must not runtime suspend to D3hot.  Can
>   this happen?  Yes.  I release the runtime PM ref once a slot has been
>   enabled or disabled.  The device remains runtime active as long as it
>   has active children.  If all children runtime suspend, the port will
>   go to D3hot, which might cause trouble if this implies that slot power
>   is turned off.  To test this you need a card whose Linux driver supports
>   runtime PM (e.g. Nvidia GPU, boot with nouveau.runpm=1).
> 
> * If the hotplug slot has runtime suspended to D3hot and there are ports
>   above it that also runtime suspend to D3hot, its config space is no
>   longer accessible and in-band interrupts won't come through.  A side-band
>   signaling method such as PME WAKE# is required to deliver interrupts from
>   this state.

It actually can use in-band PME messages too, at least if my interpretation of
this part of the spec is correct (or reflects the interpretation of the people
who design the chips in question to be more precise).

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web