On Sat, Apr 09, 2022 at 10:13:15AM +0200, Pavel Machek wrote:
From: Liguang Zhang zhangliguang@linux.alibaba.com
Writes to a Downstream Port's Slot Control register are PCIe hotplug "commands." If the Port supports Command Completed events, software must wait for a command to complete before writing to Slot Control again.
pcie_do_write_cmd() sets ctrl->cmd_busy when it writes to Slot Control. If software notification is enabled, i.e., PCI_EXP_SLTCTL_HPIE and PCI_EXP_SLTCTL_CCIE are set, ctrl->cmd_busy is cleared by pciehp_isr().
But when software notification is disabled, as it is when pcie_init() powers off an empty slot, pcie_wait_cmd() uses pcie_poll_cmd() to poll for command completion, and it neglects to clear ctrl->cmd_busy, which leads to spurious timeouts:
I'm pretty sure this fixes the problem, but... it is still not fully correct.
+++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -98,6 +98,8 @@ static int pcie_poll_cmd(struct controll if (slot_status & PCI_EXP_SLTSTA_CC) { pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
ctrl->cmd_busy = 0;smp_mb(); return 1;Is the memory barrier neccessary? I don't see corresponding memory barrier for reading.
If it is neccessary, should we have WRITE_ONCE at the very least, or probably normal atomic operations?
The cmd_busy flag is set by pcie_do_write_cmd() before writing the Slot Control register and it is then cleared by pciehp_isr().
The purpose of the memory barriers is to ensure that order. IOW, we want to avoid a scenario where the write to cmd_busy in pcie_do_write_cmd() hasn't been committed to memory yet, the Slot Control write is performed, an interrupt occurs and is handled, the interrupt handler writes cmd_busy = 0 and only then is the cmd_busy = 1 write in pcie_do_write_cmd() committed to memory.
That said, you're right that such a scenario is impossible if cmd_busy is cleared by the synchronous pcie_poll_cmd() instead of the asynchronous pciehp_isr().
Care to submit a patch to remove the memory barrier in this single location?
A WRITE_ONCE() (i.e. a mere compiler barrier instead of a proper cacheline flush) is not sufficient to avoid the above scenario. An atomic bitop would work, but wouldn't offer advantages compared to the status quo.
Thanks,
Lukas