* [PATCH v2 0/2] Honoring Native AER/DPC Host Bridges @ 2020-04-20 21:37 Jon Derrick 2020-04-20 21:37 ` [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER Jon Derrick 2020-04-20 21:37 ` [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC Jon Derrick 0 siblings, 2 replies; 13+ messages in thread From: Jon Derrick @ 2020-04-20 21:37 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, Keith Busch, Jon Derrick, Russell Currey, Sam Bobroff, Oliver O'Halloran, Bjorn Helgaas, Kuppuswamy Sathyanarayanan, Andy Shevchenko, Frederick Lawler, Rajat Jain, Patel, Mayurkumar, Olof Johansson, Rafael J. Wysocki, Mika Westerberg, Alex Williamson, linuxppc-dev, linux-kernel The two patches here force AER and DPC to honor the Host Bridge's Native AER/DPC settings. This is under the assumption that when these bits are set, that Firmware-First AER/DPC should not be in use for these ports. This assumption seems to be true in ACPI, which explicitly clears these capability settings in the host bridge if the service cannot be negotiated with _OSC. This also fixes an issue I've seen in a few platforms whose BIOS and/or switch firmware leaves DPC preconfigured. In these cases, the kernel DPC driver cannot bind a handler to the interrupt and could result in unmanaged DPC link down events. Jon Derrick (2): PCI/AER: Allow Native AER Host Bridges to use AER PCI/DPC: Allow Native DPC Host Bridges to use DPC drivers/pci/pcie/aer.c | 3 +++ drivers/pci/pcie/dpc.c | 3 ++- drivers/pci/pcie/portdrv_core.c | 3 ++- 3 files changed, 7 insertions(+), 2 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER 2020-04-20 21:37 [PATCH v2 0/2] Honoring Native AER/DPC Host Bridges Jon Derrick @ 2020-04-20 21:37 ` Jon Derrick 2020-04-22 22:48 ` Kuppuswamy, Sathyanarayanan 2020-04-24 23:30 ` Bjorn Helgaas 2020-04-20 21:37 ` [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC Jon Derrick 1 sibling, 2 replies; 13+ messages in thread From: Jon Derrick @ 2020-04-20 21:37 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, Keith Busch, Jon Derrick, Russell Currey, Sam Bobroff, Oliver O'Halloran, Bjorn Helgaas, Kuppuswamy Sathyanarayanan, Andy Shevchenko, Frederick Lawler, Rajat Jain, Patel, Mayurkumar, Olof Johansson, Rafael J. Wysocki, Mika Westerberg, Alex Williamson, linuxppc-dev, linux-kernel Some platforms have a mix of ports whose capabilities can be negotiated by _OSC, and some ports which are not described by ACPI and instead managed by Native drivers. The existing Firmware-First HEST model can incorrectly tag these Native, Non-ACPI ports as Firmware-First managed ports by advertising the HEST Global Flag and matching the type and class of the port (aer_hest_parse). If the port requests Native AER through the Host Bridge's capability settings, the AER driver should honor those settings and allow the port to bind. This patch changes the definition of Firmware-First to exclude ports whose Host Bridges request Native AER. Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> --- drivers/pci/pcie/aer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index f4274d3..30fbd1f 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) if (pcie_ports_native) return 0; + if (pci_find_host_bridge(dev->bus)->native_aer) + return 0; + if (!dev->__aer_firmware_first_valid) aer_set_firmware_first(dev); return dev->__aer_firmware_first; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER 2020-04-20 21:37 ` [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER Jon Derrick @ 2020-04-22 22:48 ` Kuppuswamy, Sathyanarayanan 2020-04-23 15:11 ` Derrick, Jonathan 2020-04-24 23:30 ` Bjorn Helgaas 1 sibling, 1 reply; 13+ messages in thread From: Kuppuswamy, Sathyanarayanan @ 2020-04-22 22:48 UTC (permalink / raw) To: Jon Derrick, Bjorn Helgaas Cc: linux-pci, Keith Busch, Russell Currey, Sam Bobroff, Oliver O'Halloran, Bjorn Helgaas, Andy Shevchenko, Frederick Lawler, Rajat Jain, Patel, Mayurkumar, Olof Johansson, Rafael J. Wysocki, Mika Westerberg, Alex Williamson, linuxppc-dev, linux-kernel On 4/20/20 2:37 PM, Jon Derrick wrote: > Some platforms have a mix of ports whose capabilities can be negotiated > by _OSC, and some ports which are not described by ACPI and instead > managed by Native drivers. The existing Firmware-First HEST model can > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed > ports by advertising the HEST Global Flag and matching the type and > class of the port (aer_hest_parse). Is there a real use case for mixed mode (one host bridge in FF mode and another in native)? > > If the port requests Native AER through the Host Bridge's capability > settings, the AER driver should honor those settings and allow the port > to bind. This patch changes the definition of Firmware-First to exclude > ports whose Host Bridges request Native AER. > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > --- > drivers/pci/pcie/aer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index f4274d3..30fbd1f 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > if (pcie_ports_native) > return 0; > > + if (pci_find_host_bridge(dev->bus)->native_aer) > + return 0; > + > if (!dev->__aer_firmware_first_valid) > aer_set_firmware_first(dev); > return dev->__aer_firmware_first; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER 2020-04-22 22:48 ` Kuppuswamy, Sathyanarayanan @ 2020-04-23 15:11 ` Derrick, Jonathan 0 siblings, 0 replies; 13+ messages in thread From: Derrick, Jonathan @ 2020-04-23 15:11 UTC (permalink / raw) To: sathyanarayanan.kuppuswamy, helgaas Cc: rajatja, kbusch, ruscur, fred, Wysocki, Rafael J, linux-kernel, alex.williamson, sbobroff, olof, oohall, mika.westerberg, linuxppc-dev, bhelgaas, Patel, Mayurkumar, andriy.shevchenko, linux-pci Hi Sathyanarayanan, On Wed, 2020-04-22 at 15:48 -0700, Kuppuswamy, Sathyanarayanan wrote: > > On 4/20/20 2:37 PM, Jon Derrick wrote: > > Some platforms have a mix of ports whose capabilities can be negotiated > > by _OSC, and some ports which are not described by ACPI and instead > > managed by Native drivers. The existing Firmware-First HEST model can > > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed > > ports by advertising the HEST Global Flag and matching the type and > > class of the port (aer_hest_parse). > Is there a real use case for mixed mode (one host bridge in FF mode and > another in native)? Intel's VMD exposes PCIe segments containing Root Ports and Bridges and other DPC consumers. These extra PCIe domains aren't described by ACPI. There have been a few versions where DPC won't bind due to platform's HEST configuration. > > If the port requests Native AER through the Host Bridge's capability > > settings, the AER driver should honor those settings and allow the port > > to bind. This patch changes the definition of Firmware-First to exclude > > ports whose Host Bridges request Native AER. > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > --- > > drivers/pci/pcie/aer.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index f4274d3..30fbd1f 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > > if (pcie_ports_native) > > return 0; > > > > + if (pci_find_host_bridge(dev->bus)->native_aer) > > + return 0; > > + > > if (!dev->__aer_firmware_first_valid) > > aer_set_firmware_first(dev); > > return dev->__aer_firmware_first; > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER 2020-04-20 21:37 ` [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER Jon Derrick 2020-04-22 22:48 ` Kuppuswamy, Sathyanarayanan @ 2020-04-24 23:30 ` Bjorn Helgaas 2020-04-27 16:11 ` Derrick, Jonathan 1 sibling, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2020-04-24 23:30 UTC (permalink / raw) To: Jon Derrick Cc: linux-pci, Keith Busch, Russell Currey, Sam Bobroff, Oliver O'Halloran, Kuppuswamy Sathyanarayanan, Andy Shevchenko, Frederick Lawler, Rajat Jain, Patel, Mayurkumar, Olof Johansson, Rafael J. Wysocki, Mika Westerberg, Alex Williamson, linuxppc-dev, linux-kernel Hi Jon, I'm glad you raised this because I think the way we handle FIRMWARE_FIRST is really screwed up. On Mon, Apr 20, 2020 at 03:37:09PM -0600, Jon Derrick wrote: > Some platforms have a mix of ports whose capabilities can be negotiated > by _OSC, and some ports which are not described by ACPI and instead > managed by Native drivers. The existing Firmware-First HEST model can > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed > ports by advertising the HEST Global Flag and matching the type and > class of the port (aer_hest_parse). > > If the port requests Native AER through the Host Bridge's capability > settings, the AER driver should honor those settings and allow the port > to bind. This patch changes the definition of Firmware-First to exclude > ports whose Host Bridges request Native AER. > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > --- > drivers/pci/pcie/aer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index f4274d3..30fbd1f 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > if (pcie_ports_native) > return 0; > > + if (pci_find_host_bridge(dev->bus)->native_aer) > + return 0; I hope we don't have to complicate pcie_aer_get_firmware_first() by adding this "native_aer" check here. I'm not sure what we actually *should* do based on FIRMWARE_FIRST, but I don't think the current uses really make sense. I think Linux makes too many assumptions based on the FIRMWARE_FIRST bit. The ACPI spec really only says (ACPI v6.3, sec 18.3.2.4): If set, FIRMWARE_FIRST indicates to the OSPM that system firmware will handle errors from this source first. If FIRMWARE_FIRST is set in the flags field, the Enabled field [of the HEST AER structure] is ignored by the OSPM. I do not see anything there about who owns the AER Capability, but Linux assumes that if FIRMWARE_FIRST is set, firmware must own the AER Capability. I think that's reading too much into the spec. We already have _OSC, which *does* explicitly talk about who owns the AER Capability, and I think we should rely on that. If firmware doesn't want the OS to touch the AER Capability, it should decline to give ownership to the OS via _OSC. > if (!dev->__aer_firmware_first_valid) > aer_set_firmware_first(dev); > return dev->__aer_firmware_first; > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER 2020-04-24 23:30 ` Bjorn Helgaas @ 2020-04-27 16:11 ` Derrick, Jonathan 2020-04-27 22:14 ` Bjorn Helgaas 0 siblings, 1 reply; 13+ messages in thread From: Derrick, Jonathan @ 2020-04-27 16:11 UTC (permalink / raw) To: helgaas Cc: rajatja, fred, ruscur, kbusch, Wysocki, Rafael J, alex.williamson, olof, sbobroff, linux-kernel, oohall, mika.westerberg, linuxppc-dev, linux-pci, Patel, Mayurkumar, andriy.shevchenko, sathyanarayanan.kuppuswamy Hi Bjorn, On Fri, 2020-04-24 at 18:30 -0500, Bjorn Helgaas wrote: > Hi Jon, > > I'm glad you raised this because I think the way we handle > FIRMWARE_FIRST is really screwed up. > > On Mon, Apr 20, 2020 at 03:37:09PM -0600, Jon Derrick wrote: > > Some platforms have a mix of ports whose capabilities can be negotiated > > by _OSC, and some ports which are not described by ACPI and instead > > managed by Native drivers. The existing Firmware-First HEST model can > > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed > > ports by advertising the HEST Global Flag and matching the type and > > class of the port (aer_hest_parse). > > > > If the port requests Native AER through the Host Bridge's capability > > settings, the AER driver should honor those settings and allow the port > > to bind. This patch changes the definition of Firmware-First to exclude > > ports whose Host Bridges request Native AER. > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > --- > > drivers/pci/pcie/aer.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index f4274d3..30fbd1f 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > > if (pcie_ports_native) > > return 0; > > > > + if (pci_find_host_bridge(dev->bus)->native_aer) > > + return 0; > > I hope we don't have to complicate pcie_aer_get_firmware_first() by > adding this "native_aer" check here. I'm not sure what we actually > *should* do based on FIRMWARE_FIRST, but I don't think the current > uses really make sense. > > I think Linux makes too many assumptions based on the FIRMWARE_FIRST > bit. The ACPI spec really only says (ACPI v6.3, sec 18.3.2.4): > > If set, FIRMWARE_FIRST indicates to the OSPM that system firmware > will handle errors from this source first. > > If FIRMWARE_FIRST is set in the flags field, the Enabled field [of > the HEST AER structure] is ignored by the OSPM. > > I do not see anything there about who owns the AER Capability, but > Linux assumes that if FIRMWARE_FIRST is set, firmware must own the AER > Capability. I think that's reading too much into the spec. > > We already have _OSC, which *does* explicitly talk about who owns the > AER Capability, and I think we should rely on that. If firmware > doesn't want the OS to touch the AER Capability, it should decline to > give ownership to the OS via _OSC. > > > if (!dev->__aer_firmware_first_valid) > > aer_set_firmware_first(dev); > > return dev->__aer_firmware_first; > > -- > > 1.8.3.1 > > Just a little bit of reading and my interpretation, as it seems like some of this is just layers upon layers of possibly conflicting yet intentionally vague descriptions. _OSC seems to describe that OSPM can handle AER (6.2.11.3): PCI Express Advanced Error Reporting (AER) control The OS sets this bit to 1 to request control over PCI Express AER. If the OS successfully receives control of this feature, it must handle error reporting through the AER Capability as described in the PCI Express Base Specification. For AER and DPC the ACPI root port enumeration will properly set native_aer/dpc based on _OSC: struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, ... if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL)) host_bridge->native_aer = 0; if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL)) host_bridge->native_pme = 0; if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL)) host_bridge->native_ltr = 0; if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL)) host_bridge->native_dpc = 0; As DPC was defined in an ECN [1], I would imagine AER will need to cover DPC for legacy platforms prior to the ECN. The complication is that HEST also seems to describe how ports (and other devices) are managed either individually or globally: Table 18-387 PCI Express Root Port AER Structure ... Flags: [0] - FIRMWARE_FIRST: If set, this bit indicates to the OSPM that system firmware will handle errors from this source [1] - GLOBAL: If set, indicates that the settings contained in this structure apply globally to all PCI Express Devices. All other bits must be set to zero The _OSC definition seems to contradict/negate the above FIRMWARE_FIRST definition that says only firmware will handle errors. It's a bit different than the IA_32 MCE definition which allows for a GHES_ASSIST condition, which would cause Firmware 'First', however does allow the error to be received by OSPM AER via GHES: Table 18-385 IA-32 Architecture Corrected Machine Check Structure [0] - FIRMWARE_FIRST: If set, this bit indicates that system firmware will handle errors from this source first. [2] - GHES_ASSIST: If set, this bit indicates that although OSPM is responsible for directly handling the error (as expected when FIRMWARE_FIRST is not set), system firmware reports additional information in the context of an interrupt generated by the error. The additional information is reported in a Generic Hardware Error Source structure with a matching Related Source Id. I think Linux needs to make an assumption that devices either enumerated in HEST or enumerated globally by HEST should be managed by FFS. However it seems that Linux should also be correlating that with _OSC as _OSC seems to directly contradict and possibly supercede the HEST expectation. [1] https://members.pcisig.com/wg/PCI-SIG/document/12888 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER 2020-04-27 16:11 ` Derrick, Jonathan @ 2020-04-27 22:14 ` Bjorn Helgaas 0 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2020-04-27 22:14 UTC (permalink / raw) To: Derrick, Jonathan Cc: rajatja, fred, ruscur, kbusch, Wysocki, Rafael J, alex.williamson, olof, sbobroff, linux-kernel, oohall, mika.westerberg, linuxppc-dev, linux-pci, Patel, Mayurkumar, andriy.shevchenko, sathyanarayanan.kuppuswamy On Mon, Apr 27, 2020 at 04:11:07PM +0000, Derrick, Jonathan wrote: > On Fri, 2020-04-24 at 18:30 -0500, Bjorn Helgaas wrote: > > I'm glad you raised this because I think the way we handle > > FIRMWARE_FIRST is really screwed up. > > > > On Mon, Apr 20, 2020 at 03:37:09PM -0600, Jon Derrick wrote: > > > Some platforms have a mix of ports whose capabilities can be negotiated > > > by _OSC, and some ports which are not described by ACPI and instead > > > managed by Native drivers. The existing Firmware-First HEST model can > > > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed > > > ports by advertising the HEST Global Flag and matching the type and > > > class of the port (aer_hest_parse). > > > > > > If the port requests Native AER through the Host Bridge's capability > > > settings, the AER driver should honor those settings and allow the port > > > to bind. This patch changes the definition of Firmware-First to exclude > > > ports whose Host Bridges request Native AER. > > > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > > --- > > > drivers/pci/pcie/aer.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > > index f4274d3..30fbd1f 100644 > > > --- a/drivers/pci/pcie/aer.c > > > +++ b/drivers/pci/pcie/aer.c > > > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev) > > > if (pcie_ports_native) > > > return 0; > > > > > > + if (pci_find_host_bridge(dev->bus)->native_aer) > > > + return 0; > > > > I hope we don't have to complicate pcie_aer_get_firmware_first() by > > adding this "native_aer" check here. I'm not sure what we actually > > *should* do based on FIRMWARE_FIRST, but I don't think the current > > uses really make sense. > > > > I think Linux makes too many assumptions based on the FIRMWARE_FIRST > > bit. The ACPI spec really only says (ACPI v6.3, sec 18.3.2.4): > > > > If set, FIRMWARE_FIRST indicates to the OSPM that system firmware > > will handle errors from this source first. > > > > If FIRMWARE_FIRST is set in the flags field, the Enabled field [of > > the HEST AER structure] is ignored by the OSPM. > > > > I do not see anything there about who owns the AER Capability, but > > Linux assumes that if FIRMWARE_FIRST is set, firmware must own the AER > > Capability. I think that's reading too much into the spec. > > > > We already have _OSC, which *does* explicitly talk about who owns the > > AER Capability, and I think we should rely on that. If firmware > > doesn't want the OS to touch the AER Capability, it should decline to > > give ownership to the OS via _OSC. > > > > > if (!dev->__aer_firmware_first_valid) > > > aer_set_firmware_first(dev); > > > return dev->__aer_firmware_first; > > Just a little bit of reading and my interpretation, as it seems like > some of this is just layers upon layers of possibly conflicting yet > intentionally vague descriptions. > > _OSC seems to describe that OSPM can handle AER (6.2.11.3): > PCI Express Advanced Error Reporting (AER) control > The OS sets this bit to 1 to request control over PCI Express AER. > If the OS successfully receives control of this feature, it must > handle error reporting through the AER Capability as described in > the PCI Express Base Specification. > > > For AER and DPC the ACPI root port enumeration will properly set > native_aer/dpc based on _OSC: > > struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > ... > if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL)) > host_bridge->native_aer = 0; > if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL)) > host_bridge->native_pme = 0; > if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL)) > host_bridge->native_ltr = 0; > if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL)) > host_bridge->native_dpc = 0; > > As DPC was defined in an ECN [1], I would imagine AER will need to > cover DPC for legacy platforms prior to the ECN. > > > > The complication is that HEST also seems to describe how ports (and > other devices) are managed either individually or globally: > > Table 18-387 PCI Express Root Port AER Structure > ... > Flags: > [0] - FIRMWARE_FIRST: If set, this bit indicates to the OSPM that > system firmware will handle errors from this source > [1] - GLOBAL: If set, indicates that the settings contained in this > structure apply globally to all PCI Express Devices. All other bits > must be set to zero > > > The _OSC definition seems to contradict/negate the above FIRMWARE_FIRST > definition that says only firmware will handle errors. It's a bit > different than the IA_32 MCE definition which allows for a GHES_ASSIST > condition, which would cause Firmware 'First', however does allow the > error to be received by OSPM AER via GHES: > > Table 18-385 IA-32 Architecture Corrected Machine Check Structure > [0] - FIRMWARE_FIRST: If set, this bit indicates that system > firmware will handle errors from this source first. > [2] - GHES_ASSIST: If set, this bit indicates that although OSPM is > responsible for directly handling the error (as expected when > FIRMWARE_FIRST is not set), system firmware reports additional > information in the context of an interrupt generated by the error. > The additional information is reported in a Generic Hardware Error > Source structure with a matching Related Source Id. > > > I think Linux needs to make an assumption that devices either > enumerated in HEST or enumerated globally by HEST should be managed by > FFS. However it seems that Linux should also be correlating that with > _OSC as _OSC seems to directly contradict and possibly supercede the > HEST expectation. That's basically what Linux been doing -- we've been assuming that if _OSC declines to grant us control, *or* if FFS is set somewhere, we shouldn't touch the AER capability. But this leads to lots of weird corner cases, and I really doubt that firmware and Linux are interpreting all these the same way. What breaks if we change Linux to *only* use _OSC to determine ownership of the AER capability? My argument is that firmware doesn't want the OS to touch the AER capability registers, it should decline to give the OS control of the AER capability via _OSC. If _OSC grants control to the OS in a case where firmware doesn't want the OS to have control, I'd say that's just a firmware defect that should be worked around with some sort of quirk. > [1] https://members.pcisig.com/wg/PCI-SIG/document/12888 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC 2020-04-20 21:37 [PATCH v2 0/2] Honoring Native AER/DPC Host Bridges Jon Derrick 2020-04-20 21:37 ` [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER Jon Derrick @ 2020-04-20 21:37 ` Jon Derrick 2020-04-22 22:50 ` Kuppuswamy, Sathyanarayanan 1 sibling, 1 reply; 13+ messages in thread From: Jon Derrick @ 2020-04-20 21:37 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, Keith Busch, Jon Derrick, Russell Currey, Sam Bobroff, Oliver O'Halloran, Bjorn Helgaas, Kuppuswamy Sathyanarayanan, Andy Shevchenko, Frederick Lawler, Rajat Jain, Patel, Mayurkumar, Olof Johansson, Rafael J. Wysocki, Mika Westerberg, Alex Williamson, linuxppc-dev, linux-kernel The existing portdrv model prevents DPC services without either OS control (_OSC) granted to AER services, a Host Bridge requesting Native AER, or using one of the 'pcie_ports=' parameters of 'native' or 'dpc-native'. The DPC port service driver itself will also fail to probe if the kernel assumes the port is using Firmware-First AER. It's a reasonable expectation that a port using Firmware-First AER will also be using Firmware-First DPC, however if a Host Bridge requests Native DPC, the DPC driver should allow it and not fail to bind due to AER capability settings. Host Bridges which request Native DPC port services will also likely request Native AER, however it shouldn't be a requirement. This patch allows ports on those Host Bridges to have DPC port services. This will avoid the unlikely situation where the port is Firmware-First AER and Native DPC, and a BIOS or switch firmware preconfiguration of the DPC trigger could result in unhandled DPC events. Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> --- drivers/pci/pcie/dpc.c | 3 ++- drivers/pci/pcie/portdrv_core.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index 7621704..3f3106f 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) int status; u16 ctl, cap; - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && + !pci_find_host_bridge(pdev->bus)->native_dpc) return -ENOTSUPP; status = devm_request_threaded_irq(device, dev->irq, dpc_irq, diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 50a9522..f2139a1 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev) */ if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && pci_aer_available() && - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) + (pcie_ports_dpc_native || host->native_dpc || + (services & PCIE_PORT_SERVICE_AER))) services |= PCIE_PORT_SERVICE_DPC; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC 2020-04-20 21:37 ` [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC Jon Derrick @ 2020-04-22 22:50 ` Kuppuswamy, Sathyanarayanan 2020-04-23 15:11 ` Derrick, Jonathan 0 siblings, 1 reply; 13+ messages in thread From: Kuppuswamy, Sathyanarayanan @ 2020-04-22 22:50 UTC (permalink / raw) To: Jon Derrick, Bjorn Helgaas Cc: linux-pci, Keith Busch, Russell Currey, Sam Bobroff, Oliver O'Halloran, Bjorn Helgaas, Andy Shevchenko, Frederick Lawler, Rajat Jain, Patel, Mayurkumar, Olof Johansson, Rafael J. Wysocki, Mika Westerberg, Alex Williamson, linuxppc-dev, linux-kernel On 4/20/20 2:37 PM, Jon Derrick wrote: > The existing portdrv model prevents DPC services without either OS > control (_OSC) granted to AER services, a Host Bridge requesting Native > AER, or using one of the 'pcie_ports=' parameters of 'native' or > 'dpc-native'. > > The DPC port service driver itself will also fail to probe if the kernel > assumes the port is using Firmware-First AER. It's a reasonable > expectation that a port using Firmware-First AER will also be using > Firmware-First DPC, however if a Host Bridge requests Native DPC, the > DPC driver should allow it and not fail to bind due to AER capability > settings. > > Host Bridges which request Native DPC port services will also likely > request Native AER, however it shouldn't be a requirement. This patch > allows ports on those Host Bridges to have DPC port services. > > This will avoid the unlikely situation where the port is Firmware-First > AER and Native DPC, and a BIOS or switch firmware preconfiguration of > the DPC trigger could result in unhandled DPC events. > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > --- > drivers/pci/pcie/dpc.c | 3 ++- > drivers/pci/pcie/portdrv_core.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > index 7621704..3f3106f 100644 > --- a/drivers/pci/pcie/dpc.c > +++ b/drivers/pci/pcie/dpc.c > @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) > int status; > u16 ctl, cap; > > - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && > + !pci_find_host_bridge(pdev->bus)->native_dpc) Why do it in probe as well ? if host->native_dpc is not set then the device DPC probe it self won't happen right ? > return -ENOTSUPP; > > status = devm_request_threaded_irq(device, dev->irq, dpc_irq, > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 50a9522..f2139a1 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev) > */ > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && > pci_aer_available() && > - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) > + (pcie_ports_dpc_native || host->native_dpc || > + (services & PCIE_PORT_SERVICE_AER))) > services |= PCIE_PORT_SERVICE_DPC; > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC 2020-04-22 22:50 ` Kuppuswamy, Sathyanarayanan @ 2020-04-23 15:11 ` Derrick, Jonathan 2020-04-25 20:46 ` Kuppuswamy, Sathyanarayanan 0 siblings, 1 reply; 13+ messages in thread From: Derrick, Jonathan @ 2020-04-23 15:11 UTC (permalink / raw) To: sathyanarayanan.kuppuswamy, helgaas Cc: rajatja, kbusch, ruscur, fred, Wysocki, Rafael J, linux-kernel, alex.williamson, sbobroff, olof, oohall, mika.westerberg, linuxppc-dev, bhelgaas, Patel, Mayurkumar, andriy.shevchenko, linux-pci Hi Sathyanarayanan, On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote: > > On 4/20/20 2:37 PM, Jon Derrick wrote: > > The existing portdrv model prevents DPC services without either OS > > control (_OSC) granted to AER services, a Host Bridge requesting Native > > AER, or using one of the 'pcie_ports=' parameters of 'native' or > > 'dpc-native'. > > > > The DPC port service driver itself will also fail to probe if the kernel > > assumes the port is using Firmware-First AER. It's a reasonable > > expectation that a port using Firmware-First AER will also be using > > Firmware-First DPC, however if a Host Bridge requests Native DPC, the > > DPC driver should allow it and not fail to bind due to AER capability > > settings. > > > > Host Bridges which request Native DPC port services will also likely > > request Native AER, however it shouldn't be a requirement. This patch > > allows ports on those Host Bridges to have DPC port services. > > > > This will avoid the unlikely situation where the port is Firmware-First > > AER and Native DPC, and a BIOS or switch firmware preconfiguration of > > the DPC trigger could result in unhandled DPC events. > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > --- > > drivers/pci/pcie/dpc.c | 3 ++- > > drivers/pci/pcie/portdrv_core.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > > index 7621704..3f3106f 100644 > > --- a/drivers/pci/pcie/dpc.c > > +++ b/drivers/pci/pcie/dpc.c > > @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) > > int status; > > u16 ctl, cap; > > > > - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) > > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && > > + !pci_find_host_bridge(pdev->bus)->native_dpc) > Why do it in probe as well ? if host->native_dpc is not set then the > device DPC probe it self won't happen right ? Portdrv only enables the interrupt and allows the probe to occur. The probe itself will still fail if there's a mixed-mode _OSC negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1 for AER and no check for DPC. I don't know if such a platform will exist, but the kernel is already wired for 'dpc-native' so it makes sense to extend it for this.. This transform might be more readable: if (pcie_aer_get_firmware_first(pdev) && !(pcie_ports_dpc_native || hb->native_dpc)) > > return -ENOTSUPP; > > > > status = devm_request_threaded_irq(device, dev->irq, dpc_irq, > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > index 50a9522..f2139a1 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev) > > */ > > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && > > pci_aer_available() && > > - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) > > + (pcie_ports_dpc_native || host->native_dpc || > > + (services & PCIE_PORT_SERVICE_AER))) > > services |= PCIE_PORT_SERVICE_DPC; > > > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC 2020-04-23 15:11 ` Derrick, Jonathan @ 2020-04-25 20:46 ` Kuppuswamy, Sathyanarayanan 2020-04-27 15:15 ` Derrick, Jonathan 0 siblings, 1 reply; 13+ messages in thread From: Kuppuswamy, Sathyanarayanan @ 2020-04-25 20:46 UTC (permalink / raw) To: Derrick, Jonathan, helgaas Cc: rajatja, kbusch, ruscur, fred, Wysocki, Rafael J, linux-kernel, alex.williamson, sbobroff, olof, oohall, mika.westerberg, linuxppc-dev, bhelgaas, Patel, Mayurkumar, andriy.shevchenko, linux-pci On 4/23/20 8:11 AM, Derrick, Jonathan wrote: > Hi Sathyanarayanan, > > On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote: >> >> On 4/20/20 2:37 PM, Jon Derrick wrote: >>> The existing portdrv model prevents DPC services without either OS >>> control (_OSC) granted to AER services, a Host Bridge requesting Native >>> AER, or using one of the 'pcie_ports=' parameters of 'native' or >>> 'dpc-native'. >>> >>> The DPC port service driver itself will also fail to probe if the kernel >>> assumes the port is using Firmware-First AER. It's a reasonable >>> expectation that a port using Firmware-First AER will also be using >>> Firmware-First DPC, however if a Host Bridge requests Native DPC, the >>> DPC driver should allow it and not fail to bind due to AER capability >>> settings. >>> >>> Host Bridges which request Native DPC port services will also likely >>> request Native AER, however it shouldn't be a requirement. This patch >>> allows ports on those Host Bridges to have DPC port services. >>> >>> This will avoid the unlikely situation where the port is Firmware-First >>> AER and Native DPC, and a BIOS or switch firmware preconfiguration of >>> the DPC trigger could result in unhandled DPC events. >>> >>> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> >>> --- >>> drivers/pci/pcie/dpc.c | 3 ++- >>> drivers/pci/pcie/portdrv_core.c | 3 ++- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c >>> index 7621704..3f3106f 100644 >>> --- a/drivers/pci/pcie/dpc.c >>> +++ b/drivers/pci/pcie/dpc.c >>> @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) >>> int status; >>> u16 ctl, cap; >>> >>> - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) >>> + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && >>> + !pci_find_host_bridge(pdev->bus)->native_dpc) >> Why do it in probe as well ? if host->native_dpc is not set then the >> device DPC probe it self won't happen right ? > > Portdrv only enables the interrupt and allows the probe to occur. Please check the following snippet of code (from portdrv_core.c). IIUC, pcie_device_init() will not be called if PCIE_PORT_SERVICE_DPC is not set in capabilities. Your change in portdrv_core.c already selectively enables the PCIE_PORT_SERVICE_DPC service based on native_dpc value. So IMO, adding native_dpc check in dpc_probe() is redundant. int pcie_port_device_register(struct pci_dev *dev) /* Allocate child services if any */ status = -ENODEV; nr_service = 0; for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) { int service = 1 << i; if (!(capabilities & service)) continue; if (!pcie_device_init(dev, service, irqs[i])) nr_service++; } > > The probe itself will still fail if there's a mixed-mode _OSC > negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1 > for AER and no check for DPC. > > I don't know if such a platform will exist, but the kernel is already > wired for 'dpc-native' so it makes sense to extend it for this.. > > This transform might be more readable: > if (pcie_aer_get_firmware_first(pdev) && > !(pcie_ports_dpc_native || hb->native_dpc)) > > > >>> return -ENOTSUPP; >>> >>> status = devm_request_threaded_irq(device, dev->irq, dpc_irq, >>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >>> index 50a9522..f2139a1 100644 >>> --- a/drivers/pci/pcie/portdrv_core.c >>> +++ b/drivers/pci/pcie/portdrv_core.c >>> @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev) >>> */ >>> if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && >>> pci_aer_available() && >>> - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) >>> + (pcie_ports_dpc_native || host->native_dpc || >>> + (services & PCIE_PORT_SERVICE_AER))) >>> services |= PCIE_PORT_SERVICE_DPC; >>> >>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || >>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC 2020-04-25 20:46 ` Kuppuswamy, Sathyanarayanan @ 2020-04-27 15:15 ` Derrick, Jonathan 2020-04-27 15:43 ` Kuppuswamy, Sathyanarayanan 0 siblings, 1 reply; 13+ messages in thread From: Derrick, Jonathan @ 2020-04-27 15:15 UTC (permalink / raw) To: sathyanarayanan.kuppuswamy, helgaas Cc: rajatja, fred, ruscur, kbusch, oohall, alex.williamson, linux-kernel, sbobroff, olof, Wysocki, Rafael J, mika.westerberg, linuxppc-dev, linux-pci, Patel, Mayurkumar, andriy.shevchenko, bhelgaas Hi Sathyanarayanan, On Sat, 2020-04-25 at 13:46 -0700, Kuppuswamy, Sathyanarayanan wrote: > > On 4/23/20 8:11 AM, Derrick, Jonathan wrote: > > Hi Sathyanarayanan, > > > > On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote: > > > On 4/20/20 2:37 PM, Jon Derrick wrote: > > > > The existing portdrv model prevents DPC services without either OS > > > > control (_OSC) granted to AER services, a Host Bridge requesting Native > > > > AER, or using one of the 'pcie_ports=' parameters of 'native' or > > > > 'dpc-native'. > > > > > > > > The DPC port service driver itself will also fail to probe if the kernel > > > > assumes the port is using Firmware-First AER. It's a reasonable > > > > expectation that a port using Firmware-First AER will also be using > > > > Firmware-First DPC, however if a Host Bridge requests Native DPC, the > > > > DPC driver should allow it and not fail to bind due to AER capability > > > > settings. > > > > > > > > Host Bridges which request Native DPC port services will also likely > > > > request Native AER, however it shouldn't be a requirement. This patch > > > > allows ports on those Host Bridges to have DPC port services. > > > > > > > > This will avoid the unlikely situation where the port is Firmware-First > > > > AER and Native DPC, and a BIOS or switch firmware preconfiguration of > > > > the DPC trigger could result in unhandled DPC events. > > > > > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > > > --- > > > > drivers/pci/pcie/dpc.c | 3 ++- > > > > drivers/pci/pcie/portdrv_core.c | 3 ++- > > > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > > > > index 7621704..3f3106f 100644 > > > > --- a/drivers/pci/pcie/dpc.c > > > > +++ b/drivers/pci/pcie/dpc.c > > > > @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) > > > > int status; > > > > u16 ctl, cap; > > > > > > > > - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) > > > > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && > > > > + !pci_find_host_bridge(pdev->bus)->native_dpc) > > > Why do it in probe as well ? if host->native_dpc is not set then the > > > device DPC probe it self won't happen right ? > > > > Portdrv only enables the interrupt and allows the probe to occur. > > Please check the following snippet of code (from portdrv_core.c). > > IIUC, pcie_device_init() will not be called if PCIE_PORT_SERVICE_DPC is > not set in capabilities. Your change in portdrv_core.c already > selectively enables the PCIE_PORT_SERVICE_DPC service based on > native_dpc value. > That's right. So pcie_device_init registers the port service driver allowing the services enumeration to occur. > So IMO, adding native_dpc check in dpc_probe() is redundant. > > int pcie_port_device_register(struct pci_dev *dev) > /* Allocate child services if any */ > status = -ENODEV; > nr_service = 0; > for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) { > int service = 1 << i; > if (!(capabilities & service)) > continue; > if (!pcie_device_init(dev, service, irqs[i])) > nr_service++; > } > This is the tricky part There's still a check in dpc_probe for AER FFS or pcie_ports=dpc- native: if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) return -ENOTSUPP; One option is to move that to get_port_device_capability and remove the dpc_probe check > > The probe itself will still fail if there's a mixed-mode _OSC > > negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1 > > for AER and no check for DPC. > > > > I don't know if such a platform will exist, but the kernel is already > > wired for 'dpc-native' so it makes sense to extend it for this.. > > > > This transform might be more readable: > > if (pcie_aer_get_firmware_first(pdev) && > > !(pcie_ports_dpc_native || hb->native_dpc)) > > > > > > > > > > return -ENOTSUPP; > > > > > > > > status = devm_request_threaded_irq(device, dev->irq, dpc_irq, > > > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > > > index 50a9522..f2139a1 100644 > > > > --- a/drivers/pci/pcie/portdrv_core.c > > > > +++ b/drivers/pci/pcie/portdrv_core.c > > > > @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev) > > > > */ > > > > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && > > > > pci_aer_available() && > > > > - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) > > > > + (pcie_ports_dpc_native || host->native_dpc || > > > > + (services & PCIE_PORT_SERVICE_AER))) > > > > services |= PCIE_PORT_SERVICE_DPC; > > > > > > > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC 2020-04-27 15:15 ` Derrick, Jonathan @ 2020-04-27 15:43 ` Kuppuswamy, Sathyanarayanan 0 siblings, 0 replies; 13+ messages in thread From: Kuppuswamy, Sathyanarayanan @ 2020-04-27 15:43 UTC (permalink / raw) To: Derrick, Jonathan, helgaas Cc: rajatja, fred, ruscur, kbusch, oohall, alex.williamson, linux-kernel, sbobroff, olof, Wysocki, Rafael J, mika.westerberg, linuxppc-dev, linux-pci, Patel, Mayurkumar, andriy.shevchenko, bhelgaas On 4/27/20 8:15 AM, Derrick, Jonathan wrote: > Hi Sathyanarayanan, > > On Sat, 2020-04-25 at 13:46 -0700, Kuppuswamy, Sathyanarayanan wrote: >> >> On 4/23/20 8:11 AM, Derrick, Jonathan wrote: >>> Hi Sathyanarayanan, >>> >>> On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote: >>>> On 4/20/20 2:37 PM, Jon Derrick wrote: >>>>> The existing portdrv model prevents DPC services without either OS >>>>> control (_OSC) granted to AER services, a Host Bridge requesting Native >>>>> AER, or using one of the 'pcie_ports=' parameters of 'native' or >>>>> 'dpc-native'. >>>>> >>>>> The DPC port service driver itself will also fail to probe if the kernel >>>>> assumes the port is using Firmware-First AER. It's a reasonable >>>>> expectation that a port using Firmware-First AER will also be using >>>>> Firmware-First DPC, however if a Host Bridge requests Native DPC, the >>>>> DPC driver should allow it and not fail to bind due to AER capability >>>>> settings. >>>>> >>>>> Host Bridges which request Native DPC port services will also likely >>>>> request Native AER, however it shouldn't be a requirement. This patch >>>>> allows ports on those Host Bridges to have DPC port services. >>>>> >>>>> This will avoid the unlikely situation where the port is Firmware-First >>>>> AER and Native DPC, and a BIOS or switch firmware preconfiguration of >>>>> the DPC trigger could result in unhandled DPC events. >>>>> >>>>> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> >>>>> --- >>>>> drivers/pci/pcie/dpc.c | 3 ++- >>>>> drivers/pci/pcie/portdrv_core.c | 3 ++- >>>>> 2 files changed, 4 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c >>>>> index 7621704..3f3106f 100644 >>>>> --- a/drivers/pci/pcie/dpc.c >>>>> +++ b/drivers/pci/pcie/dpc.c >>>>> @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev) >>>>> int status; >>>>> u16 ctl, cap; >>>>> >>>>> - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) >>>>> + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native && >>>>> + !pci_find_host_bridge(pdev->bus)->native_dpc) >>>> Why do it in probe as well ? if host->native_dpc is not set then the >>>> device DPC probe it self won't happen right ? >>> >>> Portdrv only enables the interrupt and allows the probe to occur. >> >> Please check the following snippet of code (from portdrv_core.c). >> >> IIUC, pcie_device_init() will not be called if PCIE_PORT_SERVICE_DPC is >> not set in capabilities. Your change in portdrv_core.c already >> selectively enables the PCIE_PORT_SERVICE_DPC service based on >> native_dpc value. >> > That's right. So pcie_device_init registers the port service driver > allowing the services enumeration to occur. > >> So IMO, adding native_dpc check in dpc_probe() is redundant. >> >> int pcie_port_device_register(struct pci_dev *dev) >> /* Allocate child services if any */ >> status = -ENODEV; >> nr_service = 0; >> for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) { >> int service = 1 << i; >> if (!(capabilities & service)) >> continue; >> if (!pcie_device_init(dev, service, irqs[i])) >> nr_service++; >> } >> > This is the tricky part > There's still a check in dpc_probe for AER FFS or pcie_ports=dpc- > native: > > if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) > return -ENOTSUPP; > > One option is to move that to get_port_device_capability and remove the > dpc_probe check Yes, its better to group them together in get_port_device_capability(). But it should be done in a separate patch. > >>> The probe itself will still fail if there's a mixed-mode _OSC >>> negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1 >>> for AER and no check for DPC. >>> >>> I don't know if such a platform will exist, but the kernel is already >>> wired for 'dpc-native' so it makes sense to extend it for this.. >>> >>> This transform might be more readable: >>> if (pcie_aer_get_firmware_first(pdev) && >>> !(pcie_ports_dpc_native || hb->native_dpc)) >>> >>> >>> >>>>> return -ENOTSUPP; >>>>> >>>>> status = devm_request_threaded_irq(device, dev->irq, dpc_irq, >>>>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c >>>>> index 50a9522..f2139a1 100644 >>>>> --- a/drivers/pci/pcie/portdrv_core.c >>>>> +++ b/drivers/pci/pcie/portdrv_core.c >>>>> @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev) >>>>> */ >>>>> if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && >>>>> pci_aer_available() && >>>>> - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) >>>>> + (pcie_ports_dpc_native || host->native_dpc || >>>>> + (services & PCIE_PORT_SERVICE_AER))) >>>>> services |= PCIE_PORT_SERVICE_DPC; >>>>> >>>>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || >>>>> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-04-27 22:14 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-20 21:37 [PATCH v2 0/2] Honoring Native AER/DPC Host Bridges Jon Derrick 2020-04-20 21:37 ` [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER Jon Derrick 2020-04-22 22:48 ` Kuppuswamy, Sathyanarayanan 2020-04-23 15:11 ` Derrick, Jonathan 2020-04-24 23:30 ` Bjorn Helgaas 2020-04-27 16:11 ` Derrick, Jonathan 2020-04-27 22:14 ` Bjorn Helgaas 2020-04-20 21:37 ` [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC Jon Derrick 2020-04-22 22:50 ` Kuppuswamy, Sathyanarayanan 2020-04-23 15:11 ` Derrick, Jonathan 2020-04-25 20:46 ` Kuppuswamy, Sathyanarayanan 2020-04-27 15:15 ` Derrick, Jonathan 2020-04-27 15:43 ` Kuppuswamy, Sathyanarayanan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).