On Wed, Feb 03, 2021 at 08:53:15PM +0800, Yicong Yang wrote:
Per Downstream Port Containment Related Enhancements ECN[1], Table 4-6 Interpretation of _OSC Control Field Returned Value, for bit 7 of _OSC control return value:
"If firmware allows the OS control of this feature, then, in the context of the _OSC method the OS must ensure that Downstream Port Containment ERR_COR signaling is disabled as described in the PCI Express Base Specification."
I think "the OS must ensure" is a typo in the spec. In the new r3.3 of the spec, it has been corrected to:
If firmware allows the operating system control of this feature, then, in the context of the _OSC method firmware must clear the DPC ERR_COR Enable bit in the DPC Control Register (refer to the PCI Express Base Specification) to 0.
and PCI Express Base Specification Revision 4.0 Version 1.0 section 6.2.10.2, Use of DPC ERR_COR Signaling:
"...DPC ERR_COR signaling is primarily intended for use by platform firmware..."
Currently we don't set DPC ERR_COR enable bit, but explicitly clear the bit to ensure it's disabled.
Does this fix a problem you observed? If you're seeing a problem, and this patch fixes it, we need to do something. But if it's just to line up with the language in the spec, I think we can rely on the corrected spec language, which says the *firmware* is responsible for doing this, and leave dpc_probe() alone.
[1] Downstream Port Containment Related Enhancements ECN, Jan 28, 2019, affecting PCI Firmware Specification, Rev. 3.2 https://members.pcisig.com/wg/PCI-SIG/document/12888
Signed-off-by: Yicong Yang yangyicong@hisilicon.com
drivers/pci/pcie/dpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index e05aba8..5cc8ef3 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev) pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap); pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
- ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
- ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
If we need to clear things here, I'd prefer to have names instead of the 0xfff4 or 0xffe4 magic numbers.
pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
-- 2.8.1