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

Smita Koralahalli
2024-06-25T20:20:15Z

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@linux.intel.com/

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
>