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

[Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>]

On 6/18/2024 11:51 AM, Smita Koralahalli wrote:

[snip]

>>>> 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.

Thanks
Smita
>>>
>>> Yeah by talking to HW people I realized that HW could interfere possibly
>>> anytime to set LBMS when the slot power is on. Will change it to 
>>> include in
>>> remove_board().
>>>

[snip]