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
