| Message ID | 1512467438-42850-2-git-send-email-liudongdong3@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/portdrv: Fix switch devctrl error report enable | expand |
On Tue, Dec 05, 2017 at 05:50:37PM +0800, Dongdong Liu wrote: > + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) && > + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)) > + pci_disable_pcie_error_reporting(dev); No need for the inner braces here.
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. > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> > CC: stable@vger.kernel.org > --- > 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(). 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. > } > /* VC support */ > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VC)) > -- > 1.9.1 >
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@huawei.com> >> CC: stable@vger.kernel.org >> --- >> 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 >> > > . >
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); } /* VC support */ if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VC))
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) Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> CC: stable@vger.kernel.org --- drivers/pci/pcie/portdrv_core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)