On Tue, 22 Jul 2025, Shuai Xue wrote: > 在 2025/7/21 18:18, Ilpo Järvinen 写道: > > On Fri, 18 Jul 2025, Shuai Xue wrote: > > > 在 2025/7/18 11:46, Matthew W Carlis 写道: > > > > On Thu, Jul 17, 2025 Bjorn Helgaas wrote > > > > > So I think your idea of adding current link speed/width to the "Link > > > > > Up" event is still on the table, and that does sound useful to me. > > > > > > > > We're already reading the link status register here to check DLLA so > > > > it would be nice. I guess if everything is healthy we're probably > > > > already > > > > at the maximum speed by this point. > > > > > > > > > In the future we might add another tracepoint when we enumerate the > > > > > device and know the Vendor/Device ID. > > > > > > > > I think we might have someone who would be interested in doing it. > > > > > > > > > Hi, all, > > > > > > IIUC, the current hotplug event (or presence event) is enough for Matthew. > > > and we would like a new tracepoing for link speed change which reports > > > speeds. > > > > > > For hotplug event, I plan to send a new version to > > > > > > 1. address Bjorn' concerns about event strings by removing its spaces. > > > > > > #define PCI_HOTPLUG_EVENT > > > \ > > > EM(PCI_HOTPLUG_LINK_UP, "PCI_HOTPLUG_LINK_UP") > > > \ > > > EM(PCI_HOTPLUG_LINK_DOWN, "PCI_HOTPLUG_LINK_DOWN") > > > \ > > > EM(PCI_HOTPLUG_CARD_PRESENT, "PCI_HOTPLUG_CARD_PRESENT") > > > \ > > > EMe(PCI_HOTPLUG_CARD_NOT_PRESENT, > > > "PCI_HOTPLUG_CARD_NOT_PRESENT") > > > > > > 2. address Ilpo comments by moving pci_hp_event to a common place > > > (include/trace/events/pci.h) so that the new comming can also use it. > > > > Ah, I only now noticed you've decided to re-place them. Please disregard > > my other comment about this being still open/undecided item. > > > > > For link speed change event (perhaps named as pci_link_event), > > > I plan to send a seperate patch, which provides: > > > > > > TP_STRUCT__entry( > > > __string( port_name, port_name ) > > > __field( unsigned char, cur_bus_speed ) > > > __field( unsigned char, max_bus_speed ) > > > __field( unsigned char, width ) > > > __field( unsigned int, flit_mode ) > > > __field( unsigned char, reason ) > > > ), > > > > > > The reason field is from Lukas ideas which indicates why the link speed > > > changed, e.g. "hotplug", "autonomous", "thermal", "retrain", etc. > > > > > > Are you happy with above changes? > > > > Since you're probably quite far with the pcie link event patch too given > > above, could you take a look at the LNKSTA flags representation in my > > patch and incorporate those as well as there seems to always lot of > > uncertainty about those flags when investigating the LBMS/bwctrl related > > issues so it seems prudent to explicitly include them into the traceevent > > output: > > > > https://lore.kernel.org/linux-pci/7c289bba-3133-0989-6333-41fc41fe3504@xxxxxxxxxxxxxxx/ > > > > > > Sure, Thank you for the feedback. > > I like the LNKSTA flags, LNKSTA flags provides better genericity > compared to the custom reason field I initially proposed. But it may > cause confusion when used in pcie_retrain_link(). However, I've > identified a potential issue when this approach is applied in > pcie_retrain_link() scenarios. I was trying to say the flags should be in addition to the other information, not replace reason. > Consider the following trace output when a device hotpluged: > > $ cat /sys/kernel/debug/tracing/trace_pipe > $ cat /sys/kernel/debug/tracing/trace_pipe > <...>-118 [002] ..... 28.414220: pci_hp_event: 0000:00:03.0 > slot:30, event:PCI_HOTPLUG_CARD_PRESENT > > <...>-118 [002] ..... 28.414273: pci_hp_event: 0000:00:03.0 > slot:30, event:PCI_HOTPLUG_LINK_UP > > irq/57-pciehp-118 [002] ..... 28.540189: pcie_link_event: > 0000:00:03.0 type:4, cur_bus_speed:2.5 GT/s PCIe, max_bus_speed:16.0 GT/s > PCIe, width:1, flit_mode:0, status:DLLLA > > irq/57-pciehp-118 [002] ..... 28.544999: pcie_link_event: > 0000:00:03.0 type:4, cur_bus_speed:2.5 GT/s PCIe, max_bus_speed:16.0 GT/s > PCIe, width:1, flit_mode:0, status:DLLLA > > The problem is that both trace events show status:DLLLA (Data Link Layer > Link Active), which is the direct reading from PCI_EXP_LNKSTA. However, > this doesn't accurately reflect the underlying context: > > - First DLLLA: Triggered by board_added() - link establishment after > card insertion > - Second DLLLA: Triggered by pcie_retrain_link() - link retraining > completion > > ( I trace the events in pcie_update_link_speed() ) > > In the second case, the more relevant status would be PCI_EXP_LNKSTA_LT > (Link Training) to indicate that link retraining was performed, even > though the final register state shows DLLLA. > > Question: Should we explicitly report the contextual status (e.g., > PCI_EXP_LNKSTA_LT for retraining scenarios) rather than always reading > the current register field? This would provide more meaningful trace > information for debugging link state transitions. I'd prefer it coming from the LNKSTA register (TBH, I don't see much value in synthetizing it at all). If we start to synthetize them, it will potentially hide hw issues. I see on some platforms two LBMS assertions per bwctrl speed change (which is done by retraining the link), one with LT=1 and the second with LT=0. ...But I never meant to replace "reason" with "flags". > Additionally, I'd appreciate your thoughts on the overall tracepoint > format shown above. Does this structure provide sufficient information > for hotplug and link analysis while maintaining readability? I don't have ideas how it could be improved beyond having those 4 flags available. I suspect noone does as we've not had ability to collect this information before when investigating issues so we're yet to understand all its potential. -- i.
