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

[Greg Kroah-Hartman]

On Mon, Aug 22, 2022 at 01:00:46PM +0900, Dominique Martinet wrote:
> 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.)

I've queued up the commit you referenced above now, thanks!

greg k-h