| Message ID | 20180818065126.77912-2-okaya@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v8,1/2] PCI: pciehp: Ignore link events when there is a fatal error pending | expand |
On 8/18/2018 2:51 AM, Sinan Kaya wrote: > cleanup_slot(ctrl); > + pciehp_control_surprise_error(ctrl, true); I think I need to move this one line up but I'd like to see some input here and also ask for some testing. I don't have any hardware to test.
Hi Sinan,
I love your patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on next-20180817]
[cannot apply to v4.18]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sinan-Kaya/PCI-pciehp-Ignore-link-events-when-there-is-a-fatal-error-pending/20180820-074636
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-randconfig-x075-201833 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/pci/hotplug/pciehp_core.c: In function 'pciehp_control_surprise_error':
>> drivers/pci/hotplug/pciehp_core.c:241:14: error: 'struct pci_dev' has no member named 'aer_cap'; did you mean 'ats_cap'?
pos = pdev->aer_cap;
^~~~~~~
ats_cap
vim +241 drivers/pci/hotplug/pciehp_core.c
231
232 static int pciehp_control_surprise_error(struct controller *ctrl, bool enable)
233 {
234 struct pci_dev *pdev = ctrl->pcie->port;
235 u32 reg32;
236 int pos;
237
238 if (!pci_is_pcie(pdev))
239 return -ENODEV;
240
> 241 pos = pdev->aer_cap;
242 if (!pos)
243 return -ENODEV;
244
245 pci_read_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, ®32);
246 if (enable)
247 reg32 &= ~PCI_ERR_UNC_SURPDN;
248 else
249 reg32 |= PCI_ERR_UNC_SURPDN;
250 pci_write_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, reg32);
251
252 return 0;
253 }
254
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Aug 17, 2018 at 11:51:10PM -0700, Sinan Kaya wrote: > +static int pciehp_control_surprise_error(struct controller *ctrl, bool enable) The return value isn't checked, so this could return void. > @@ -280,6 +303,9 @@ static int pciehp_probe(struct pcie_device *dev) > > pciehp_check_presence(ctrl); > > + /* We want exclusive control of link down events in hotplug driver */ > + pciehp_control_surprise_error(ctrl, false); > + > return 0; Hm, if the platform firmware hasn't granted native hotplug control to OSPM, or if some other hotplug driver than pciehp is used, shouldn't surprise down be ignored by error recovery as well? If yes, the mask would have to be set in generic code somewhere in drivers/pci/probe.c I guess, based on the is_hotplug_bridge bit in struct pci_dev. (Interestingly, PCI_ERR_UNCOR_MASK is already changed in probe.c by program_hpp_type2(). That seems to be ACPI-specific code, which kind of begs the question why it's not in pci-acpi.c?) Thanks, Lukas
On 8/20/2018 4:21 AM, Lukas Wunner wrote: > On Fri, Aug 17, 2018 at 11:51:10PM -0700, Sinan Kaya wrote: >> +static int pciehp_control_surprise_error(struct controller *ctrl, bool enable) > > The return value isn't checked, so this could return void. > Sure, I can do that. > >> @@ -280,6 +303,9 @@ static int pciehp_probe(struct pcie_device *dev) >> >> pciehp_check_presence(ctrl); >> >> + /* We want exclusive control of link down events in hotplug driver */ >> + pciehp_control_surprise_error(ctrl, false); >> + >> return 0; > > Hm, if the platform firmware hasn't granted native hotplug control to OSPM, > or if some other hotplug driver than pciehp is used, shouldn't surprise down > be ignored by error recovery as well? If yes, the mask would have to be set > in generic code somewhere in drivers/pci/probe.c I guess, based on the > is_hotplug_bridge bit in struct pci_dev. I could move this code if we know that is_hotplug_bridge flag is set regardless of OS hotplug driver control or not. > > (Interestingly, PCI_ERR_UNCOR_MASK is already changed in probe.c by > program_hpp_type2(). That seems to be ACPI-specific code, which kind > of begs the question why it's not in pci-acpi.c?) Yes, you can tell the OS what AER mask to set following hotplug insertion via ACPI HPP table especially if you remove a hotplug bridge. This is used during ACPI hotplug. > > Thanks, > > Lukas >
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index ec48c9433ae5..8322db8f369a 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -229,6 +229,29 @@ static void pciehp_check_presence(struct controller *ctrl) up_read(&ctrl->reset_lock); } +static int pciehp_control_surprise_error(struct controller *ctrl, bool enable) +{ + struct pci_dev *pdev = ctrl->pcie->port; + u32 reg32; + int pos; + + if (!pci_is_pcie(pdev)) + return -ENODEV; + + pos = pdev->aer_cap; + if (!pos) + return -ENODEV; + + pci_read_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, ®32); + if (enable) + reg32 &= ~PCI_ERR_UNC_SURPDN; + else + reg32 |= PCI_ERR_UNC_SURPDN; + pci_write_config_dword(pdev, pos + PCI_ERR_UNCOR_MASK, reg32); + + return 0; +} + static int pciehp_probe(struct pcie_device *dev) { int rc; @@ -280,6 +303,9 @@ static int pciehp_probe(struct pcie_device *dev) pciehp_check_presence(ctrl); + /* We want exclusive control of link down events in hotplug driver */ + pciehp_control_surprise_error(ctrl, false); + return 0; err_out_shutdown_notification: @@ -298,6 +324,7 @@ static void pciehp_remove(struct pcie_device *dev) pci_hp_del(ctrl->slot->hotplug_slot); pcie_shutdown_notification(ctrl); cleanup_slot(ctrl); + pciehp_control_surprise_error(ctrl, true); pciehp_release_ctrl(ctrl); }
PCIe Spec 3.0. 7.10.2. Uncorrectable Error Status Register (Offset 04h) defines link down errors as an AER error as bit 5 Surprise Down Error Status. If hotplug is supported by a particular port, we want hotplug driver to handle the link down/up conditions via Data Link Layer Active interrupt rather than the AER error interrupt. Mask the Surprise Down Error during hotplug driver and re-enable it during remove. Signed-off-by: Sinan Kaya <okaya@kernel.org> --- drivers/pci/hotplug/pciehp_core.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)