Re: nvme-pci: Disabling device after reset failure: -5 occurs while AER recovery

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Mar 13, 2023 at 05:57:43PM -0700, Tushar Dave wrote:
> On 3/11/23 00:22, Lukas Wunner wrote:
> > On Fri, Mar 10, 2023 at 05:45:48PM -0800, Tushar Dave wrote:
> > > On 3/10/2023 3:53 PM, Bjorn Helgaas wrote:
> > > > In the log below, pciehp obviously is enabled; should I infer that in
> > > > the log above, it is not?
> > > 
> > > pciehp is enabled all the time. In the log above and below.
> > > I do not have answer yet why pciehp shows-up only in some tests (due to DPC
> > > link down/up) and not in others like you noticed in both the logs.
> > 
> > Maybe some of the switch Downstream Ports are hotplug-capable and
> > some are not?  (Check the Slot Implemented bit in the PCI Express
> > Capabilities Register as well as the Hot-Plug Capable bit in the
> > Slot Capabilities Register.)
> > ...

> > > > Generally we've avoided handling a device reset as a
> > > > remove/add event because upper layers can't deal well with
> > > > that.  But in the log below it looks like pciehp *did* treat
> > > > the DPC containment as a remove/add, which of course involves
> > > > configuring the "new" device and its MPS settings.
> > > 
> > > yes and that puzzled me why? especially when"Link Down/Up
> > > ignored (recovered by DPC)". Do we still have race somewhere, I
> > > am not sure.
> > 
> > You're seeing the expected behavior.  pciehp ignores DLLSC events
> > caused by DPC, but then double-checks that DPC recovery succeeded.
> > If it didn't, it would be a bug not to bring down the slot.  So
> > pciehp does exactly that.  See this code snippet in
> > pciehp_ignore_dpc_link_change():
> > 
> > 	/*
> > 	 * If the link is unexpectedly down after successful recovery,
> > 	 * the corresponding link change may have been ignored above.
> > 	 * Synthesize it to ensure that it is acted on.
> > 	 */
> > 	down_read_nested(&ctrl->reset_lock, ctrl->depth);
> > 	if (!pciehp_check_link_active(ctrl))
> > 		pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
> > 	up_read(&ctrl->reset_lock);
> > 
> > So on hotplug-capable ports, pciehp is able to mop up the mess
> > created by fiddling with the MPS settings behind the kernel's
> > back.
> 
> That's the thing, even on hotplug-capable slot I do not see pciehp
> _all_ the time. Sometime pciehp get involve and takes care of things
> (like I mentioned in the previous thread) and other times no pciehp
> engagement at all!

Possibly a timing issue, so I'll be interested to see if 53b54ad074de
("PCI/DPC: Await readiness of secondary bus after reset") makes any
difference.  Lukas didn't mention that, so maybe it's a red herring,
but I'm still curious since it explicitly mentions the DPC reset case
that you're exercising here.

Bjorn