On Tue, 25 Jun 2024, Smita Koralahalli wrote:
> 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?
I did add code to clear the LBMS count in pciehp_unconfigure_device() in
part thanks to this patch of yours. Do you think it wouldn't work?
But I agree there would still be problem if BWCTRL is not enabled. I
already have to keep part of it enabled due to the Target Speed quirk
and now this is another case where just having it always on would be
beneficial.
> > 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?
Yes, BW controller will take control of LBMS and other code should not
touch it directly (and LBMS will be kept cleared by the BW controller).
However, in this case I'll just need to adapt the code to replace the
LBMS clearing with resetting the LBMS count (if this patch is accepted
before BW controller), the resetting is already there anyway.
> > 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.
One thing I don't like in the Target Speed quirk is that it leaves the
Link Speed into the lower value if the quirk fails to bring the link up,
the quirk could restore the original Link Speed on failure to avoid these
problems. I even suggested that earlier, however, the downside of
restoring the original Link Speed is that it will require triggering yet
another retraining (perhaps we could avoid waiting for its completion
though since we expect it to fail).
It might be possible to eventually trigger the Target Speed quirk from the
BW controller but it would require writing some state machine so that the
quirk is not repeatedly attempted. It seemed to complicate things too much
to add such a state machine at this point.
--
i.
