Re: [PATCH v4 1/3] PCI: trace: Add PCI controller LTSSM transition tracepoint

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5896 bytes --]

On Tue, 24 Feb 2026, Manivannan Sadhasivam wrote:

> On Tue, Feb 24, 2026 at 05:22:35PM +0200, Ilpo Järvinen wrote:
> > On Thu, 22 Jan 2026, Shawn Lin wrote:
> > 
> > > Some platforms may provide LTSSM trace functionality, recording historical
> > > LTSSM state transition information. This is very useful for debugging, such
> > > as when certain devices cannot be recognized or link broken during test.
> > > Implement the pci controller tracepoint for recording LTSSM and rate.
> > > 
> > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > > ---
> > > 
> > > Changes in v4:
> > > - use TRACE_EVENT_FN to notify when to start and stop the tracepoint,
> > >   and export pci_ltssm_tp_enabled() for host drivers to use
> > > 
> > > Changes in v3:
> > > - add TRACE_DEFINE_ENUM for all enums(Steven Rostedt)
> > > 
> > > Changes in v2: None
> > > 
> > >  drivers/pci/trace.c                   | 20 ++++++++++++
> > >  include/linux/pci.h                   |  4 +++
> > >  include/trace/events/pci_controller.h | 57 +++++++++++++++++++++++++++++++++++
> > >  3 files changed, 81 insertions(+)
> > >  create mode 100644 include/trace/events/pci_controller.h
> > > 
> > > diff --git a/drivers/pci/trace.c b/drivers/pci/trace.c
> > > index cf11abc..d351a51 100644
> > > --- a/drivers/pci/trace.c
> > > +++ b/drivers/pci/trace.c
> > > @@ -9,3 +9,23 @@
> > >  
> > >  #define CREATE_TRACE_POINTS
> > >  #include <trace/events/pci.h>
> > > +#include <trace/events/pci_controller.h>
> > > +
> > > +static atomic_t pcie_ltssm_tp_enabled = ATOMIC_INIT(0);
> > > +
> > > +bool pci_ltssm_tp_enabled(void)
> > > +{
> > > +	return atomic_read(&pcie_ltssm_tp_enabled) > 0;
> > > +}
> > > +EXPORT_SYMBOL(pci_ltssm_tp_enabled);
> > > +
> > > +int pci_ltssm_tp_reg(void)
> > > +{
> > > +	atomic_inc(&pcie_ltssm_tp_enabled);
> > > +	return 0;
> > > +}
> > > +
> > > +void pci_ltssm_tp_unreg(void)
> > > +{
> > > +	atomic_dec(&pcie_ltssm_tp_enabled);
> > > +}
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index e7cb527..ac25a3e 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -2770,6 +2770,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
> > >  }
> > >  #endif
> > >  
> > > +#ifdef CONFIG_TRACING
> > > +bool pci_ltssm_tp_enabled(void);
> > > +#endif
> > > +
> > >  void pci_add_dma_alias(struct pci_dev *dev, u8 devfn_from, unsigned nr_devfns);
> > >  bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2);
> > >  int pci_for_each_dma_alias(struct pci_dev *pdev,
> > > diff --git a/include/trace/events/pci_controller.h b/include/trace/events/pci_controller.h
> > > new file mode 100644
> > > index 0000000..db4a960
> > > --- /dev/null
> > > +++ b/include/trace/events/pci_controller.h
> > > @@ -0,0 +1,57 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#undef TRACE_SYSTEM
> > > +#define TRACE_SYSTEM pci_controller
> > 
> > I find putting this into "pci_controller" little odd as LTSSM is related 
> > to PCIe links/ports. To me looks something that belongs to the existing 
> > include/trace/events/pci.h.
> 
> I suggested 'pci_controller.h' since these tracepoints are only going to be used
> by the controller drivers. Putting it under 'pci.h' will imply that these can be
> used by the client drivers also.

PCIe r7 spec has Flit Performance Measurement Extended Capability that 
seems to have something for LTSSM tracking and those seem more generic 
than just for controllers (I've not spent much time on trying to fully 
understand those capabilities, just recalled seeing them earlier).

--
 i.

> > > +#if !defined(_TRACE_HW_EVENT_PCI_CONTROLLER_H) || defined(TRACE_HEADER_MULTI_READ)
> > > +#define _TRACE_HW_EVENT_PCI_CONTROLLER_H
> > > +
> > > +#include <uapi/linux/pci_regs.h>
> > > +#include <linux/tracepoint.h>
> > > +
> > > +TRACE_DEFINE_ENUM(PCIE_SPEED_2_5GT);
> > > +TRACE_DEFINE_ENUM(PCIE_SPEED_5_0GT);
> > > +TRACE_DEFINE_ENUM(PCIE_SPEED_8_0GT);
> > > +TRACE_DEFINE_ENUM(PCIE_SPEED_16_0GT);
> > > +TRACE_DEFINE_ENUM(PCIE_SPEED_32_0GT);
> > > +TRACE_DEFINE_ENUM(PCIE_SPEED_64_0GT);
> > > +TRACE_DEFINE_ENUM(PCI_SPEED_UNKNOWN);
> > > +
> > > +extern int pci_ltssm_tp_reg(void);
> > > +extern void pci_ltssm_tp_unreg(void);
> > > +
> > > +TRACE_EVENT_FN(pcie_ltssm_state_transition,
> > > +	TP_PROTO(const char *dev_name, const char *state, u32 rate),
> > > +	TP_ARGS(dev_name, state, rate),
> > > +
> > > +	TP_STRUCT__entry(
> > > +		__string(dev_name, dev_name)
> > > +		__string(state, state)
> > > +		__field(u32, rate)
> > > +	),
> > > +
> > > +	TP_fast_assign(
> > > +		__assign_str(dev_name);
> > > +		__assign_str(state);
> > > +		__entry->rate = rate;
> > > +	),
> > > +
> > > +	TP_printk("dev: %s state: %s rate: %s",
> > > +		__get_str(dev_name), __get_str(state),
> > > +		__print_symbolic(__entry->rate,
> > > +			{ PCIE_SPEED_2_5GT,  "2.5 GT/s" },
> > > +			{ PCIE_SPEED_5_0GT,  "5.0 GT/s" },
> > > +			{ PCIE_SPEED_8_0GT,  "8.0 GT/s" },
> > > +			{ PCIE_SPEED_16_0GT, "16.0 GT/s" },
> > > +			{ PCIE_SPEED_32_0GT, "32.0 GT/s" },
> > > +			{ PCIE_SPEED_64_0GT, "64.0 GT/s" },
> > > +			{ PCI_SPEED_UNKNOWN, "Unknown" }
> > 
> > Why are these done inline instead of using EM/EMe()? Or simply with
> > pci_speed_string()?
> > 
> > 
> > Unrelated to this, sadly I failed to notice Shuai's version of 
> > pcie_link_event() did not translate link speeds (my own version used 
> > pci_speed_string()).
> > 
> > > +		)
> > > +	),
> > > +
> > > +	pci_ltssm_tp_reg, pci_ltssm_tp_unreg
> > > +);
> > > +
> > > +#endif /* _TRACE_HW_EVENT_PCI_CONTROLLER_H */
> > > +
> > > +/* This part must be outside protection */
> > > +#include <trace/define_trace.h>
> > > 
> > 
> > -- 
> >  i.
> > 
> 
> 

-- 
 i.