| Message ID | 98a243badc295903ec4cf7de5e2985b468c45907.1463134231.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
[+cc Mika, Rafael] On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote: > If a hotplug port is suspended to D3cold, its slot status register > cannot be read. If that hotplug port happens to share its IRQ with > other devices, then whenever an interrupt occurs for one of these > devices, a "no response from device" message is logged with level > KERN_INFO. Apart from this annoyance, CPU time is needlessly spent > trying to read the slot status register even though we know in advance > that it will fail. I guess this is a pretty generic problem that could affect any device that shares an IRQ. I think I'll queue this on my pci/pm branch, since it seems closely related to Mika's "PCI: Add runtime PM support for PCIe ports". Did you check for the same issue in other likely places, e.g., AER, PME, etc.? > On MacBook Pros introduced 2011 and 2012, the IRQ of a Thunderbolt > hotplug port is unfortunately shared with a wireless card, an audio card > and an SDXC controller. When the Thunderbolt controller is powered > down, the machine carries out at least one unneeded slot status register > read for each wireless packet received and prints a corresponding error > message to the system log. > > The hotplug port's current_state will be D3cold when it's powered down, > so ignore interrupts that occur during that power state. > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/hotplug/pciehp_hpc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 5c24e93..08e84d6 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -546,6 +546,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id) > u8 present; > bool link; > > + /* Interrupts cannot originate from a controller that's asleep */ > + if (pdev->current_state == PCI_D3cold) > + return IRQ_NONE; > + > /* > * In order to guarantee that all interrupt events are > * serviced, we need to re-inspect Slot Status register after > -- > 2.8.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 17, 2016 at 05:52:04PM -0500, Bjorn Helgaas wrote: > On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote: > > If a hotplug port is suspended to D3cold, its slot status register > > cannot be read. If that hotplug port happens to share its IRQ with > > other devices, then whenever an interrupt occurs for one of these > > devices, a "no response from device" message is logged with level > > KERN_INFO. Apart from this annoyance, CPU time is needlessly spent > > trying to read the slot status register even though we know in advance > > that it will fail. > > I guess this is a pretty generic problem that could affect any device > that shares an IRQ. > > I think I'll queue this on my pci/pm branch, since it seems closely > related to Mika's "PCI: Add runtime PM support for PCIe ports". > > Did you check for the same issue in other likely places, e.g., AER, > PME, etc.? Apologies for the delay, I've checked all other port services now: - Our AER and PME drivers bind only to root ports and I can't imagine how those could go to D3cold, they're part of the root complex or PCH and I'm not aware of a chipset that would allow turning off the power well for individual PCIe ports. - DPC on the other hand also binds to downstream ports. I do have downstream ports in my machine (as part of the Thunderbolt switch) but they do not have the DPC capability. I've never seen devices with that capability and cannot estimate what the chances are of them going to D3cold and sharing an IRQ with other devices. It's probably not worth preparing for such a situation without knowing its likelihood. - VC: We allocate a port service for this but do not have a driver. Bottom line is that the patch for the PCIe hotplug driver seems to be sufficient. FWIW, on my machine I see numerous devices with AER, PME and VC capabilities. The Nvidia GPU as well as network, Firewire and Thunderbolt controllers all have those. AFAICS we ignore them because their specific drivers do not care for the capabilities and portdrv only binds to root ports. This seems to support your argument that the PCIe capabilities should be handled by the core rather than portdrv, as we could then make use of the capabilities on endpoint devices in a universal manner. On the other hand, I think we cannot use a separate MSI for AER, PME et al, can we? If we cannot, then AER and PME would share the IRQ with an endpoint device's regular interrupt handler, and that might ruin performance. E.g. the Broadcom wireless card generates millions of interrupts on a sufficiently active WiFi. Accessing the device's config space on every interrupt just to check for AER or PME seems like a bad idea. So at the very least we'd need some kind of opt-out. Best regards, Lukas > > On MacBook Pros introduced 2011 and 2012, the IRQ of a Thunderbolt > > hotplug port is unfortunately shared with a wireless card, an audio card > > and an SDXC controller. When the Thunderbolt controller is powered > > down, the machine carries out at least one unneeded slot status register > > read for each wireless packet received and prints a corresponding error > > message to the system log. > > > > The hotplug port's current_state will be D3cold when it's powered down, > > so ignore interrupts that occur during that power state. > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > --- > > drivers/pci/hotplug/pciehp_hpc.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > > index 5c24e93..08e84d6 100644 > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -546,6 +546,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id) > > u8 present; > > bool link; > > > > + /* Interrupts cannot originate from a controller that's asleep */ > > + if (pdev->current_state == PCI_D3cold) > > + return IRQ_NONE; > > + > > /* > > * In order to guarantee that all interrupt events are > > * serviced, we need to re-inspect Slot Status register after > > -- > > 2.8.1 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, August 02, 2016 06:27:40 PM Lukas Wunner wrote: > On Fri, Jun 17, 2016 at 05:52:04PM -0500, Bjorn Helgaas wrote: > > On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote: > > > If a hotplug port is suspended to D3cold, its slot status register > > > cannot be read. If that hotplug port happens to share its IRQ with > > > other devices, then whenever an interrupt occurs for one of these > > > devices, a "no response from device" message is logged with level > > > KERN_INFO. Apart from this annoyance, CPU time is needlessly spent > > > trying to read the slot status register even though we know in advance > > > that it will fail. > > > > I guess this is a pretty generic problem that could affect any device > > that shares an IRQ. > > > > I think I'll queue this on my pci/pm branch, since it seems closely > > related to Mika's "PCI: Add runtime PM support for PCIe ports". > > > > Did you check for the same issue in other likely places, e.g., AER, > > PME, etc.? > > Apologies for the delay, I've checked all other port services now: > > - Our AER and PME drivers bind only to root ports and I can't imagine > how those could go to D3cold, they're part of the root complex or > PCH and I'm not aware of a chipset that would allow turning off the > power well for individual PCIe ports. No, they don't go into D3cold. They can go into D3hot, however. > - DPC on the other hand also binds to downstream ports. I do have > downstream ports in my machine (as part of the Thunderbolt switch) > but they do not have the DPC capability. I've never seen devices > with that capability and cannot estimate what the chances are of them > going to D3cold and sharing an IRQ with other devices. It's probably > not worth preparing for such a situation without knowing its likelihood. > > - VC: We allocate a port service for this but do not have a driver. > > Bottom line is that the patch for the PCIe hotplug driver seems to be > sufficient. > > FWIW, on my machine I see numerous devices with AER, PME and VC > capabilities. The Nvidia GPU as well as network, Firewire and > Thunderbolt controllers all have those. AFAICS we ignore them > because their specific drivers do not care for the capabilities > and portdrv only binds to root ports. > > This seems to support your argument that the PCIe capabilities > should be handled by the core rather than portdrv, as we could > then make use of the capabilities on endpoint devices in a > universal manner. PME, for one, is not an endpoint capability. It very specifically is defined as a port capability AFAICS, and the whole idea here is that endpoints will not use their in-band interrupts to signal PME. That is supposed to be done by ports. > On the other hand, I think we cannot use a separate MSI for > AER, PME et al, can we? We can, at least in principle. More precisely, the spec requires PME and hotplug to use the same interrupt (please see the comment in pcie_port_enable_msix() on that), but AER can use a different one. > If we cannot, then AER and PME would > share the IRQ with an endpoint device's regular interrupt handler, > and that might ruin performance. E.g. the Broadcom wireless card > generates millions of interrupts on a sufficiently active WiFi. > Accessing the device's config space on every interrupt just to > check for AER or PME seems like a bad idea. So at the very least > we'd need some kind of opt-out. That is why AER, PME and hotplug are all supposed to be signaled by ports, possibly among other things. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 5c24e93..08e84d6 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -546,6 +546,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id) u8 present; bool link; + /* Interrupts cannot originate from a controller that's asleep */ + if (pdev->current_state == PCI_D3cold) + return IRQ_NONE; + /* * In order to guarantee that all interrupt events are * serviced, we need to re-inspect Slot Status register after
If a hotplug port is suspended to D3cold, its slot status register cannot be read. If that hotplug port happens to share its IRQ with other devices, then whenever an interrupt occurs for one of these devices, a "no response from device" message is logged with level KERN_INFO. Apart from this annoyance, CPU time is needlessly spent trying to read the slot status register even though we know in advance that it will fail. On MacBook Pros introduced 2011 and 2012, the IRQ of a Thunderbolt hotplug port is unfortunately shared with a wireless card, an audio card and an SDXC controller. When the Thunderbolt controller is powered down, the machine carries out at least one unneeded slot status register read for each wireless packet received and prints a corresponding error message to the system log. The hotplug port's current_state will be D3cold when it's powered down, so ignore interrupts that occur during that power state. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/hotplug/pciehp_hpc.c | 4 ++++ 1 file changed, 4 insertions(+)