2024-03-04 13:15:04

by Ethan Zhao

Subject: [PATCH pci-next] pci/edr: Ignore Surprise Down error on hot removal

Per PCI firmware spec r3.3 sec 4.6.12, for firmware first mode DPC
handling path, FW should clear UC errors logged by port and bring link
out of DPC, but because of ambiguity of wording in the spec, some BIOSes
doesn't clear the surprise down error and the error bits in pci status,
still notify OS to handle it. thus following trick is needed in EDR when
double reporting (hot removal interrupt && dpc notification) is hit.

https://patchwork.kernel.org/project/linux-pci/patch/20240207181854.
[email protected]/

Signed-off-by: Ethan Zhao <[email protected]>
---
drivers/pci/pci.h | 1 +
drivers/pci/pcie/dpc.c | 9 +++++----
drivers/pci/pcie/edr.c | 3 +++
3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 50134b5e3235..3787bb32e724 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -443,6 +443,7 @@ void pci_save_dpc_state(struct pci_dev *dev);
void pci_restore_dpc_state(struct pci_dev *dev);
void pci_dpc_init(struct pci_dev *pdev);
void dpc_process_error(struct pci_dev *pdev);
+bool dpc_handle_surprise_removal(struct pci_dev *pdev);
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
bool pci_dpc_recovered(struct pci_dev *pdev);
#else
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 98b42e425bb9..be79f205e04c 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -319,8 +319,10 @@ static void pci_clear_surpdn_errors(struct pci_dev *pdev)
pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_FED);
}

-static void dpc_handle_surprise_removal(struct pci_dev *pdev)
+bool dpc_handle_surprise_removal(struct pci_dev *pdev)
{
+ if (!dpc_is_surprise_removal(pdev))
+ return false;
if (!pcie_wait_for_link(pdev, false)) {
pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
goto out;
@@ -338,6 +340,7 @@ static void dpc_handle_surprise_removal(struct pci_dev *pdev)
out:
clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
wake_up_all(&dpc_completed_waitqueue);
+ return true;
}

static bool dpc_is_surprise_removal(struct pci_dev *pdev)
@@ -362,10 +365,8 @@ static irqreturn_t dpc_handler(int irq, void *context)
* According to PCIe r6.0 sec 6.7.6, errors are an expected side effect
* of async removal and should be ignored by software.
*/
- if (dpc_is_surprise_removal(pdev)) {
- dpc_handle_surprise_removal(pdev);
+ if (dpc_handle_surprise_removal(pdev))
return IRQ_HANDLED;
- }

dpc_process_error(pdev);

diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
index 5f4914d313a1..556edfb2696a 100644
--- a/drivers/pci/pcie/edr.c
+++ b/drivers/pci/pcie/edr.c
@@ -184,6 +184,9 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
goto send_ost;
}

+ if (dpc_handle_surprise_removal(edev))
+ goto send_ost;
+
dpc_process_error(edev);
pci_aer_raw_clear_status(edev);


base-commit: a66f2b4a4d365dc4bac35576f3a9d4f5982f1d63
--
2.31.1

2024-03-04 11:59:04

by Lukas Wunner

Subject: Re: [PATCH pci-next] pci/edr: Ignore Surprise Down error on hot removal

On Mon, Mar 04, 2024 at 04:08:19AM -0500, Ethan Zhao wrote:
> Per PCI firmware spec r3.3 sec 4.6.12, for firmware first mode DPC
> handling path, FW should clear UC errors logged by port and bring link
> out of DPC, but because of ambiguity of wording in the spec, some BIOSes
> doesn't clear the surprise down error and the error bits in pci status,
> still notify OS to handle it. thus following trick is needed in EDR when
> double reporting (hot removal interrupt && dpc notification) is hit.

Please provide more detailed information about the hardware and BIOS
affected by this.


> -static void dpc_handle_surprise_removal(struct pci_dev *pdev)
> +bool dpc_handle_surprise_removal(struct pci_dev *pdev)
> {
> + if (!dpc_is_surprise_removal(pdev))
> + return false;

This change of moving dpc_is_surprise_removal() into
dpc_handle_surprise_removal() seems unrelated to the problem at hand.

Please drop it if it's unnecessary to fix the issue.


> --- a/drivers/pci/pcie/edr.c
> +++ b/drivers/pci/pcie/edr.c
> @@ -184,6 +184,9 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data)
> goto send_ost;
> }
>
> + if (dpc_handle_surprise_removal(edev))
> + goto send_ost;
> +
> dpc_process_error(edev);
> pci_aer_raw_clear_status(edev);

This seems to be the only necessary change. Please reduce the
patch to contain only it and no other refactoring.

Please capitalize the "PCI/EDR: " prefix in the subject and add
a Fixes tag.

Thanks,

Lukas