Re: [PATCH v2 1/1] PCI: pciehp: Skip DLLSC handling if DPC is triggered

From: Lukas Wunner <lukas@wunner.de>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Linux PCI <linux-pci@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Keith Busch <kbusch@kernel.org>,
	knsathya@kernel.org, Sinan Kaya <okaya@kernel.org>
Subject: Re: [PATCH v2 1/1] PCI: pciehp: Skip DLLSC handling if DPC is triggered
Date: Wed, 17 Mar 2021 06:31:14 +0100	[thread overview]
Message-ID: <20210317053114.GA32370@wunner.de> (raw)
In-Reply-To: <CAPcyv4jxTcUEgcfPRckHqrUPy8gR7ZJsxDaeU__pSq6PqJERAQ@mail.gmail.com>

On Tue, Mar 16, 2021 at 10:08:31PM -0700, Dan Williams wrote:
> On Tue, Mar 16, 2021 at 9:14 PM Lukas Wunner <lukas@wunner.de> wrote:
> >
> > On Fri, Mar 12, 2021 at 07:32:08PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > +     if ((events == PCI_EXP_SLTSTA_DLLSC) && is_dpc_reset_active(pdev)) {
> > > +             ctrl_info(ctrl, "Slot(%s): DLLSC event(DPC), skipped\n",
> > > +                       slot_name(ctrl));
> > > +             ret = IRQ_HANDLED;
> > > +             goto out;
> > > +     }
> >
> > Two problems here:
> >
> > (1) If recovery fails, the link will *remain* down, so there'll be
> >     no Link Up event.  You've filtered the Link Down event, thus the
> >     slot will remain in ON_STATE even though the device in the slot is
> >     no longer accessible.  That's not good, the slot should be brought
> >     down in this case.
> 
> Can you elaborate on why that is "not good" from the end user
> perspective? From a driver perspective the device driver context is
> lost and the card needs servicing. The service event starts a new
> cycle of slot-attention being triggered and that syncs the slot-down
> state at that time.

All of pciehp's code assumes that if the link is down, the slot must be
off.  A slot which is in ON_STATE for a prolonged period of time even
though the link is down is an oddity the code doesn't account for.

If the link goes down, the slot should be brought into OFF_STATE.
(It's okay though to delay bringdown until DPC recovery has completed
unsuccessfully, which is what the patch I'm proposing does.)

I don't understand what you mean by "service event".  Someone unplugging
and replugging the NVMe drive?

> > (2) If recovery succeeds, there's a race where pciehp may call
> >     is_dpc_reset_active() *after* dpc_reset_link() has finished.
> >     So both the DPC Trigger Status bit as well as pdev->dpc_reset_active
> >     will be cleared.  Thus, the Link Up event is not filtered by pciehp
> >     and the slot is brought down and back up even though DPC recovery
> >     was succesful, which seems undesirable.
> 
> The hotplug driver never saw the Link Down, so what does it do when
> the slot transitions from Link Up to Link Up? Do you mean the Link
> Down might fire after the dpc recovery has completed if the hotplug
> notification was delayed?

If the Link Down is filtered and the Link Up is not, pciehp will
bring down the slot and then bring it back up.  That's because pciehp
can't really tell whether a DLLSC event is Link Up or Link Down.

It just knows that the link was previously up, is now up again,
but must have been down intermittently, so transactions to the
device in the slot may have been lost and the slot is therefore
brought down for safety.  Because the link is up, it is then
brought back up.

Thanks,

Lukas