2019-05-07 17:00:21

by Akinobu Mita

[permalink] [raw]
Subject: [PATCH v2 4/7] nvme.h: add telemetry log page definisions

Copy telemetry log page definisions from nvme-cli.

Cc: Johannes Berg <[email protected]>
Cc: Keith Busch <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Minwoo Im <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
---
* v2
- New patch in this version.

include/linux/nvme.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c40720c..5217fe4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -396,6 +396,28 @@ enum {
NVME_NIDT_UUID = 0x03,
};

+/* Derived from 1.3a Figure 101: Get Log Page – Telemetry Host
+ * -Initiated Log (Log Identifier 07h)
+ */
+struct nvme_telemetry_log_page_hdr {
+ __u8 lpi; /* Log page identifier */
+ __u8 rsvd[4];
+ __u8 iee_oui[3];
+ __le16 dalb1; /* Data area 1 last block */
+ __le16 dalb2; /* Data area 2 last block */
+ __le16 dalb3; /* Data area 3 last block */
+ __u8 rsvd1[368]; /* TODO verify */
+ __u8 ctrlavail; /* Controller initiated data avail?*/
+ __u8 ctrldgn; /* Controller initiated telemetry Data Gen # */
+ __u8 rsnident[128];
+ /* We'll have to double fetch so we can get the header,
+ * parse dalb1->3 determine how much size we need for the
+ * log then alloc below. Or just do a secondary non-struct
+ * allocation.
+ */
+ __u8 telemetry_dataarea[0];
+};
+
struct nvme_smart_log {
__u8 critical_warning;
__u8 temperature[2];
@@ -832,6 +854,7 @@ enum {
NVME_LOG_FW_SLOT = 0x03,
NVME_LOG_CHANGED_NS = 0x04,
NVME_LOG_CMD_EFFECTS = 0x05,
+ NVME_LOG_TELEMETRY_CTRL = 0x08,
NVME_LOG_ANA = 0x0c,
NVME_LOG_DISC = 0x70,
NVME_LOG_RESERVATION = 0x80,
--
2.7.4


2019-05-07 17:29:40

by Heitke, Kenneth

[permalink] [raw]
Subject: Re: [PATCH v2 4/7] nvme.h: add telemetry log page definisions



On 5/7/2019 10:58 AM, Akinobu Mita wrote:
> Copy telemetry log page definisions from nvme-cli.
>
> Cc: Johannes Berg <[email protected]>
> Cc: Keith Busch <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Minwoo Im <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
> ---
> * v2
> - New patch in this version.
>
> include/linux/nvme.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index c40720c..5217fe4 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -396,6 +396,28 @@ enum {
> NVME_NIDT_UUID = 0x03,
> };
>
> +/* Derived from 1.3a Figure 101: Get Log Page – Telemetry Host
> + * -Initiated Log (Log Identifier 07h)
> + */
> +struct nvme_telemetry_log_page_hdr {
> + __u8 lpi; /* Log page identifier */
> + __u8 rsvd[4];
> + __u8 iee_oui[3];
> + __le16 dalb1; /* Data area 1 last block */
> + __le16 dalb2; /* Data area 2 last block */
> + __le16 dalb3; /* Data area 3 last block */
> + __u8 rsvd1[368]; /* TODO verify */

Remove the TODO

> + __u8 ctrlavail; /* Controller initiated data avail?*/
> + __u8 ctrldgn; /* Controller initiated telemetry Data Gen # */
> + __u8 rsnident[128];
> + /* We'll have to double fetch so we can get the header,
> + * parse dalb1->3 determine how much size we need for the
> + * log then alloc below. Or just do a secondary non-struct
> + * allocation.
> + */

This comment isn't necessary. You usually can't read the entire
telemetry log at once and the header is a fixed size. You would likely
just read the header followed by reads of the different data areas.

> + __u8 telemetry_dataarea[0];
> +};
> +
> struct nvme_smart_log {
> __u8 critical_warning;
> __u8 temperature[2];
> @@ -832,6 +854,7 @@ enum {
> NVME_LOG_FW_SLOT = 0x03,
> NVME_LOG_CHANGED_NS = 0x04,
> NVME_LOG_CMD_EFFECTS = 0x05,
> + NVME_LOG_TELEMETRY_CTRL = 0x08,
> NVME_LOG_ANA = 0x0c,
> NVME_LOG_DISC = 0x70,
> NVME_LOG_RESERVATION = 0x80,
>

2019-05-07 17:54:15

by Heitke, Kenneth

[permalink] [raw]
Subject: Re: [PATCH v2 4/7] nvme.h: add telemetry log page definisions



On 5/7/2019 10:58 AM, Akinobu Mita wrote:
> Copy telemetry log page definisions from nvme-cli.
>
> Cc: Johannes Berg <[email protected]>
> Cc: Keith Busch <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Minwoo Im <[email protected]>
> Signed-off-by: Akinobu Mita <[email protected]>
> ---
> * v2
> - New patch in this version.
>
> include/linux/nvme.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index c40720c..5217fe4 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -396,6 +396,28 @@ enum {
> NVME_NIDT_UUID = 0x03,
> };
>
> +/* Derived from 1.3a Figure 101: Get Log Page – Telemetry Host
> + * -Initiated Log (Log Identifier 07h)
> + */

Is this Host Initiated or Controller Initiated? The comment says host
initiated but everything else seems to indicated controller initiated.
Is controller initiated even the correct choice because the controller
would have sent an AER to indicate that the host should pull the
telemetry data.


> +struct nvme_telemetry_log_page_hdr {
> + __u8 lpi; /* Log page identifier */
> + __u8 rsvd[4];
> + __u8 iee_oui[3];
> + __le16 dalb1; /* Data area 1 last block */
> + __le16 dalb2; /* Data area 2 last block */
> + __le16 dalb3; /* Data area 3 last block */
> + __u8 rsvd1[368]; /* TODO verify */
> + __u8 ctrlavail; /* Controller initiated data avail?*/
> + __u8 ctrldgn; /* Controller initiated telemetry Data Gen # */
> + __u8 rsnident[128];
> + /* We'll have to double fetch so we can get the header,
> + * parse dalb1->3 determine how much size we need for the
> + * log then alloc below. Or just do a secondary non-struct
> + * allocation.
> + */
> + __u8 telemetry_dataarea[0];
> +};
> +
> struct nvme_smart_log {
> __u8 critical_warning;
> __u8 temperature[2];
> @@ -832,6 +854,7 @@ enum {
> NVME_LOG_FW_SLOT = 0x03,
> NVME_LOG_CHANGED_NS = 0x04,
> NVME_LOG_CMD_EFFECTS = 0x05,
> + NVME_LOG_TELEMETRY_CTRL = 0x08,
> NVME_LOG_ANA = 0x0c,
> NVME_LOG_DISC = 0x70,
> NVME_LOG_RESERVATION = 0x80,
>

2019-05-08 15:56:42

by Akinobu Mita

[permalink] [raw]
Subject: Re: [PATCH v2 4/7] nvme.h: add telemetry log page definisions

2019年5月8日(水) 2:53 Heitke, Kenneth <[email protected]>:
>
>
>
> On 5/7/2019 10:58 AM, Akinobu Mita wrote:
> > Copy telemetry log page definisions from nvme-cli.
> >
> > Cc: Johannes Berg <[email protected]>
> > Cc: Keith Busch <[email protected]>
> > Cc: Jens Axboe <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Sagi Grimberg <[email protected]>
> > Cc: Minwoo Im <[email protected]>
> > Signed-off-by: Akinobu Mita <[email protected]>
> > ---
> > * v2
> > - New patch in this version.
> >
> > include/linux/nvme.h | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> > index c40720c..5217fe4 100644
> > --- a/include/linux/nvme.h
> > +++ b/include/linux/nvme.h
> > @@ -396,6 +396,28 @@ enum {
> > NVME_NIDT_UUID = 0x03,
> > };
> >
> > +/* Derived from 1.3a Figure 101: Get Log Page – Telemetry Host
> > + * -Initiated Log (Log Identifier 07h)
> > + */
>
> Is this Host Initiated or Controller Initiated? The comment says host
> initiated but everything else seems to indicated controller initiated.

Both telemetry host initiated and controller initiated log headers have
the same structure. If this comment is confusing, it is also considered
to be removed.

> Is controller initiated even the correct choice because the controller
> would have sent an AER to indicate that the host should pull the
> telemetry data.

It seems useful to retrieve telemetry log continually with the aid of
user space tool reacting an Asynchronous Event.

Similarly, it could be useful to retrieve telemetry log as soon as the
device is successfully recovered from the crash. (Although I still do
not find the device that has Telemetry Controller-Initiated Data Available
field is set to 1h.)

2019-05-08 17:47:16

by Akinobu Mita

[permalink] [raw]
Subject: Re: [PATCH v2 4/7] nvme.h: add telemetry log page definisions

2019年5月8日(水) 2:28 Heitke, Kenneth <[email protected]>:
>
>
>
> On 5/7/2019 10:58 AM, Akinobu Mita wrote:
> > Copy telemetry log page definisions from nvme-cli.
> >
> > Cc: Johannes Berg <[email protected]>
> > Cc: Keith Busch <[email protected]>
> > Cc: Jens Axboe <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Sagi Grimberg <[email protected]>
> > Cc: Minwoo Im <[email protected]>
> > Signed-off-by: Akinobu Mita <[email protected]>
> > ---
> > * v2
> > - New patch in this version.
> >
> > include/linux/nvme.h | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> > index c40720c..5217fe4 100644
> > --- a/include/linux/nvme.h
> > +++ b/include/linux/nvme.h
> > @@ -396,6 +396,28 @@ enum {
> > NVME_NIDT_UUID = 0x03,
> > };
> >
> > +/* Derived from 1.3a Figure 101: Get Log Page – Telemetry Host
> > + * -Initiated Log (Log Identifier 07h)
> > + */
> > +struct nvme_telemetry_log_page_hdr {
> > + __u8 lpi; /* Log page identifier */
> > + __u8 rsvd[4];
> > + __u8 iee_oui[3];
> > + __le16 dalb1; /* Data area 1 last block */
> > + __le16 dalb2; /* Data area 2 last block */
> > + __le16 dalb3; /* Data area 3 last block */
> > + __u8 rsvd1[368]; /* TODO verify */
>
> Remove the TODO

OK.

> > + __u8 ctrlavail; /* Controller initiated data avail?*/
> > + __u8 ctrldgn; /* Controller initiated telemetry Data Gen # */
> > + __u8 rsnident[128];
> > + /* We'll have to double fetch so we can get the header,
> > + * parse dalb1->3 determine how much size we need for the
> > + * log then alloc below. Or just do a secondary non-struct
> > + * allocation.
> > + */
>
> This comment isn't necessary. You usually can't read the entire
> telemetry log at once and the header is a fixed size. You would likely
> just read the header followed by reads of the different data areas.

This comment is derived from nvme-cli. So firstly, I'll send a patch
for nvme-cli. If the changes are accepted, I'll update this comment, too.

> > + __u8 telemetry_dataarea[0];
> > +};
> > +
> > struct nvme_smart_log {
> > __u8 critical_warning;
> > __u8 temperature[2];
> > @@ -832,6 +854,7 @@ enum {
> > NVME_LOG_FW_SLOT = 0x03,
> > NVME_LOG_CHANGED_NS = 0x04,
> > NVME_LOG_CMD_EFFECTS = 0x05,
> > + NVME_LOG_TELEMETRY_CTRL = 0x08,
> > NVME_LOG_ANA = 0x0c,
> > NVME_LOG_DISC = 0x70,
> > NVME_LOG_RESERVATION = 0x80,
> >