[PATCH 0/3] *** nvme: add some commands tracing ***

[PATCH 0/3] *** nvme: add some commands tracing ***

[Guixin Liu]

Hi guys:
    I found that there are no reservation commands tracing and some
commands's parsing aren't fit the newest NVMe spec, and aren't readble.
    Changed as follow:

Guixin Liu (3):
  nvme: add tracing of reservation commands
  nvme: parse dsm command detailly when tracing
  nvme: parse format command's lbafu when tracing

 drivers/nvme/host/trace.c | 74 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 1/3] nvme: add tracing of reservation commands

[Guixin Liu]

Add detailed parsing of reservation commands to make the trace log
more consistent and human-readable.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/trace.c | 62 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index 1c36fcedea20..95572c85b22a 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -191,6 +191,60 @@ static const char *nvme_trace_zone_mgmt_recv(struct trace_seq *p, u8 *cdw10)
 	return ret;
 }
 
+static const char *nvme_trace_resv_reg(struct trace_seq *p, u8 *cdw10)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 rrega = cdw10[0] & 0x7;
+	u8 iekey = (cdw10[0] >> 3) & 0x1;
+	u8 ptpl = (cdw10[3] >> 6) & 0x3;
+
+	trace_seq_printf(p, "rrega=%u, iekey=%u, ptpl=%u",
+			 rrega, iekey, ptpl);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+static const char *nvme_trace_resv_acq(struct trace_seq *p, u8 *cdw10)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 racqa = cdw10[0] & 0x7;
+	u8 iekey = (cdw10[0] >> 3) & 0x1;
+	u8 rtype = cdw10[1];
+
+	trace_seq_printf(p, "racqa=%u, iekey=%u, rtype=%u",
+			 racqa, iekey, rtype);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+static const char *nvme_trace_resv_rel(struct trace_seq *p, u8 *cdw10)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 rrela = cdw10[0] & 0x7;
+	u8 iekey = (cdw10[0] >> 3) & 0x1;
+	u8 rtype = cdw10[1];
+
+	trace_seq_printf(p, "rrela=%u, iekey=%u, rtype=%u",
+			 rrela, iekey, rtype);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+static const char *nvme_trace_resv_report(struct trace_seq *p, u8 *cdw10)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u32 numd = get_unaligned_le32(cdw10);
+	u8 eds = cdw10[4] & 0x1;
+
+	trace_seq_printf(p, "numd=%u, eds=%u", numd, eds);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
 static const char *nvme_trace_common(struct trace_seq *p, u8 *cdw10)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
@@ -243,6 +297,14 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p,
 		return nvme_trace_zone_mgmt_send(p, cdw10);
 	case nvme_cmd_zone_mgmt_recv:
 		return nvme_trace_zone_mgmt_recv(p, cdw10);
+	case nvme_cmd_resv_register:
+		return nvme_trace_resv_reg(p, cdw10);
+	case nvme_cmd_resv_acquire:
+		return nvme_trace_resv_acq(p, cdw10);
+	case nvme_cmd_resv_release:
+		return nvme_trace_resv_rel(p, cdw10);
+	case nvme_cmd_resv_report:
+		return nvme_trace_resv_report(p, cdw10);
 	default:
 		return nvme_trace_common(p, cdw10);
 	}
-- 
2.43.0

[PATCH 2/3] nvme: parse dsm command detailly when tracing

[Guixin Liu]

Add detailed parse of dsm command to make the trace log more
consistent and human-readable.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/trace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index 95572c85b22a..b383d51c1a0c 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -153,10 +153,12 @@ static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
 static const char *nvme_trace_dsm(struct trace_seq *p, u8 *cdw10)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
+	u8 nr = cdw10[0];
+	u8 idr = cdw10[4] & 0x1;
+	u8 idw = (cdw10[4] >> 1) & 0x1;
+	u8 ad = (cdw10[4] >> 2) & 0x1;
 
