[PATCH v2 0/2] CXL/events: Fix and improve event dev_dbg() messages

[PATCH v2 0/2] CXL/events: Fix and improve event dev_dbg() messages

[Ira Weiny]

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Changes in v2:
- [Dan] split fix from additional debug message
- Link to v1: https://lore.kernel.org/r/20230731-cxl-fix-clear-event-debug-print-v1-1-42c068f500d1@intel.com

---
Ira Weiny (2):
      cxl/mbox: Use correct handle in events debug message
      cxl/mbox: Add handle to event processing debug

 drivers/cxl/core/mbox.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
---
base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
change-id: 20230731-cxl-fix-clear-event-debug-print-3b57da0e956c

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

[PATCH v2 1/2] cxl/mbox: Use correct handle in events debug message

[Ira Weiny]

The handle value used to report an event being cleared by dev_dbg() is
incorrect due to a post increment of the payload handle index.

Delay the increment of the index until after the print.

Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for v2
[Dan] Go ahead and mark this for backporting
[Dan] Remove 'To/Cc' cruft from individual patches
[Dan/Alison] Split out the additional debugging
[Alison] New oneliner
---
 drivers/cxl/core/mbox.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index d6d067fbee97..a1c490f66bbf 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -882,9 +882,10 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 	 */
 	i = 0;
 	for (cnt = 0; cnt < total; cnt++) {
-		payload->handles[i++] = get_pl->records[cnt].hdr.handle;
+		payload->handles[i] = get_pl->records[cnt].hdr.handle;
 		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
 			le16_to_cpu(payload->handles[i]));
+		i++;
 
 		if (i == max_handles) {
 			payload->nr_recs = i;

-- 
2.41.0

Re: [PATCH v2 1/2] cxl/mbox: Use correct handle in events debug message

[Dave Jiang]

On 8/1/23 13:13, Ira Weiny wrote:
> The handle value used to report an event being cleared by dev_dbg() is
> incorrect due to a post increment of the payload handle index.
> 
> Delay the increment of the index until after the print.
> 
> Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> Changes for v2
> [Dan] Go ahead and mark this for backporting
> [Dan] Remove 'To/Cc' cruft from individual patches
> [Dan/Alison] Split out the additional debugging
> [Alison] New oneliner
> ---
>   drivers/cxl/core/mbox.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d6d067fbee97..a1c490f66bbf 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -882,9 +882,10 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>   	 */
>   	i = 0;
>   	for (cnt = 0; cnt < total; cnt++) {
> -		payload->handles[i++] = get_pl->records[cnt].hdr.handle;
> +		payload->handles[i] = get_pl->records[cnt].hdr.handle;
>   		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
>   			le16_to_cpu(payload->handles[i]));
> +		i++;
>   
>   		if (i == max_handles) {
>   			payload->nr_recs = i;
> 

Re: [PATCH v2 1/2] cxl/mbox: Use correct handle in events debug message

[Alison Schofield]

On Tue, Aug 01, 2023 at 01:13:28PM -0700, Ira Weiny wrote:
> The handle value used to report an event being cleared by dev_dbg() is
> incorrect due to a post increment of the payload handle index.
> 
> Delay the increment of the index until after the print.
> 
> Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

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

> 
> ---
> Changes for v2
> [Dan] Go ahead and mark this for backporting
> [Dan] Remove 'To/Cc' cruft from individual patches
> [Dan/Alison] Split out the additional debugging
> [Alison] New oneliner
> ---
>  drivers/cxl/core/mbox.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d6d067fbee97..a1c490f66bbf 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -882,9 +882,10 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  	 */
>  	i = 0;
>  	for (cnt = 0; cnt < total; cnt++) {
> -		payload->handles[i++] = get_pl->records[cnt].hdr.handle;
> +		payload->handles[i] = get_pl->records[cnt].hdr.handle;
>  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
>  			le16_to_cpu(payload->handles[i]));
> +		i++;
>  
>  		if (i == max_handles) {
>  			payload->nr_recs = i;
> 
> -- 
> 2.41.0
> 

Re: [PATCH v2 1/2] cxl/mbox: Use correct handle in events debug message

[Jonathan Cameron]

On Tue, 01 Aug 2023 13:13:28 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> The handle value used to report an event being cleared by dev_dbg() is
> incorrect due to a post increment of the payload handle index.
> 
> Delay the increment of the index until after the print.
> 
> Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 

FWIW given how well this has already been reviewed.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Changes for v2
> [Dan] Go ahead and mark this for backporting
> [Dan] Remove 'To/Cc' cruft from individual patches
> [Dan/Alison] Split out the additional debugging
> [Alison] New oneliner
> ---
>  drivers/cxl/core/mbox.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index d6d067fbee97..a1c490f66bbf 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -882,9 +882,10 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  	 */
>  	i = 0;
>  	for (cnt = 0; cnt < total; cnt++) {
> -		payload->handles[i++] = get_pl->records[cnt].hdr.handle;
> +		payload->handles[i] = get_pl->records[cnt].hdr.handle;
>  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
>  			le16_to_cpu(payload->handles[i]));
> +		i++;
>  
>  		if (i == max_handles) {
>  			payload->nr_recs = i;
> 

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

[Ira Weiny]

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

[Dave Jiang]

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);
> 

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

[Alison Schofield]

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

[Ira Weiny]

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

[Alison Schofield]

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

[Jonathan Cameron]

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

[Alison Schofield]

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
>