From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from szxga07-in.huawei.com ([45.249.212.35]:48890 "EHLO huawei.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752588AbdLRMzZ (ORCPT ); Mon, 18 Dec 2017 07:55:25 -0500 Subject: Re: [PATCH V2 1/2] PCI/portdrv: Fix switch devctrl error report enable To: Bjorn Helgaas References: <1512467438-42850-1-git-send-email-liudongdong3@huawei.com> <1512467438-42850-2-git-send-email-liudongdong3@huawei.com> <20171213162931.GD30595@bhelgaas-glaptop.roam.corp.google.com> CC: , , , From: Dongdong Liu Message-ID: Date: Mon, 18 Dec 2017 20:55:12 +0800 MIME-Version: 1.0 In-Reply-To: <20171213162931.GD30595@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: 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 >> 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 >> > > . >