diff mbox series

[v8,2/2] PCI: pciehp: Mask AER surprise link down error if hotplug is enabled

Message ID 20180818065126.77912-2-okaya@kernel.org (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series [v8,1/2] PCI: pciehp: Ignore link events when there is a fatal error pending | expand

Commit Message

Sinan Kaya Aug. 18, 2018, 6:51 a.m. UTC 
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(+)

Comments

Sinan Kaya Aug. 19, 2018, 11:23 p.m. UTC | #1 
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.
kernel test robot Aug. 20, 2018, 12:20 a.m. UTC | #2 
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, &reg32);
   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
Lukas Wunner Aug. 20, 2018, 8:21 a.m. UTC | #3 
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
Sinan Kaya Aug. 20, 2018, 5:43 p.m. UTC | #4 
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 mbox series

Patch

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, &reg32);
+	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);
 }