| Message ID | 20240104013229.693041-1-vidyas@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V1] PCI: Clear errors logged in Secondary Status Register | expand |
Hi Bjorn, Do you have any comments for this patch? Thanks, Vidya Sagar On 1/4/2024 7:02 AM, Vidya Sagar wrote: > If a downstream port has a PCIe switch connected to it, the enumeration > process leaves the 'Received Master Abort' bit set in the Secondary > Status Register of the downstream port because of the Unsupported > Requests (URs) take place in the downstream hierarchy. Since the > ownership of Secondary Status Register always lies with the OS including > systems with Firmware-First approach for error handling[1], clear the > error status bits in the Secondary Status Register post enumeration. > > [1] https://lore.kernel.org/all/1fb9d746-0695-4d19-af98-f442f31cd464@nvidia.com/T/ > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > drivers/pci/probe.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 43159965e09e..edf8202465d8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1470,6 +1470,9 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > } > > out: > + /* Clear errors in the Secondary Status Register */ > + pci_write_config_word(dev, PCI_SEC_STATUS, 0xffff); > + > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); > > pm_runtime_put(&dev->dev);
On Thu, Jan 04, 2024 at 07:02:29AM +0530, Vidya Sagar wrote: > If a downstream port has a PCIe switch connected to it, the enumeration > process leaves the 'Received Master Abort' bit set in the Secondary > Status Register of the downstream port because of the Unsupported > Requests (URs) take place in the downstream hierarchy. Since the > ownership of Secondary Status Register always lies with the OS including > systems with Firmware-First approach for error handling[1], clear the > error status bits in the Secondary Status Register post enumeration. I would expect these URs to happen when enumerating below *all* PCIe Root Ports (not just when switches are present), and Master Aborts should happen in conventional PCI. Similarly, I don't think Firmware-First is relevant here. Only the fact that the OS owns PCI_SEC_STATUS because there's no mechanism to negotiate for platform ownership of it. We're in the merge window right now, so we'll start merging v6.9 material after v6.8-rc1 is tagged. > [1] https://lore.kernel.org/all/1fb9d746-0695-4d19-af98-f442f31cd464@nvidia.com/T/ > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > drivers/pci/probe.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 43159965e09e..edf8202465d8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1470,6 +1470,9 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, > } > > out: > + /* Clear errors in the Secondary Status Register */ > + pci_write_config_word(dev, PCI_SEC_STATUS, 0xffff); > + > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); > > pm_runtime_put(&dev->dev); > -- > 2.25.1 >
On 1/12/2024 10:36 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Thu, Jan 04, 2024 at 07:02:29AM +0530, Vidya Sagar wrote: >> If a downstream port has a PCIe switch connected to it, the enumeration >> process leaves the 'Received Master Abort' bit set in the Secondary >> Status Register of the downstream port because of the Unsupported >> Requests (URs) take place in the downstream hierarchy. Since the >> ownership of Secondary Status Register always lies with the OS including >> systems with Firmware-First approach for error handling[1], clear the >> error status bits in the Secondary Status Register post enumeration. > > I would expect these URs to happen when enumerating below *all* PCIe > Root Ports (not just when switches are present), and Master Aborts > should happen in conventional PCI. Agree. There was a misunderstanding from my side because of which I had said that the 'Received Master Abort' bit gets set only if there is a PCIe switch connected downstream. I'll correct it in my next patch. > > Similarly, I don't think Firmware-First is relevant here. Only the > fact that the OS owns PCI_SEC_STATUS because there's no mechanism to > negotiate for platform ownership of it. I mentioned about Firmware-First as a continuation to the discussion we had in [1]. But, agree that, this being a standalone patch, there is no need to mentioned about Firmware-First flow. > > We're in the merge window right now, so we'll start merging v6.9 > material after v6.8-rc1 is tagged. > >> [1] https://lore.kernel.org/all/1fb9d746-0695-4d19-af98-f442f31cd464@nvidia.com/T/ >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> drivers/pci/probe.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 43159965e09e..edf8202465d8 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1470,6 +1470,9 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, >> } >> >> out: >> + /* Clear errors in the Secondary Status Register */ >> + pci_write_config_word(dev, PCI_SEC_STATUS, 0xffff); >> + >> pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); >> >> pm_runtime_put(&dev->dev); >> -- >> 2.25.1 >>
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 43159965e09e..edf8202465d8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1470,6 +1470,9 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, } out: + /* Clear errors in the Secondary Status Register */ + pci_write_config_word(dev, PCI_SEC_STATUS, 0xffff); + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); pm_runtime_put(&dev->dev);
If a downstream port has a PCIe switch connected to it, the enumeration process leaves the 'Received Master Abort' bit set in the Secondary Status Register of the downstream port because of the Unsupported Requests (URs) take place in the downstream hierarchy. Since the ownership of Secondary Status Register always lies with the OS including systems with Firmware-First approach for error handling[1], clear the error status bits in the Secondary Status Register post enumeration. [1] https://lore.kernel.org/all/1fb9d746-0695-4d19-af98-f442f31cd464@nvidia.com/T/ Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- drivers/pci/probe.c | 3 +++ 1 file changed, 3 insertions(+)