CXL/events: Fix and improve event dev_dbg() messages

[PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Ira Weiny 2 years, 1 month ago

Knowing which handle is being processed helped in debugging new event
code.  Add a dev_dbg() message with this information.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 drivers/cxl/core/mbox.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index a1c490f66bbf..f052d5f174ee 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
 		if (!nr_rec)
 			break;
 
-		for (i = 0; i < nr_rec; i++)
+		for (i = 0; i < nr_rec; i++) {
+			dev_dbg(dev, "Event log %d: processing handle %u\n",
+				type,
+				le16_to_cpu(payload->records[i].hdr.handle));
 			cxl_event_trace_record(cxlmd, type,
 					       &payload->records[i]);
+		}
 
 		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
 			trace_cxl_overflow(cxlmd, type, payload);

-- 
2.41.0

Re: [PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Alison Schofield 2 years, 1 month ago

On Tue, Aug 01, 2023 at 01:13:29PM -0700, Ira Weiny wrote:
> Knowing which handle is being processed helped in debugging new event
> code.  Add a dev_dbg() message with this information.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>


> ---
>  drivers/cxl/core/mbox.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a1c490f66bbf..f052d5f174ee 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		if (!nr_rec)
>  			break;
>  
> -		for (i = 0; i < nr_rec; i++)
> +		for (i = 0; i < nr_rec; i++) {
> +			dev_dbg(dev, "Event log %d: processing handle %u\n",
> +				type,
> +				le16_to_cpu(payload->records[i].hdr.handle));
>  			cxl_event_trace_record(cxlmd, type,
>  					       &payload->records[i]);
> +		}
>  
>  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>  			trace_cxl_overflow(cxlmd, type, payload);
> 
> -- 
> 2.41.0
>

Re: [PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Alison Schofield 2 years, 1 month ago

On Tue, Aug 01, 2023 at 01:13:29PM -0700, Ira Weiny wrote:
> Knowing which handle is being processed helped in debugging new event
> code.  Add a dev_dbg() message with this information.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a1c490f66bbf..f052d5f174ee 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>  		if (!nr_rec)
>  			break;
>  
> -		for (i = 0; i < nr_rec; i++)
> +		for (i = 0; i < nr_rec; i++) {
> +			dev_dbg(dev, "Event log %d: processing handle %u\n",
> +				type,
> +				le16_to_cpu(payload->records[i].hdr.handle));
>  			cxl_event_trace_record(cxlmd, type,
>  					       &payload->records[i]);
> +		}

Is dev_dbg() overkill when the info is in the trace event?

>  
>  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>  			trace_cxl_overflow(cxlmd, type, payload);
> 
> -- 
> 2.41.0
>

Re: [PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Ira Weiny 2 years, 1 month ago

Alison Schofield wrote:
> On Tue, Aug 01, 2023 at 01:13:29PM -0700, Ira Weiny wrote:
> > Knowing which handle is being processed helped in debugging new event
> > code.  Add a dev_dbg() message with this information.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/cxl/core/mbox.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index a1c490f66bbf..f052d5f174ee 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> >  		if (!nr_rec)
> >  			break;
> >  
> > -		for (i = 0; i < nr_rec; i++)
> > +		for (i = 0; i < nr_rec; i++) {
> > +			dev_dbg(dev, "Event log %d: processing handle %u\n",
> > +				type,
> > +				le16_to_cpu(payload->records[i].hdr.handle));
> >  			cxl_event_trace_record(cxlmd, type,
> >  					       &payload->records[i]);
> > +		}
> 
> Is dev_dbg() overkill when the info is in the trace event?

The trace event will not happen if tracing is not turned on.

This was all part of my testing the DCD events which don't have to, and
likely will not, have tracing on.  So this helped to track which handles
were being processed from the cxl-test and qemu layers.

Ira

> 
> >  
> >  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
> >  			trace_cxl_overflow(cxlmd, type, payload);
> > 
> > -- 
> > 2.41.0
> >

Re: [PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Alison Schofield 2 years, 1 month ago

On Wed, Aug 02, 2023 at 10:05:56PM -0700, Ira Weiny wrote:
> Alison Schofield wrote:
> > On Tue, Aug 01, 2023 at 01:13:29PM -0700, Ira Weiny wrote:
> > > Knowing which handle is being processed helped in debugging new event
> > > code.  Add a dev_dbg() message with this information.
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > ---
> > >  drivers/cxl/core/mbox.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > > index a1c490f66bbf..f052d5f174ee 100644
> > > --- a/drivers/cxl/core/mbox.c
> > > +++ b/drivers/cxl/core/mbox.c
> > > @@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> > >  		if (!nr_rec)
> > >  			break;
> > >  
> > > -		for (i = 0; i < nr_rec; i++)
> > > +		for (i = 0; i < nr_rec; i++) {
> > > +			dev_dbg(dev, "Event log %d: processing handle %u\n",
> > > +				type,
> > > +				le16_to_cpu(payload->records[i].hdr.handle));
> > >  			cxl_event_trace_record(cxlmd, type,
> > >  					       &payload->records[i]);
> > > +		}
> > 
> > Is dev_dbg() overkill when the info is in the trace event?
> 
> The trace event will not happen if tracing is not turned on.
> 
> This was all part of my testing the DCD events which don't have to, and
> likely will not, have tracing on.  So this helped to track which handles
> were being processed from the cxl-test and qemu layers.
> 
> Ira

Thanks for explaining!

> 
> > 
> > >  
> > >  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
> > >  			trace_cxl_overflow(cxlmd, type, payload);
> > > 
> > > -- 
> > > 2.41.0
> > > 
> 
>

Re: [PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Jonathan Cameron 2 years, 1 month ago

On Thu, 3 Aug 2023 10:26:00 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Wed, Aug 02, 2023 at 10:05:56PM -0700, Ira Weiny wrote:
> > Alison Schofield wrote:  
> > > On Tue, Aug 01, 2023 at 01:13:29PM -0700, Ira Weiny wrote:  
> > > > Knowing which handle is being processed helped in debugging new event
> > > > code.  Add a dev_dbg() message with this information.
> > > > 
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > ---
> > > >  drivers/cxl/core/mbox.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > > > index a1c490f66bbf..f052d5f174ee 100644
> > > > --- a/drivers/cxl/core/mbox.c
> > > > +++ b/drivers/cxl/core/mbox.c
> > > > @@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
> > > >  		if (!nr_rec)
> > > >  			break;
> > > >  
> > > > -		for (i = 0; i < nr_rec; i++)
> > > > +		for (i = 0; i < nr_rec; i++) {
> > > > +			dev_dbg(dev, "Event log %d: processing handle %u\n",
> > > > +				type,
> > > > +				le16_to_cpu(payload->records[i].hdr.handle));
> > > >  			cxl_event_trace_record(cxlmd, type,
> > > >  					       &payload->records[i]);
> > > > +		}  
> > > 
> > > Is dev_dbg() overkill when the info is in the trace event?  
> > 
> > The trace event will not happen if tracing is not turned on.
> > 
> > This was all part of my testing the DCD events which don't have to, and
> > likely will not, have tracing on.  So this helped to track which handles
> > were being processed from the cxl-test and qemu layers.
> > 
> > Ira  
> 
> Thanks for explaining!

Good to have that detail in the commit message as it's good justification
for the change. Otherwise LGTM.

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

> 
> >   
> > >   
> > > >  
> > > >  		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
> > > >  			trace_cxl_overflow(cxlmd, type, payload);
> > > > 
> > > > -- 
> > > > 2.41.0
> > > >   
> > 
> >

Re: [PATCH v2 2/2] cxl/mbox: Add handle to event processing debug

Posted by Dave Jiang 2 years, 1 month ago


On 8/1/23 13:13, Ira Weiny wrote:
> Knowing which handle is being processed helped in debugging new event
> code.  Add a dev_dbg() message with this information.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/mbox.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index a1c490f66bbf..f052d5f174ee 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -947,9 +947,13 @@ static void cxl_mem_get_records_log(struct cxl_memdev_state *mds,
>   		if (!nr_rec)
>   			break;
>   
> -		for (i = 0; i < nr_rec; i++)
> +		for (i = 0; i < nr_rec; i++) {
> +			dev_dbg(dev, "Event log %d: processing handle %u\n",
> +				type,
> +				le16_to_cpu(payload->records[i].hdr.handle));
>   			cxl_event_trace_record(cxlmd, type,
>   					       &payload->records[i]);
> +		}
>   
>   		if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
>   			trace_cxl_overflow(cxlmd, type, payload);
>