-	trace_seq_printf(p, "nr=%u, attributes=%u",
-			 get_unaligned_le32(cdw10),
-			 get_unaligned_le32(cdw10 + 4));
+	trace_seq_printf(p, "nr=%u, idr=%u, idw=%u, ad=%u", nr, idr, idw, ad);
 	trace_seq_putc(p, 0);
 
 	return ret;
-- 
2.43.0

Re: [PATCH 2/3] nvme: parse dsm command detailly when tracing

[Chaitanya Kulkarni]

On 1/29/24 00:22, Guixin Liu wrote:
> Add detailed parse of dsm command to make the trace log more
> consistent and human-readable.
>
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/host/trace.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> index 95572c85b22a..b383d51c1a0c 100644
> --- a/drivers/nvme/host/trace.c
> +++ b/drivers/nvme/host/trace.c
> @@ -153,10 +153,12 @@ static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
>   static const char *nvme_trace_dsm(struct trace_seq *p, u8 *cdw10)
>   {
>   	const char *ret = trace_seq_buffer_ptr(p);
> +	u8 nr = cdw10[0];
> +	u8 idr = cdw10[4] & 0x1;
> +	u8 idw = (cdw10[4] >> 1) & 0x1;
> +	u8 ad = (cdw10[4] >> 2) & 0x1;
>   
> -	trace_seq_printf(p, "nr=%u, attributes=%u",
> -			 get_unaligned_le32(cdw10),
> -			 get_unaligned_le32(cdw10 + 4));
> +	trace_seq_printf(p, "nr=%u, idr=%u, idw=%u, ad=%u", nr, idr, idw, ad);
>   	trace_seq_putc(p, 0);
>   
>   	return ret;

I'm really not sure if we need to decode idw/idr as I've not seen those 
fields so far
used by anybody maybe I'm not aware here, but can you please provide a 
usecase where
you need these fields to be decoded ?

-ck

Re: [PATCH 2/3] nvme: parse dsm command detailly when tracing

[Guixin Liu]

在 2024/1/29 19:09, Chaitanya Kulkarni 写道:
> On 1/29/24 00:22, Guixin Liu wrote:
>> Add detailed parse of dsm command to make the trace log more
>> consistent and human-readable.
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>    drivers/nvme/host/trace.c | 8 +++++---
>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
>> index 95572c85b22a..b383d51c1a0c 100644
>> --- a/drivers/nvme/host/trace.c
>> +++ b/drivers/nvme/host/trace.c
>> @@ -153,10 +153,12 @@ static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
>>    static const char *nvme_trace_dsm(struct trace_seq *p, u8 *cdw10)
>>    {
>>    	const char *ret = trace_seq_buffer_ptr(p);
>> +	u8 nr = cdw10[0];
>> +	u8 idr = cdw10[4] & 0x1;
>> +	u8 idw = (cdw10[4] >> 1) & 0x1;
>> +	u8 ad = (cdw10[4] >> 2) & 0x1;
>>    
>> -	trace_seq_printf(p, "nr=%u, attributes=%u",
>> -			 get_unaligned_le32(cdw10),
>> -			 get_unaligned_le32(cdw10 + 4));
>> +	trace_seq_printf(p, "nr=%u, idr=%u, idw=%u, ad=%u", nr, idr, idw, ad);
>>    	trace_seq_putc(p, 0);
>>    
>>    	return ret;
> I'm really not sure if we need to decode idw/idr as I've not seen those
> fields so far
> used by anybody maybe I'm not aware here, but can you please provide a
> usecase where
> you need these fields to be decoded ?
>
> -ck

Mostly we only use "ad" filed, but sometimes our tester will set idw/idr to

see whether any errors have been reported by our target. Well this is a

low-persuasive usecase, but I still insist it's better to follow the spec.

Best regadrs,

Guixin Liu

>
>

Re: [PATCH 2/3] nvme: parse dsm command detailly when tracing

[Chaitanya Kulkarni]

>> I'm really not sure if we need to decode idw/idr as I've not seen those
>> fields so far
>> used by anybody maybe I'm not aware here, but can you please provide a
>> usecase where
>> you need these fields to be decoded ?
>>
>> -ck
> 
> Mostly we only use "ad" filed, but sometimes our tester will set idw/idr to
> 
> see whether any errors have been reported by our target. Well this is a
> 
> low-persuasive usecase, but I still insist it's better to follow the spec.
> 
> Best regadrs,
> 
> Guixin Liu
> 
>>

But why ? do you have a valid implementation that shows clear benefits
for idw/idr ? if not I'm not sure why we need to decode these fields,
unless others think it's a good idea let's not do that.

Following spec only makes sense where there is a valid implementation
for the fields we are printing and not sure if it applies to idw/idr ...

I'm fine with AD and NR they are useful ...

-ck

Re: [PATCH 2/3] nvme: parse dsm command detailly when tracing

[Guixin Liu]

在 2024/1/30 16:05, Chaitanya Kulkarni 写道:
>>> I'm really not sure if we need to decode idw/idr as I've not seen those
>>> fields so far
>>> used by anybody maybe I'm not aware here, but can you please provide a
>>> usecase where
>>> you need these fields to be decoded ?
>>>
>>> -ck
>> Mostly we only use "ad" filed, but sometimes our tester will set idw/idr to
>>
>> see whether any errors have been reported by our target. Well this is a
>>
>> low-persuasive usecase, but I still insist it's better to follow the spec.
>>
>> Best regadrs,
>>
>> Guixin Liu
>>
> But why ? do you have a valid implementation that shows clear benefits
> for idw/idr ? if not I'm not sure why we need to decode these fields,
> unless others think it's a good idea let's not do that.
>
> Following spec only makes sense where there is a valid implementation
> for the fields we are printing and not sure if it applies to idw/idr ...
>
> I'm fine with AD and NR they are useful ...
>
> -ck
Okay, it will be changed in v2.
>

Re: [PATCH 2/3] nvme: parse dsm command detailly when tracing

[Keith Busch]

On Mon, Jan 29, 2024 at 11:09:38AM +0000, Chaitanya Kulkarni wrote:
> On 1/29/24 00:22, Guixin Liu wrote:
> > Add detailed parse of dsm command to make the trace log more
> > consistent and human-readable.
> >
> > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> > ---
> >   drivers/nvme/host/trace.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> > index 95572c85b22a..b383d51c1a0c 100644
> > --- a/drivers/nvme/host/trace.c
> > +++ b/drivers/nvme/host/trace.c
> > @@ -153,10 +153,12 @@ static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
> >   static const char *nvme_trace_dsm(struct trace_seq *p, u8 *cdw10)
> >   {
> >   	const char *ret = trace_seq_buffer_ptr(p);
> > +	u8 nr = cdw10[0];
> > +	u8 idr = cdw10[4] & 0x1;
> > +	u8 idw = (cdw10[4] >> 1) & 0x1;
> > +	u8 ad = (cdw10[4] >> 2) & 0x1;
> >   
> > -	trace_seq_printf(p, "nr=%u, attributes=%u",
> > -			 get_unaligned_le32(cdw10),
> > -			 get_unaligned_le32(cdw10 + 4));
> > +	trace_seq_printf(p, "nr=%u, idr=%u, idw=%u, ad=%u", nr, idr, idw, ad);
> >   	trace_seq_putc(p, 0);
> >   
> >   	return ret;
> 
> I'm really not sure if we need to decode idw/idr as I've not seen those 
> fields so far
> used by anybody maybe I'm not aware here, but can you please provide a 
> usecase where
> you need these fields to be decoded ?

It's only reachable through passthrough. I'm not aware of a production
use case, but I know there are test cases that do muck with idr/idw
attributes.

I'm generally not a fan of decoding complex fields because it's not
forward compatible: a newer drive in an older linux can't trace command
bits that are currently reserved.

[PATCH 3/3] nvme: parse format command's lbafu when tracing

[Guixin Liu]

Add the parse of format command's lbafu to calculate lbaf.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/trace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index b383d51c1a0c..3ccb33646794 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -119,7 +119,9 @@ static const char *nvme_trace_get_lba_status(struct trace_seq *p,
 static const char *nvme_trace_admin_format_nvm(struct trace_seq *p, u8 *cdw10)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
-	u8 lbaf = cdw10[0] & 0xF;
+	u8 lbafl = cdw10[0] & 0xF;
+	u8 lbafu = (cdw10[1] >> 4) & 0x3;
+	u8 lbaf = (lbafu << 4) | lbafl;
 	u8 mset = (cdw10[0] >> 4) & 0x1;
 	u8 pi = (cdw10[0] >> 5) & 0x7;
 	u8 pil = cdw10[1] & 0x1;
-- 
2.43.0

Re: [PATCH 3/3] nvme: parse format command's lbafu when tracing

[Chaitanya Kulkarni]

On 1/29/24 00:22, Guixin Liu wrote:
> Add the parse of format command's lbafu to calculate lbaf.
>
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/host/trace.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> index b383d51c1a0c..3ccb33646794 100644
> --- a/drivers/nvme/host/trace.c
> +++ b/drivers/nvme/host/trace.c
> @@ -119,7 +119,9 @@ static const char *nvme_trace_get_lba_status(struct trace_seq *p,
>   static const char *nvme_trace_admin_format_nvm(struct trace_seq *p, u8 *cdw10)
>   {
>   	const char *ret = trace_seq_buffer_ptr(p);
> -	u8 lbaf = cdw10[0] & 0xF;
> +	u8 lbafl = cdw10[0] & 0xF;
> +	u8 lbafu = (cdw10[1] >> 4) & 0x3;
> +	u8 lbaf = (lbafu << 4) | lbafl;
>   	u8 mset = (cdw10[0] >> 4) & 0x1;
>   	u8 pi = (cdw10[0] >> 5) & 0x7;
>   	u8 pil = cdw10[1] & 0x1;

as I don't see the newly added variables used in printing a trace do we 
really need
the calculation or a meaningful comment is would work ?

-ck

Re: [PATCH 3/3] nvme: parse format command's lbafu when tracing

[Guixin Liu]

在 2024/1/29 19:01, Chaitanya Kulkarni 写道:
> On 1/29/24 00:22, Guixin Liu wrote:
>> Add the parse of format command's lbafu to calculate lbaf.
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>    drivers/nvme/host/trace.c | 4 +++-
>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
>> index b383d51c1a0c..3ccb33646794 100644
>> --- a/drivers/nvme/host/trace.c
>> +++ b/drivers/nvme/host/trace.c
>> @@ -119,7 +119,9 @@ static const char *nvme_trace_get_lba_status(struct trace_seq *p,
>>    static const char *nvme_trace_admin_format_nvm(struct trace_seq *p, u8 *cdw10)
>>    {
>>    	const char *ret = trace_seq_buffer_ptr(p);
>> -	u8 lbaf = cdw10[0] & 0xF;
>> +	u8 lbafl = cdw10[0] & 0xF;
>> +	u8 lbafu = (cdw10[1] >> 4) & 0x3;
>> +	u8 lbaf = (lbafu << 4) | lbafl;
>>    	u8 mset = (cdw10[0] >> 4) & 0x1;
>>    	u8 pi = (cdw10[0] >> 5) & 0x7;
>>    	u8 pil = cdw10[1] & 0x1;
> as I don't see the newly added variables used in printing a trace do we
> really need
> the calculation or a meaningful comment is would work ?
>
> -ck
OK, I will delete lbafl and lbafu variables, and add a comment for lbaf.
>
>