Re: [PATCH 5.10 480/545] PCI/ERR: Add pci_walk_bridge() to pcie_do_recovery()

[Dominique Martinet]

Greg Kroah-Hartman wrote on Fri, Aug 19, 2022 at 05:44:10PM +0200:
> From: Sean V Kelley <sean.v.kelley@xxxxxxxxx>
> 
> [ Upstream commit 05e9ae19ab83881a0f33025bd1288e41e552a34b ]
> 
> Consolidate subordinate bus checks with pci_walk_bus() into
> pci_walk_bridge() for walking below potentially AER affected bridges.
> 
> Link: https://lore.kernel.org/r/20201121001036.8560-10-sean.v.kelley@xxxxxxxxx
> Tested-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> # non-native/no RCEC
> Signed-off-by: Sean V Kelley <sean.v.kelley@xxxxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  drivers/pci/pcie/err.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 931e75f2549d..8b53aecdb43d 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> [...]
> @@ -165,23 +182,22 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>  	else
>  		bridge = pci_upstream_bridge(dev);
>  
> -	bus = bridge->subordinate;
>  	pci_dbg(bridge, "broadcast error_detected message\n");
>  	if (state == pci_channel_io_frozen) {
> -		pci_walk_bus(bus, report_frozen_detected, &status);
> +		pci_walk_bridge(bridge, report_frozen_detected, &status);
>  		status = reset_subordinates(bridge);
>  		if (status != PCI_ERS_RESULT_RECOVERED) {
>  			pci_warn(bridge, "subordinate device reset failed\n");
>  			goto failed;
>  		}

A local conflict merging this made me notice a later commit:
-----
commit 387c72cdd7fb6bef650fb078d0f6ae9682abf631
Author: Keith Busch <kbusch@xxxxxxxxxx>
Date:   Mon Jan 4 15:02:58 2021 -0800

PCI/ERR: Retain status from error notification

Overwriting the frozen detected status with the result of the link reset
loses the NEED_RESET result that drivers are depending on for error
handling to report the .slot_reset() callback. Retain this status so
that subsequent error handling has the correct flow.

Link: https://lore.kernel.org/r/20210104230300.1277180-4-kbusch@xxxxxxxxxx
Reported-by: Hinko Kocevar <hinko.kocevar@xxxxxx>
Tested-by: Hedi Berriche <hedi.berriche@xxxxxxx>
Signed-off-by: Keith Busch <kbusch@xxxxxxxxxx>
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Acked-by: Sean V Kelley <sean.v.kelley@xxxxxxxxx>
Acked-by: Hedi Berriche <hedi.berriche@xxxxxxx>

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index a84f0bf4c1e2..b576aa890c76 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -198,8 +198,7 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
 	pci_dbg(bridge, "broadcast error_detected message\n");
 	if (state == pci_channel_io_frozen) {
 		pci_walk_bridge(bridge, report_frozen_detected, &status);
-		status = reset_subordinates(bridge);
-		if (status != PCI_ERS_RESULT_RECOVERED) {
+		if (reset_subordinates(bridge) != PCI_ERS_RESULT_RECOVERED) {
 			pci_warn(bridge, "subordinate device reset failed\n");
 			goto failed;
 		}
-----

Since this (commit I reply to) has been picked up, I think it'd make
sense to also include this (commit I just listed) in a later 5.10 tag.
It cherry-picks without error but would you like me to resend?
(I have added in Cc all involved people to this mail)

Digging through the mails the patch came with seem to imply approval for
stable merges; but it didn't make sense until pci_walk_bridge() had been
added just now. Now it's here we probably want both:
https://lore.kernel.org/all/d9ee4151-b28d-a52a-b5be-190a75e0e49b@xxxxxxxxx/


(I noticed because the NXP kernel we are provided includes a different
"fix" for what I believe to be the same issue, previously discussed here:
https://lore.kernel.org/linux-pci/12115.1588207324@famine/

I haven't actually encountered any of the problems discribed, so this is
purely theorical for me; it just looks a bit weird.)


Thanks,
--
Dominique