Re: [PATCH v8] PCI: hotplug: Add a generic RAS tracepoinggt for hotplug event

[Shuai Xue <xueshuai@linux.alibaba.com>]

在 2025/7/22 20:29, Ilpo Järvinen 写道:
> 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@linux.intel.com/
>>>
>>>
>>
>> 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".

I see, I will keep both reason and 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.
> 


Aha, agree.

Thanks for valuable coments.

Best Regards,
Shuai