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

2018-08-20 Thread Sinan Kaya 

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

2018-08-20 Thread Lukas Wunner 
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

2018-08-19 Thread kbuild test robot 
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 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

2018-08-19 Thread Sinan Kaya 

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

2018-08-19 Thread Sinan Kaya 
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(&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);
 }
 
-- 
2.17.1