On 2021/2/19 1:20, Bjorn Helgaas wrote: > 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. > yes, it's probably a typo according to the latest spec. >> 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. > this patch comes when i was debugging the EDR and navigating the code and spec (i cannot get the latest spec at that time). no problem was observed but i have thought it might be sanity to ensure the ERR_COR was not set. it's ok leave the code as is, as the latest spec exlicitly requires the firmware to ensure this. >> [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@xxxxxxxxxxxxx> >> --- >> 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. > sure, that will be clearer. i just followed the previous implementation. Thanks, Yicong >> 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 >> > > . >
