Re: [PATCH v2] PCI: pciehp: Clear LBMS on hot-remove to prevent link speed reduction

[Smita Koralahalli]

Hi all,

Sorry for the delay here. Took some time to find a system to run 
experiments. Comments inline.

On 6/19/2024 12:47 AM, Lukas Wunner wrote:
> On Tue, Jun 18, 2024 at 02:23:21PM -0700, Smita Koralahalli wrote:
> > On 6/18/2024 11:51 AM, Smita Koralahalli wrote:
> > > > > > But IIUC LBMS is set by hardware but never cleared by hardware, so if
> > > > > > we remove a device and power off the slot, it doesn't seem like LBMS
> > > > > > could be telling us anything useful (what could we do in response to
> > > > > > LBMS when the slot is empty?), so it makes sense to me to clear it.
> > > > > >
> > > > > > It seems like pciehp_unconfigure_device() does sort of PCI core and
> > > > > > driver-related things and possibly could be something shared by all
> > > > > > hotplug drivers, while remove_board() does things more specific to the
> > > > > > hotplug model (pciehp, shpchp, etc).
> > > > > >
> > > > > >  From that perspective, clearing LBMS might fit better in
> > > > > > remove_board(). In that case, I wonder whether it should be done
> > > > > > after turning off slot power? This patch clears is *before* turning
> > > > > > off the power, so I wonder if hardware could possibly set it again
> > > > > > before the poweroff?
> >
> > While clearing LBMS in remove_board() here:
> >
> > if (POWER_CTRL(ctrl)) {
> > 	pciehp_power_off_slot(ctrl);
> > +	pcie_capability_write_word(ctrl->pcie->port, PCI_EXP_LNKSTA,
> > 				   PCI_EXP_LNKSTA_LBMS);
> >
> > 	/*
> > 	 * After turning power off, we must wait for at least 1 second
> > 	 * before taking any action that relies on power having been
> > 	 * removed from the slot/adapter.
> > 	 */
> > 	msleep(1000);
> >
> > 	/* Ignore link or presence changes caused by power off */
> > 	atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
> > 		   &ctrl->pending_events);
> > }
> >
> > This can happen too right? I.e Just after the slot poweroff and before LBMS
> > clearing the PDC/PDSC could be fired. Then
> > pciehp_handle_presence_or_link_change() would hit case "OFF_STATE" and
> > proceed with pciehp_enable_slot() ....pcie_failed_link_retrain() and
> > ultimately link speed drops..
> >
> > So, I added clearing just before turning off the slot.. Let me know if I'm
> > thinking it right.

I guess I should have experimented before putting this comment out.

After talking to the HW/FW teams, I understood that, none of our CRBs 
support power controller for NVMe devices, which means the "Power 
Controller Present" in Slot_Cap is always false. That's what makes it a 
"surprise removal." If the OS was notified beforehand and there was a 
power controller attached, the OS would turn off the power with 
SLOT_CNTL. That's an "orderly" removal. So essentially, the entire block 
from "if (POWER_CTRL(ctrl))" will never be executed for surprise removal 
for us.

There could be board designs outside of us, with power controllers for 
the NVME devices, which I'm not aware of.
> This was added by 3943af9d01e9 ("PCI: pciehp: Ignore Link State Changes
> after powering off a slot").  You can try reproducing it by writing "0"
> to the slot's "power" file in sysfs, but your hardware needs to support
> slot power.
>
> Basically the idea is that after waiting for 1 sec, chances are very low
> that any DLLSC or PDSC events caused by removing slot power may still
> occur.

PDSC events occurring in our case aren't by removing slot power. It 
should/will always happen on a surprise removal along with DLLSC for us. 
But this PDSC is been delayed and happens after DLLSC is invoked and 
ctrl->state = OFF_STATE in pciehp_disable_slot(). So the PDSC is mistook 
to enable slot in pciehp_enable_slot() inside 
pciehp_handle_presence_or_link_change().
> Arguably the same applies to LBMS changes, so I'd recommend to likewise
> clear stale LBMS after the msleep(1000).
>
> pciehp_ctrl.c only contains the state machine and higher-level logic of
> the hotplug controller and all the actual register accesses are in helpers
> in pciehp_hpc.c.  So if you want to do it picture-perfectly, add a helper
> in pciehp_hpc.c to clear LBMS and call that from remove_board().
>
> That all being said, I'm wondering how this plays together with Ilpo's
> bandwidth control driver?
>
> https://lore.kernel.org/all/20240516093222.1684-1-ilpo.jarvinen@xxxxxxxxxxxxxxx/

I need to yet do a thorough reading of Ilpo's bandwidth control driver. 
Ilpo please correct me if I misspeak something as I don't have a 
thorough understanding.

Ilpo's bandwidth controller also checks for lbms count to be greater 
than zero to bring down link speeds if CONFIG_PCIE_BWCTRL is true. If 
false, it follows the default path to check LBMS bit in link status 
register. So if, CONFIG_PCIE_BWCTRL is disabled by default we continue 
to see link speed drops. Even, if BWCTRL is enabled, LBMS count is 
incremented to 1 in pcie_bwnotif_enable() so likely pcie_lbms_seen() 
might return true thereby bringing down speeds here as well if DLLLA is 
clear?

> IIUC, the bandwidth control driver will be in charge of handling LBMS
> changes.  So clearing LBMS behind the bandwidth control driver's back
> might be problematic.  Ilpo?
>
> Also, since you've confirmed that this issue is fallout from
> a89c82249c37 ("PCI: Work around PCIe link training failures"),
> I'm wondering if the logic introduced by that commit can be
> changed so that the quirk is applied more narrowly, i.e. *not*
> applied to unaffected hardware, such as AMD's hotplug ports.
> That would avoid the need to undo the effect of the quirk and
> work around the downtraining you're seeing.
>
> Maciej, any ideas?

Yeah I'm okay to go down to that approach as well. Any ideas would be 
helpful here.

Thanks
Smita
> Thanks,
>
> Lukas