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
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 <[email protected]>
---
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
On Mon, 18 Mar 2024 10:29:28 +0800
Yuquan Wang <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
>
> if (i == max_handles) {
> payload->nr_recs = i;
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 <[email protected]>
> ---
> 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 <[email protected]>
Fan
>
> if (i == max_handles) {
> payload->nr_recs = i;
> --
> 2.34.1
>
Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 10:29:28 +0800
> Yuquan Wang <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
..with the same caveat as above.
On 3/18/24 5:38 PM, Dan Williams wrote:
> Jonathan Cameron wrote:
>> On Mon, 18 Mar 2024 10:29:28 +0800
>> Yuquan Wang <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
> 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 <[email protected]>
>
> ...with the same caveat as above.
Applied, updated, and added
Fixes: 6ebe28f9ec72 ("cxl/mem: Read, trace, and clear events on driver load")
>