[PATCH v2 0/1] cxl/mem: Fix for the index of Clear Event Record Handle

[PATCH v2 0/1] cxl/mem: Fix for the index of Clear Event Record Handle

[Yuquan Wang]

This is a simple fix for the index of 'Clear Event Record' Handle. The
print content of dev_dbg from Clear Event Records mailbox command would
report the handle of the next record to clear not the current one.

The problem was found when I was doing the debug of CXL Event Error on
Qemu. I injected an individual event through QMP 
'cxl-inject-general-media-event':
{ "execute": "cxl-inject-general-media-event",
    "arguments": {
        "path": "/machine/peripheral/cxl-mem0",
        "log": "informational",
        "flags": 1,
        "dpa": 1000,
        "descriptor": 3,
        "type": 3,
        "transaction-type": 192,
        "channel": 3,
        "device": 5,
        "component-id": "iras mem"
    }}

Then the kernel printed: 
[ 1639.106181] cxl_pci 0000:0d:00.0: Event log '0': Clearing 0

However, the line 36 in 'hw/cxl/cxl-events.c': log->next_handle = 1;
It will set the actual handle value of injected event to '1'.

With this fix, the kernel will print:
[  122.456750] cxl_pci 0000:0d:00.0: Event log '0': Clearing 1
which is in line with the simulated value in Qemu.

Yuquan Wang (1):
  cxl/mem: Fix for the index of Clear Event Record Handle

 drivers/cxl/core/mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/1] cxl/mem: Fix for the index of Clear Event Record Handle

[Yuquan Wang]

The dev_dbg info for Clear Event Records mailbox command would report
the handle of the next record to clear not the current one.

This was because the index 'i' had incremented before printing the
current handle value.

Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
---
 drivers/cxl/core/mbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 9adda4795eb7..b810a6aa3010 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -915,7 +915,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
 
 		payload->handles[i++] = gen->hdr.handle;
 		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
-			le16_to_cpu(payload->handles[i]));
+			le16_to_cpu(payload->handles[i-1]));
 
 		if (i == max_handles) {
 			payload->nr_recs = i;
-- 
2.34.1

Re: [PATCH v2 1/1] cxl/mem: Fix for the index of Clear Event Record Handle

[Jonathan Cameron via]

On Mon, 18 Mar 2024 10:29:28 +0800
Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:

> The dev_dbg info for Clear Event Records mailbox command would report
> the handle of the next record to clear not the current one.
> 
> This was because the index 'i' had incremented before printing the
> current handle value.
> 
> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> ---
>  drivers/cxl/core/mbox.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9adda4795eb7..b810a6aa3010 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -915,7 +915,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  
>  		payload->handles[i++] = gen->hdr.handle;
>  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
> -			le16_to_cpu(payload->handles[i]));
> +			le16_to_cpu(payload->handles[i-1]));
Trivial but needs spaces around the -. e.g.  [i - 1] 

Maybe Dan can fix up whilst applying.

Otherwise

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

>  
>  		if (i == max_handles) {
>  			payload->nr_recs = i;

Re: [PATCH v2 1/1] cxl/mem: Fix for the index of Clear Event Record Handle

[Dan Williams]

Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 10:29:28 +0800
> Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:
> 
> > The dev_dbg info for Clear Event Records mailbox command would report
> > the handle of the next record to clear not the current one.
> > 
> > This was because the index 'i' had incremented before printing the
> > current handle value.
> > 
> > Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> > ---
> >  drivers/cxl/core/mbox.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 9adda4795eb7..b810a6aa3010 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -915,7 +915,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
> >  
> >  		payload->handles[i++] = gen->hdr.handle;
> >  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
> > -			le16_to_cpu(payload->handles[i]));
> > +			le16_to_cpu(payload->handles[i-1]));
> Trivial but needs spaces around the -. e.g.  [i - 1] 
> 
> Maybe Dan can fix up whilst applying.
> 
> Otherwise
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I have enlisted Dave to start wrangling CXL kernel patches upstream, and
I will fall back to just reviewing.

Dave, you can add my:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...with the same caveat as above.

Re: [PATCH v2 1/1] cxl/mem: Fix for the index of Clear Event Record Handle

[Dave Jiang]

On 3/18/24 5:38 PM, Dan Williams wrote:
> Jonathan Cameron wrote:
>> On Mon, 18 Mar 2024 10:29:28 +0800
>> Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote:
>>
>>> The dev_dbg info for Clear Event Records mailbox command would report
>>> the handle of the next record to clear not the current one.
>>>
>>> This was because the index 'i' had incremented before printing the
>>> current handle value.
>>>
>>> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
>>> ---
>>>  drivers/cxl/core/mbox.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>>> index 9adda4795eb7..b810a6aa3010 100644
>>> --- a/drivers/cxl/core/mbox.c
>>> +++ b/drivers/cxl/core/mbox.c
>>> @@ -915,7 +915,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>>>  
>>>  		payload->handles[i++] = gen->hdr.handle;
>>>  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
>>> -			le16_to_cpu(payload->handles[i]));
>>> +			le16_to_cpu(payload->handles[i-1]));
>> Trivial but needs spaces around the -. e.g.  [i - 1] 
>>
>> Maybe Dan can fix up whilst applying.
>>
>> Otherwise
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> I have enlisted Dave to start wrangling CXL kernel patches upstream, and
> I will fall back to just reviewing.
> 
> Dave, you can add my:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 
> ...with the same caveat as above.

Applied, updated, and added 
Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")

> 

Re: [PATCH v2 1/1] cxl/mem: Fix for the index of Clear Event Record Handle

[fan]

On Mon, Mar 18, 2024 at 10:29:28AM +0800, Yuquan Wang wrote:
> The dev_dbg info for Clear Event Records mailbox command would report
> the handle of the next record to clear not the current one.
> 
> This was because the index 'i' had incremented before printing the
> current handle value.
> 
> Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn>
> ---
>  drivers/cxl/core/mbox.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 9adda4795eb7..b810a6aa3010 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -915,7 +915,7 @@ static int cxl_clear_event_record(struct cxl_memdev_state *mds,
>  
>  		payload->handles[i++] = gen->hdr.handle;
>  		dev_dbg(mds->cxlds.dev, "Event log '%d': Clearing %u\n", log,
> -			le16_to_cpu(payload->handles[i]));
> +			le16_to_cpu(payload->handles[i-1]));

LGTM except for the space issue mentioned by Jonathan.

After the fix,
Reviewed-by: Fan Ni <fan.ni@samsung.com>

Fan
>  
>  		if (i == max_handles) {
>  			payload->nr_recs = i;
> -- 
> 2.34.1
>