Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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, );
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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, );
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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.
Re: [PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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.
[PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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
---
drivers/pci/hotplug/pciehp_core.c | 27 +++
1 file changed, 27 insertions(+)
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(>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, );
+ 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);
}
--
2.17.1
[PATCH v8 2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled
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
---
drivers/pci/hotplug/pciehp_core.c | 27 +++
1 file changed, 27 insertions(+)
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(>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, );
+ 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);
}
--
2.17.1
