Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 17:17:07 +0100
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     <alison.schofield@intel.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>, <linux-cxl@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] trace, cxl: Introduce a TRACE_EVENT for CXL Poison
 Records
Message-ID: <20220617171707.00001c84@Huawei.com>
In-Reply-To: <32a761fe7046680a4d50762fc43988def24a4bcd.1655250669.git.alison.schofield@intel.com>
References: <cover.1655250669.git.alison.schofield@intel.com>
        <32a761fe7046680a4d50762fc43988def24a4bcd.1655250669.git.alison.schofield@intel.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 14 Jun 2022 17:10:26 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add a trace event for CXL Poison List Media Error Records that
> includes the starting DPA of the poison, the length, and the
> the source of the poison.
> 
> This trace event will be used by the CXL_MEM driver to log the
> Media Errors returned by the GET_POISON_LIST Mailbox command.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Some comments on how this is used than the actual trace point for which

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The patch set currently just pushes through the length as provided
in the CXL record.  That is in units of 64 byte cachelines so I'd suggest
the result will be more readable if we multiply it up to be in bytes.

Also the address should be masked to the same length, but I mentioned that
at the caller.

Jonathan


> ---
>  include/trace/events/cxl.h | 60 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 include/trace/events/cxl.h
> 
> diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> new file mode 100644
> index 000000000000..17e707c3817e
> --- /dev/null
> +++ b/include/trace/events/cxl.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM cxl
> +
> +#if !defined(_CXL_TRACE_H) ||  defined(TRACE_HEADER_MULTI_READ)
> +#define _CXL_TRACE_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_DEFINE_ENUM(CXL_POISON_SOURCE_UNKNOWN);
> +TRACE_DEFINE_ENUM(CXL_POISON_SOURCE_INTERNAL);
> +TRACE_DEFINE_ENUM(CXL_POISON_SOURCE_EXTERNAL);
> +TRACE_DEFINE_ENUM(CXL_POISON_SOURCE_INJECTED);
> +TRACE_DEFINE_ENUM(CXL_POISON_SOURCE_VENDOR);
> +TRACE_DEFINE_ENUM(CXL_POISON_SOURCE_INVALID);
> +
> +#define show_poison_source(source)					\
> +	__print_symbolic(source,					\
> +			{CXL_POISON_SOURCE_UNKNOWN,  "UNKNOWN"},	\
> +			{CXL_POISON_SOURCE_EXTERNAL, "EXTERNAL"},	\
> +			{CXL_POISON_SOURCE_INTERNAL, "INTERNAL"},	\
> +			{CXL_POISON_SOURCE_INJECTED, "INJECTED"},	\
> +			{CXL_POISON_SOURCE_VENDOR,   "VENDOR"},		\
> +			{CXL_POISON_SOURCE_INVALID,  "INVALID"})
> +
> +TRACE_EVENT(cxl_poison_list,
> +
> +	    TP_PROTO(struct device *dev,
> +		     int source,
> +		     unsigned long start,
> +		     unsigned int length),
> +
> +	    TP_ARGS(dev, source, start, length),
> +
> +	    TP_STRUCT__entry(
> +		__string(name, dev_name(dev))
> +		__field(int, source)
> +		__field(u64, start)
> +		__field(u32, length)
> +	    ),
> +
> +	    TP_fast_assign(
> +		__assign_str(name, dev_name(dev));
> +		__entry->source = source;
> +		__entry->start = start;
> +		__entry->length = length;
> +	    ),
> +
> +	    TP_printk("dev %s source %s start %llu length %u",
> +		__get_str(name),
> +		show_poison_source(__entry->source),
> +		__entry->start,
> +		__entry->length)
> +);
> +#endif /* _CXL_TRACE_H */
> +
> +/* This part must be outside protection */
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE cxl
> +#include <trace/define_trace.h>