Hi Bjorn
Many thanks for your review.
在 2017/12/14 0:29, Bjorn Helgaas 写道:
On Tue, Dec 05, 2017 at 05:50:37PM +0800, Dongdong Liu wrote:
Current code has a bug, switch upstream/downstream port error report
is disabled.
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
We call aer_probe() for a root port, and it enables AER reporting for
the root port and any downstream devices:
aer_probe(root port) # only binds to root ports
aer_enable_rootport
set_downstream_devices_error_reporting(root, true)
set_device_error_reporting(root, true)
pci_enable_pcie_error_reporting
pcie_capability_set_word(root, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS)
pci_walk_bus(root->subordinate set_device_error_reporting, true)
set_device_error_reporting(dev, true)
pci_enable_pcie_error_reporting
pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
PCI_EXP_AER_FLAGS)
We later call pcie_portdrv_probe() for every downstream bridge (it
matches PCI_CLASS_BRIDGE_PCI devices, then discards any non-PCIe
devices), and it *disables* AER reporting:
pcie_portdrv_probe(switch port)
pcie_port_device_register
get_port_device_capability
pci_disable_pcie_error_reporting
pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS)
The result is that we first enable AER for the downstream switch
ports, then we disable it again.
It does not need to disable AER for upstream/downstream ports as
AER driver only binds to root ports.
Fixes: 2bd50dd800b5(PCI: PCIe: Disable PCIe port services during port
initialization)
While you're correcting nits, use the conventional style here:
Fixes: 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port initialization")
all on one line for greppability.
Thanks for pointing out that, will fix.
Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
CC: stable@xxxxxxxxxxxxxxx
---
drivers/pci/pcie/portdrv_core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index a592103..a0dff44 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -241,7 +241,9 @@ static int get_port_device_capability(struct pci_dev *dev)
* Disable AER on this port in case it's been enabled by the
* BIOS (the AER service driver will enable it when necessary).
*/
- pci_disable_pcie_error_reporting(dev);
+ if ((pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) &&
+ (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM))
+ pci_disable_pcie_error_reporting(dev);
I'm not sure this code is in the right place. This is
get_port_device_capability(); we should be *getting* information, not
*configuring* the device here.
If we're not prepared to handle AER events, I think it's probably
a good idea to disable them, but I'd rather do it in the
pci_init_capabilities() path, e.g., in pci_aer_init().
So disable them in pci_aer_init(), enable them in aer_enable_rootport().
pciehp is not a capability, but I think we should also move the
disabling of PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE interrupts out
of get_port_device_capability(). Maybe to pci_configure_device()?
I also do not think we should check for upstream/downstream ports. If
we're going to disable AER (and I think that probably does make
sense), we should do it for every device until we're ready to handle
AER events.
It seems good to me.
Thanks,
Dongdong
}
/* VC support */
if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VC))
--
1.9.1
.