On Fri, Jul 25, 2025 at 04:09:21PM -0500, Bjorn Helgaas wrote:

quoted

@@ -319,8 +319,7 @@ int pciehp_check_link_status(struct controller *ctrl)
 		return -1;
 	}
 
-	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA2, &linksta2);
-	__pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status, linksta2);
+	pcie_update_link_speed(ctrl->pcie->port->subordinate, PCIE_HOTPLUG);

It kind of bugs me that the hot-add flow reads LNKSTA three times and
generates both pci_hp_event LINK_UP and link_event tracepoints:

  pciehp_handle_presence_or_link_change
    link_active = pciehp_check_link_active()
      pcie_capability_read_word(PCI_EXP_LNKSTA)
    if (link_active)
      ctrl_info(ctrl, "Slot(%s): Link Up\n")

This LNKSTA read decides whether to bring up the slot.
It can't be eliminated.

      trace_pci_hp_event(PCI_HOTPLUG_LINK_UP)
      pciehp_enable_slot
        __pciehp_enable_slot
          board_added
            pciehp_check_link_status
              pcie_capability_read_word(PCI_EXP_LNKSTA)

This is sort of a final check whether the link is (still) active
before commencing device enumeration.  Doesn't look like it can
safely be eliminated either.

              pcie_update_link_speed
                pcie_capability_read_word(PCI_EXP_LNKSTA)
                pcie_capability_read_word(PCI_EXP_LNKSTA2)
                trace_pcie_link_event(<REASON>)

This third register read is introduced by the present patch and is
indeed somewhat a step back, given that pciehp_check_link_status()
currently deliberately calls __pcie_update_link_speed() to pass
the already read LNKSTA value.

I'm wondering if the tracepoint can be moved down to
__pcie_update_link_speed()?

And maybe we need both a bare LINK_UP event and a link_event with all
the details, but again it seems a little weird to me that there are
two tracepoints when there's really only one event and we know all the
link_event information from the very first LNKSTA read.

One of the reasons is that a "Link Down" event would have to
contain dummy values for link speed etc, so it seemed cleaner
to separate the hotplug event from the link speed event.

Thanks,

Lukas