| Message ID | 20191023192205.97024-1-olof@lixom.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | PCI/DPC: Add pcie_ports=dpc-native parameter to bring back old behavior | expand |
On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: > In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), > the behavior was changed such that native (kernel) handling of DPC > got tied to whether the kernel also handled AER. While this is what > the standard recommends, there are BIOSes out there that lack the DPC > handling since it was never required in the past. > > To make DPC still work on said platforms the same way they did before, > add a "pcie_ports=dpc-native" kernel parameter that can be passed in > if needed, while keeping defaults unchanged. If platform firmware wants to handle AER events, but the kernel enables the DPC capability, the ports will be trapping events that firmware is expecting to handle. Not that that's a bad thing: firmware is generally worse at handling these errors. > +/* > + * If the user specified "pcie_ports=dpc-native", use the PCIe services > + * for DPC, but cuse platform defaults for the others. s/cuse/use > @@ -1534,9 +1534,11 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, > #ifdef CONFIG_PCIEPORTBUS > extern bool pcie_ports_disabled; > extern bool pcie_ports_native; > +extern bool pcie_ports_dpc_native; > #else > #define pcie_ports_disabled true > #define pcie_ports_native false > +#define pcie_ports_dpc_native false > #endif You do not have any references to pcie_ports_dpc_native outside of files that require CONFIG_PCIEPORTBUS, so no need to define a default.
On Wed, Oct 23, 2019 at 7:37 PM Keith Busch <kbusch@kernel.org> wrote: > > On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: > > In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), > > the behavior was changed such that native (kernel) handling of DPC > > got tied to whether the kernel also handled AER. While this is what > > the standard recommends, there are BIOSes out there that lack the DPC > > handling since it was never required in the past. > > > > To make DPC still work on said platforms the same way they did before, > > add a "pcie_ports=dpc-native" kernel parameter that can be passed in > > if needed, while keeping defaults unchanged. > > If platform firmware wants to handle AER events, but the kernel enables > the DPC capability, the ports will be trapping events that firmware is > expecting to handle. Not that that's a bad thing: firmware is generally > worse at handling these errors. Right, and in particular (and what I'm looking for here): It brings back the older behavior that some platforms rely on. :-/ > > +/* > > + * If the user specified "pcie_ports=dpc-native", use the PCIe services > > + * for DPC, but cuse platform defaults for the others. > > s/cuse/use Thanks > > @@ -1534,9 +1534,11 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, > > #ifdef CONFIG_PCIEPORTBUS > > extern bool pcie_ports_disabled; > > extern bool pcie_ports_native; > > +extern bool pcie_ports_dpc_native; > > #else > > #define pcie_ports_disabled true > > #define pcie_ports_native false > > +#define pcie_ports_dpc_native false > > #endif > > You do not have any references to pcie_ports_dpc_native outside of files that > require CONFIG_PCIEPORTBUS, so no need to define a default. If these are the only comments, maybe Bjorn can fixup when applying. Bjorn; let me know if you prefer that or if you want a fresh version. Either is fine with me. -Olof
On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: > In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), > the behavior was changed such that native (kernel) handling of DPC > got tied to whether the kernel also handled AER. While this is what > the standard recommends, there are BIOSes out there that lack the DPC > handling since it was never required in the past. Some systems do not grant OS control of AER via _OSC. I guess the problem is that on those systems, the OS DPC driver used to work, but after eed85ff4c0da7, it does not. Right? We should also update negotiate_os_control() to request control of DPC via _OSC. Kuppuswamy's patch [1] does that but hasn't been merged yet. That will conflict with this, but I can resolve that. I applied this as below (with the nits Keith noticed) to pci/aer for v5.5, thanks! [1] https://lore.kernel.org/r/b638cbd3e122b4c7a58b949d7224230d2c4b34d4.1570145778.git.sathyanarayanan.kuppuswamy@linux.intel.com commit 35a0b2378c19 Author: Olof Johansson <olof@lixom.net> Date: Wed Oct 23 12:22:05 2019 -0700 PCI/DPC: Add "pcie_ports=dpc-native" to allow DPC without AER control Prior to eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), Linux handled DPC events regardless of whether firmware had granted it ownership of AER or DPC, e.g., via _OSC. PCIe r5.0, sec 6.2.10, recommends that the OS link control of DPC to control of AER, so after eed85ff4c0da7, Linux handles DPC events only if it has control of AER. On platforms that do not grant OS control of AER via _OSC, Linux DPC handling worked before eed85ff4c0da7 but not after. To make Linux DPC handling work on those platforms the same way they did before, add a "pcie_ports=dpc-native" kernel parameter that makes Linux handle DPC events regardless of whether it has control of AER. [bhelgaas: commit log, move pcie_ports_dpc_native to drivers/pci/] Link: https://lore.kernel.org/r/20191023192205.97024-1-olof@lixom.net Signed-off-by: Olof Johansson <olof@lixom.net> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index c7ac2f3ac99f..806c89f79be8 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -3540,6 +3540,8 @@ even if the platform doesn't give the OS permission to use them. This may cause conflicts if the platform also tries to use these services. + dpc-native Use native PCIe service for DPC only. May + cause conflicts if firmware uses AER or DPC. compat Disable native PCIe services (PME, AER, DPC, PCIe hotplug). diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index a32ec3487a8d..e06f42f58d3d 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -291,7 +291,7 @@ static int dpc_probe(struct pcie_device *dev) int status; u16 ctl, cap; - if (pcie_aer_get_firmware_first(pdev)) + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) return -ENOTSUPP; dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL); diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h index 944827a8c7d3..1e673619b101 100644 --- a/drivers/pci/pcie/portdrv.h +++ b/drivers/pci/pcie/portdrv.h @@ -25,6 +25,8 @@ #define PCIE_PORT_DEVICE_MAXSERVICES 5 +extern bool pcie_ports_dpc_native; + #ifdef CONFIG_PCIEAER int pcie_aer_init(void); #else diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 1b330129089f..5075cb9e850c 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -250,8 +250,13 @@ static int get_port_device_capability(struct pci_dev *dev) pcie_pme_interrupt_enable(dev, false); } + /* + * With dpc-native, allow Linux to use DPC even if it doesn't have + * permission to use AER. + */ if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && - pci_aer_available() && services & PCIE_PORT_SERVICE_AER) + pci_aer_available() && + (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) services |= PCIE_PORT_SERVICE_DPC; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c index 0a87091a0800..160d67c59310 100644 --- a/drivers/pci/pcie/portdrv_pci.c +++ b/drivers/pci/pcie/portdrv_pci.c @@ -29,12 +29,20 @@ bool pcie_ports_disabled; */ bool pcie_ports_native; +/* + * If the user specified "pcie_ports=dpc-native", use the Linux DPC PCIe + * service even if the platform hasn't given us permission. + */ +bool pcie_ports_dpc_native; + static int __init pcie_port_setup(char *str) { if (!strncmp(str, "compat", 6)) pcie_ports_disabled = true; else if (!strncmp(str, "native", 6)) pcie_ports_native = true; + else if (!strncmp(str, "dpc-native", 10)) + pcie_ports_dpc_native = true; return 1; }
Hi, On 10/25/19 1:20 PM, Bjorn Helgaas wrote: > On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: >> In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), >> the behavior was changed such that native (kernel) handling of DPC >> got tied to whether the kernel also handled AER. While this is what >> the standard recommends, there are BIOSes out there that lack the DPC >> handling since it was never required in the past. > Some systems do not grant OS control of AER via _OSC. I guess the > problem is that on those systems, the OS DPC driver used to work, but > after eed85ff4c0da7, it does not. Right? I need some clarification on this issue. Do you mean in these systems, firmware expects OS to handle DPC and AER, but it does not let OS know about it via _OSC ? > > We should also update negotiate_os_control() to request control of DPC > via _OSC. Kuppuswamy's patch [1] does that but hasn't been merged > yet. That will conflict with this, but I can resolve that. > > I applied this as below (with the nits Keith noticed) to pci/aer for > v5.5, thanks! > > [1] https://lore.kernel.org/r/b638cbd3e122b4c7a58b949d7224230d2c4b34d4.1570145778.git.sathyanarayanan.kuppuswamy@linux.intel.com > > commit 35a0b2378c19 > Author: Olof Johansson <olof@lixom.net> > Date: Wed Oct 23 12:22:05 2019 -0700 > > PCI/DPC: Add "pcie_ports=dpc-native" to allow DPC without AER control > > Prior to eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), > Linux handled DPC events regardless of whether firmware had granted it > ownership of AER or DPC, e.g., via _OSC. > > PCIe r5.0, sec 6.2.10, recommends that the OS link control of DPC to > control of AER, so after eed85ff4c0da7, Linux handles DPC events only if it > has control of AER. > > On platforms that do not grant OS control of AER via _OSC, Linux DPC > handling worked before eed85ff4c0da7 but not after. > > To make Linux DPC handling work on those platforms the same way they did > before, add a "pcie_ports=dpc-native" kernel parameter that makes Linux > handle DPC events regardless of whether it has control of AER. > > [bhelgaas: commit log, move pcie_ports_dpc_native to drivers/pci/] > Link: https://lore.kernel.org/r/20191023192205.97024-1-olof@lixom.net > Signed-off-by: Olof Johansson <olof@lixom.net> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index c7ac2f3ac99f..806c89f79be8 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -3540,6 +3540,8 @@ > even if the platform doesn't give the OS permission to > use them. This may cause conflicts if the platform > also tries to use these services. > + dpc-native Use native PCIe service for DPC only. May > + cause conflicts if firmware uses AER or DPC. > compat Disable native PCIe services (PME, AER, DPC, PCIe > hotplug). > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c > index a32ec3487a8d..e06f42f58d3d 100644 > --- a/drivers/pci/pcie/dpc.c > +++ b/drivers/pci/pcie/dpc.c > @@ -291,7 +291,7 @@ static int dpc_probe(struct pcie_device *dev) > int status; > u16 ctl, cap; > > - if (pcie_aer_get_firmware_first(pdev)) > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) > return -ENOTSUPP; > > dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL); > diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h > index 944827a8c7d3..1e673619b101 100644 > --- a/drivers/pci/pcie/portdrv.h > +++ b/drivers/pci/pcie/portdrv.h > @@ -25,6 +25,8 @@ > > #define PCIE_PORT_DEVICE_MAXSERVICES 5 > > +extern bool pcie_ports_dpc_native; > + > #ifdef CONFIG_PCIEAER > int pcie_aer_init(void); > #else > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index 1b330129089f..5075cb9e850c 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -250,8 +250,13 @@ static int get_port_device_capability(struct pci_dev *dev) > pcie_pme_interrupt_enable(dev, false); > } > > + /* > + * With dpc-native, allow Linux to use DPC even if it doesn't have > + * permission to use AER. > + */ > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && > - pci_aer_available() && services & PCIE_PORT_SERVICE_AER) > + pci_aer_available() && > + (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) > services |= PCIE_PORT_SERVICE_DPC; > > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c > index 0a87091a0800..160d67c59310 100644 > --- a/drivers/pci/pcie/portdrv_pci.c > +++ b/drivers/pci/pcie/portdrv_pci.c > @@ -29,12 +29,20 @@ bool pcie_ports_disabled; > */ > bool pcie_ports_native; > > +/* > + * If the user specified "pcie_ports=dpc-native", use the Linux DPC PCIe > + * service even if the platform hasn't given us permission. > + */ > +bool pcie_ports_dpc_native; > + > static int __init pcie_port_setup(char *str) > { > if (!strncmp(str, "compat", 6)) > pcie_ports_disabled = true; > else if (!strncmp(str, "native", 6)) > pcie_ports_native = true; > + else if (!strncmp(str, "dpc-native", 10)) > + pcie_ports_dpc_native = true; > > return 1; > } >
On Thu, Nov 7, 2019 at 12:02 PM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > Hi, > > On 10/25/19 1:20 PM, Bjorn Helgaas wrote: > > On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: > >> In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), > >> the behavior was changed such that native (kernel) handling of DPC > >> got tied to whether the kernel also handled AER. While this is what > >> the standard recommends, there are BIOSes out there that lack the DPC > >> handling since it was never required in the past. > > Some systems do not grant OS control of AER via _OSC. I guess the > > problem is that on those systems, the OS DPC driver used to work, but > > after eed85ff4c0da7, it does not. Right? > > I need some clarification on this issue. Do you mean in these systems, > firmware expects OS to handle DPC and AER, but it does not let OS know > about it via _OSC ? The OS and BIOS both assumed behavior as before eed85ff4c0da7 -- AER handled by firmware but DPC handled by kernel. I.e. a classic case of "Sure, the spec says this, but in reality implementations relied on actual behavior", and someone had a regression and need a way to restore previous behavior. -Olof
On 11/7/19 12:09 PM, Olof Johansson wrote: > On Thu, Nov 7, 2019 at 12:02 PM Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> Hi, >> >> On 10/25/19 1:20 PM, Bjorn Helgaas wrote: >>> On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: >>>> In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), >>>> the behavior was changed such that native (kernel) handling of DPC >>>> got tied to whether the kernel also handled AER. While this is what >>>> the standard recommends, there are BIOSes out there that lack the DPC >>>> handling since it was never required in the past. >>> Some systems do not grant OS control of AER via _OSC. I guess the >>> problem is that on those systems, the OS DPC driver used to work, but >>> after eed85ff4c0da7, it does not. Right? >> I need some clarification on this issue. Do you mean in these systems, >> firmware expects OS to handle DPC and AER, but it does not let OS know >> about it via _OSC ? > The OS and BIOS both assumed behavior as before eed85ff4c0da7 -- AER > handled by firmware but DPC handled by kernel. > > I.e. a classic case of "Sure, the spec says this, but in reality > implementations relied on actual behavior", and someone had a > regression and need a way to restore previous behavior. Got it. I don't know whether its good to add hacks to support products that does not follow spec. But, do you think it would be useful to add some kind of warning message when this option is enabled ? May be it could be useful in debugging. > > -Olof >
On Thu, Nov 7, 2019 at 2:07 PM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > On 11/7/19 12:09 PM, Olof Johansson wrote: > > On Thu, Nov 7, 2019 at 12:02 PM Kuppuswamy Sathyanarayanan > > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > >> Hi, > >> > >> On 10/25/19 1:20 PM, Bjorn Helgaas wrote: > >>> On Wed, Oct 23, 2019 at 12:22:05PM -0700, Olof Johansson wrote: > >>>> In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), > >>>> the behavior was changed such that native (kernel) handling of DPC > >>>> got tied to whether the kernel also handled AER. While this is what > >>>> the standard recommends, there are BIOSes out there that lack the DPC > >>>> handling since it was never required in the past. > >>> Some systems do not grant OS control of AER via _OSC. I guess the > >>> problem is that on those systems, the OS DPC driver used to work, but > >>> after eed85ff4c0da7, it does not. Right? > >> I need some clarification on this issue. Do you mean in these systems, > >> firmware expects OS to handle DPC and AER, but it does not let OS know > >> about it via _OSC ? > > The OS and BIOS both assumed behavior as before eed85ff4c0da7 -- AER > > handled by firmware but DPC handled by kernel. > > > > I.e. a classic case of "Sure, the spec says this, but in reality > > implementations relied on actual behavior", and someone had a > > regression and need a way to restore previous behavior. > > Got it. I don't know whether its good to add hacks to support products > that does not follow spec. > But, do you think it would be useful to add some kind of warning message > when this option is > enabled ? May be it could be useful in debugging. It's not a "hack", it is fixing a regression in behavior because of changed assumptions by the kernel. We're pretty clear as a kernel community: We don't regress our users. So, in cases like these, we need to make sure we allow people to use their hardware the same way they used it with an older kernel. A printk() to indicate that this mode is enabled could be useful, if nothing else to make sure that the pre-eed85ff4c0da7 behavior is enabled without having to grep /proc/cmdline. -Olof
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 5e27d74e2b74b..e0325421980aa 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -3548,6 +3548,7 @@ even if the platform doesn't give the OS permission to use them. This may cause conflicts if the platform also tries to use these services. + dpc-native Use native PCIe service for DPC, but none other. compat Disable native PCIe services (PME, AER, DPC, PCIe hotplug). diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index a32ec3487a8d0..e06f42f58d3d4 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -291,7 +291,7 @@ static int dpc_probe(struct pcie_device *dev) int status; u16 ctl, cap; - if (pcie_aer_get_firmware_first(pdev)) + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native) return -ENOTSUPP; dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL); diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 1b330129089fe..c24bf6cac4186 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -250,8 +250,13 @@ static int get_port_device_capability(struct pci_dev *dev) pcie_pme_interrupt_enable(dev, false); } + /* + * With dpc-native, we set it if AER is available, even if AER is + * handled by firmware. + */ if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) && - pci_aer_available() && services & PCIE_PORT_SERVICE_AER) + pci_aer_available() && + (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER))) services |= PCIE_PORT_SERVICE_DPC; if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM || diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c index 0a87091a0800e..b415656519a73 100644 --- a/drivers/pci/pcie/portdrv_pci.c +++ b/drivers/pci/pcie/portdrv_pci.c @@ -29,12 +29,20 @@ bool pcie_ports_disabled; */ bool pcie_ports_native; +/* + * If the user specified "pcie_ports=dpc-native", use the PCIe services + * for DPC, but cuse platform defaults for the others. + */ +bool pcie_ports_dpc_native; + static int __init pcie_port_setup(char *str) { if (!strncmp(str, "compat", 6)) pcie_ports_disabled = true; else if (!strncmp(str, "native", 6)) pcie_ports_native = true; + else if (!strncmp(str, "dpc-native", 10)) + pcie_ports_dpc_native = true; return 1; } diff --git a/include/linux/pci.h b/include/linux/pci.h index fc1844061e88f..603d4822757b6 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1534,9 +1534,11 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, #ifdef CONFIG_PCIEPORTBUS extern bool pcie_ports_disabled; extern bool pcie_ports_native; +extern bool pcie_ports_dpc_native; #else #define pcie_ports_disabled true #define pcie_ports_native false +#define pcie_ports_dpc_native false #endif #define PCIE_LINK_STATE_L0S BIT(0)
In commit eed85ff4c0da7 ("PCI/DPC: Enable DPC only if AER is available"), the behavior was changed such that native (kernel) handling of DPC got tied to whether the kernel also handled AER. While this is what the standard recommends, there are BIOSes out there that lack the DPC handling since it was never required in the past. To make DPC still work on said platforms the same way they did before, add a "pcie_ports=dpc-native" kernel parameter that can be passed in if needed, while keeping defaults unchanged. Signed-off-by: Olof Johansson <olof@lixom.net> --- Documentation/admin-guide/kernel-parameters.txt | 1 + drivers/pci/pcie/dpc.c | 2 +- drivers/pci/pcie/portdrv_core.c | 7 ++++++- drivers/pci/pcie/portdrv_pci.c | 8 ++++++++ include/linux/pci.h | 2 ++ 5 files changed, 18 insertions(+), 2 deletions(-)