Re: [PATCH v5] PCI: hotplug: Add a generic RAS tracepoint for hotplug event

[Shuai Xue]

在 2025/1/14 04:55, Steven Rostedt 写道:
> On Thu,  9 Jan 2025 10:55:43 +0800
> Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx> wrote:
>
> > diff --git a/drivers/pci/hotplug/trace.h b/drivers/pci/hotplug/trace.h
> > new file mode 100644
> > index 000000000000..5b60cd7bcffb
> > --- /dev/null
> > +++ b/drivers/pci/hotplug/trace.h
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#if !defined(_TRACE_HW_EVENT_PCI_HP_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_HW_EVENT_PCI_HP_H
> > +
> > +#include <linux/tracepoint.h>
> > +
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM pci
> > +
> > +#define PCI_HOTPLUG_EVENT					\
> > +	EM(PCI_HOTPLUG_LINK_UP,			"Link Up")	\
> > +	EM(PCI_HOTPLUG_LINK_DOWN,		"Link Down")	\
> > +	EM(PCI_HOTPLUG_CARD_PRESENT,		"Card present")	\
> > +	EMe(PCI_HOTPLUG_CARD_NOT_PRESENT,	"Card not present")
>
> Since you are creating these enums in this patch, you can also do a
> shortcut here too. Instead of doing the define here, move it to
> include/uapi/linux/pci.h:
>
> > +
> > +/* Enums require being exported to userspace, for user tool parsing */
> > +#undef EM
> > +#undef EMe
> > +#define EM(a, b)	TRACE_DEFINE_ENUM(a);
> > +#define EMe(a, b)	TRACE_DEFINE_ENUM(a);
> > +
> > +PCI_HOTPLUG_EVENT
> > +
> > +/*
> > + * Now redefine the EM() and EMe() macros to map the enums to the strings
> > + * that will be printed in the output.
> > + */
> > +#undef EM
> > +#undef EMe
> > +#define EM(a, b)	{a, b},
> > +#define EMe(a, b)	{a, b}
> > +
> > +TRACE_EVENT(pci_hp_event,
> > +
> > +	TP_PROTO(const char *port_name,
> > +		 const char *slot,
> > +		 const int event),
> > +
> > +	TP_ARGS(port_name, slot, event),
> > +
> > +	TP_STRUCT__entry(
> > +		__string(	port_name,	port_name	)
> > +		__string(	slot,		slot		)
> > +		__field(	int,		event	)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__assign_str(port_name);
> > +		__assign_str(slot);
> > +		__entry->event = event;
> > +	),
> > +
> > +	TP_printk("%s slot:%s, event:%s\n",
> > +		__get_str(port_name),
> > +		__get_str(slot),
> > +		__print_symbolic(__entry->event, PCI_HOTPLUG_EVENT)
> > +	)
> > +);
> > +
> > +#endif /* _TRACE_HW_EVENT_PCI_HP_H */
> > +
> > +#undef TRACE_INCLUDE_PATH
> > +#define TRACE_INCLUDE_PATH  ../../drivers/pci/hotplug
> > +#undef TRACE_INCLUDE_FILE
> > +#define TRACE_INCLUDE_FILE trace
> > +
> > +/* This part must be outside protection */
> > +#include <trace/define_trace.h>
> > diff --git a/include/uapi/linux/pci.h b/include/uapi/linux/pci.h
> > index a769eefc5139..4f150028965d 100644
> > --- a/include/uapi/linux/pci.h
> > +++ b/include/uapi/linux/pci.h
> > @@ -39,4 +39,11 @@
> >   #define PCIIOC_MMAP_IS_MEM	(PCIIOC_BASE | 0x02)	/* Set mmap state to MEM space. */
> >   #define PCIIOC_WRITE_COMBINE	(PCIIOC_BASE | 0x03)	/* Enable/disable write-combining. */
> >   
> > +enum pci_hotplug_event {
> > +	PCI_HOTPLUG_LINK_UP,
> > +	PCI_HOTPLUG_LINK_DOWN,
> > +	PCI_HOTPLUG_CARD_PRESENT,
> > +	PCI_HOTPLUG_CARD_NOT_PRESENT,
> > +};
>
> Instead of defining the enum as you did above, if you have the define of
> the enums here, you could do:
>
> #define PCI_HOTPLUG_EVENT					\
> 	EM(PCI_HOTPLUG_LINK_UP,			"Link Up")	\
> 	EM(PCI_HOTPLUG_LINK_DOWN,		"Link Down")	\
> 	EM(PCI_HOTPLUG_CARD_PRESENT,		"Card present")	\
> 	EMe(PCI_HOTPLUG_CARD_NOT_PRESENT,	"Card not present")
>
> #undef EM
> #undef EMe
> #define EM(a, b)	a,
> #define EMe(a, b)	a,
>
> enum pci_hotplug_event {
> 	PCI_HOTPLUG_EVENT
> };
>
> Then you only have one place to worry about adding new enums ;-)

Will do.


> -- Steve

Thanks for valuable comments.

Best Regards,
Shuai