Hi Bjorn Helgaas,
Thanks for the review!
On Tue, Feb 18, 2025 at 04:33:54PM -0600, Bjorn Helgaas wrote:
> On Tue, Feb 18, 2025 at 11:48:58AM +0800, Feng Tang wrote:
> > There was problem reported by firmware developers that they received
> > 2 pcie link control commands in very short intervals on an ARM server,
> > which doesn't comply with pcie spec, and broke their state machine and
> > work flow. According to PCIe 6.1 spec, section 6.7.3.2, software needs
> > to wait at least 1 second for the command-complete event, before
> > resending the cmd or sending a new cmd.
>
> s/link control/hotplug/ (also below)
> s/2/two/
> s/pcie/PCIe/ (also below)
Will follow.
> > And the first link control command firmware received is from
> > get_port_device_capability(), which sends cmd to disable pcie hotplug
> > interrupts without waiting for its completion.
> >
> > Fix it by adding the necessary wait to comply with PCIe spec, referring
> > pcie_poll_cmd().
> >
> > Also make the interrupt disabling not dependent on whether pciehp
> > service driver will be loaded as suggested by Lukas.
>
> This sounds like maybe it should be two separate patches.
OK.
[...]
> >
> > +static int pcie_wait_sltctl_cmd_raw(struct pci_dev *dev)
> > +{
> > + u16 slot_status = 0;
> > + int ret, ret1, timeout_us;
> > +
> > + /* 1 second, according to PCIe spec 6.1, section 6.7.3.2 */
> > + timeout_us = 1000000;
> > + ret = read_poll_timeout(pcie_capability_read_word, ret1,
> > + (slot_status & PCI_EXP_SLTSTA_CC), 10000,
> > + timeout_us, true, dev, PCI_EXP_SLTSTA,
> > + &slot_status);
> > + if (!ret)
> > + pcie_capability_write_word(dev, PCI_EXP_SLTSTA,
> > + PCI_EXP_SLTSTA_CC);
> > +
> > + return ret;
>
> Ugh. I really don't like the way this basically duplicates
> pcie_poll_cmd(). I don't have a great suggestion to fix it; maybe we
> need a way to build part of pciehp unconditionally.
Yes, I also thought about this. One idea is to unify the two functions,
and let pcie_poll_cmd() reuse this pcie_wait_sltctl_cmd_raw() here in
portdrv.c. As CONFIG_HOTPLUG_PCI_PCIE depends on CONFIG_PCIEPORTBUS,
there should be no dependency issue. How do you think?
> At the very least
> we need a comment here pointing to pcie_poll_cmd().
Aha, I mentioned 'referring pcie_poll_cmd()' in commit log, but forgot
to add it here.
>
> And IIUC this will add a one second delay for ports that don't need
> command completed events. I don't think that's fair to those ports.
Good catch! So we should add a read of PCI_EXP_SLTCAP register and
check if PCI_EXP_SLTCAP_HPC bit is set.
> > +}
> > +
> > +void pcie_disable_hp_interrupts_early(struct pci_dev *dev)
> > +{
> > + u16 slot_ctrl = 0;
> > +
> > + pcie_capability_read_word(dev, PCI_EXP_SLTCTL, &slot_ctrl);
> > + /* Bail out early if it is already disabled */
> > + if (!(slot_ctrl & (PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE)))
> > + return;
> > +
> > + pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> > + PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
> > +
> > + if (pcie_wait_sltctl_cmd_raw(dev))
> > + pci_info(dev, "Timeout on disabling PCIE hot-plug interrupt\n");
>
> s/PCIE/PCIe/
Will change.
Thanks,
Feng
