2 patches about asynchronous events notification

The 2 patches in this small series are related to asynchronous events' handling.

The 1st aims to print textual information instead of just print the event code. The 2nd patch enables asynchronous events notification by default.

There are some points below, for which I'd be glad to hear some suggestions!

1) In the 1st patch, we do first a direct lookup in a table and then we fallback to full table search in case direct lookup does not succeed. We can rely in direct lookup solely if we are sure the NVMe spec. won't ever add events with non-incremental codes. Since I cannot guarantee this myself, my choice was to keep the full table search (which code won't be reached with current events of the spec.);

2) The 2nd patch was sent as a RFC, because I'm not sure if there is some con in enabling those events by default; I might be missing some caveat here.

Thanks in advance.

Guilherme G. Piccoli (2):
nvme: introduce asynchronous events textual output
nvme: enable asynchronous events notification by default

 drivers/nvme/host/core.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  9 ++++++
 drivers/nvme/host/pci.c  | 16 ++++++++++
 3 files changed, 102 insertions(+), 2 deletions(-)

2.1.0

Currently the nvme driver prints asynchronous events as numbers, based on NVMe specification codes. This patch introduces the textual output for such events, keeping the codes too.

This is useful for error reporting purposes in human readable format.

---
 drivers/nvme/host/core.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  9 ++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

index 1a51584..8f95947 100644
@@ -58,6 +58,79 @@ static DEFINE_SPINLOCK(dev_list_lock);
 
 static struct class *nvme_class;
 
+/* NVMe Async Events descriptive messages:
+ * Error status messages - Type 0x0
+ */
+static const struct nvme_ae_info nvme_ae_error_status[] = {
+	{ 0x0, "Write to invalid doorbell register"	},
+	{ 0x1, "Invalid doorbell write value"		},
+	{ 0x2, "Diagnostic failure"			},
+	{ 0x3, "Persistent internal error"		},
+	{ 0x4, "Transient internal error"		},
+	{ 0x5, "Firmware image load error"		},
+};
+
+/* SMART/Health status messages - Type 0x1 */
+static const struct nvme_ae_info nvme_ae_health_status[] = {
+	{ 0x0, "NVM subsystem reliability"		},
+	{ 0x1, "Temperature threshold"			},
+	{ 0x2, "Spare below threshold"			},
+};
+
+/* Notice messages - Type 0x2
+ * Event 0x0 in the below table is referred as NVME_AER_NOTICE_NS_CHANGED on
+ * nvme.h and is not just reported; the driver perform a queue rescan too.
+ */
+static const struct nvme_ae_info nvme_ae_notice[] = {
+	{ 0x0, "Namespace attribute changed"		},
+	{ 0x1, "Firmware activation starting"		},
+};
+
+/* NVM/IO Command Set Specific Status messages - Type 0x6 */
+static const struct nvme_ae_info nvme_ae_io_command_set[] = {
+	{ 0x0, "Reservation log page available"		},
+};
+
+static const char *nvme_get_ae_info(u32 result)
+{
+	const struct nvme_ae_info *tbl;
+	u8 evnt, size, type, i;
+
+	type = result & NVME_AE_TYPE_MASK;
+	evnt = (result & NVME_AE_INFO_MASK) >> 8;
+
+	switch (type) {
+	case 0x0: /* Error status */
+		tbl = nvme_ae_error_status;
+		size = ARRAY_SIZE(nvme_ae_error_status);
+		break;
+	case 0x1: /* SMART/Health status */
+		tbl = nvme_ae_health_status;
+		size = ARRAY_SIZE(nvme_ae_health_status);
+		break;
+	case 0x2: /* Notice */
+		tbl = nvme_ae_notice;
+		size = ARRAY_SIZE(nvme_ae_notice);
+		break;
+	case 0x6: /* NVM/IO Command Set Specific Status */
+		tbl = nvme_ae_io_command_set;
+		size = ARRAY_SIZE(nvme_ae_io_command_set);
+		break;
+	default: /* Reserved types/Vendor specific */
+		return NULL;
+	}
+
+	/* Try first a direct lookup to avoid full table search */
+	if (evnt < size && tbl[evnt].event == evnt)
+		return tbl[evnt].desc;
+
+	for (i = 0; i < size; i++)
+		if (tbl[i].event == evnt)
+			return tbl[i].desc;
+
+	return NULL;
+}
+
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state)
 {

@@ -1656,11 +1729,13 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl,
 
 	switch (result & 0xff07) {
 	case NVME_AER_NOTICE_NS_CHANGED:
-		dev_info(ctrl->device, "rescanning\n");
+		dev_info(ctrl->device,
+			"rescanning (Namespace attribute change detected)\n");
 		nvme_queue_scan(ctrl);
 		break;
 	default:
-		dev_warn(ctrl->device, "async event result %08x\n", result);
+		dev_warn(ctrl->device, "async event result: %s (%08x)\n",
+					nvme_get_ae_info(result), result);
 	}
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);

index 1daa048..a075eb1 100644
@@ -154,6 +154,15 @@ struct nvme_ctrl_ops {
 	void (*submit_async_event)(struct nvme_ctrl *ctrl, int aer_idx);
 };
 
+/* NVMe Asynchronous Events (AE) textual parsing helpers */
+#define NVME_AE_TYPE_MASK	0x7
+#define NVME_AE_INFO_MASK	0xFF00
+
+struct nvme_ae_info {
+	u8 event;
+	const char *desc;
+};
+
 static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)
 {
 	u32 val = 0;

2.1.0

Asynchronous events notification currently is disabled by default; to enable it, one should issue a set-feature command through nvme-cli userspace application. The tool also allows disabling these events once they're enable, as per user desire.

This patch makes the asynchronous events notification enabled by default; to do so, we submit the set-feature command from the driver, in the end of nvme_reset_work() routine. This way, the feature is enabled on the driver initialization and after resets, in case they happen.

Enabling the asynchronous events notification feature by default is interesting as an error report feature.

---
 drivers/nvme/host/pci.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index befac5b..3a82390 100644
@@ -1779,6 +1779,18 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev, int status)
 		nvme_put_ctrl(&dev->ctrl);
 }
 
+static int nvme_enable_async_events(struct nvme_dev *dev)
+{
+	struct nvme_command c;
+
+	memset(&c, 0, sizeof(c));
+	c.common.opcode = nvme_admin_set_features;
+	c.common.cdw10[0] = 0xB; /* Feature: Async Event Configuration */
+	c.common.cdw10[1] = 0x3FF; /* Enable all available events */
+
+	return nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0);
+}
+
 static void nvme_reset_work(struct work_struct *work)
 {
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work);

@@ -1849,6 +1861,10 @@ static void nvme_reset_work(struct work_struct *work)
 
 	if (dev->online_queues > 1)
 		nvme_queue_scan(&dev->ctrl);
+
+	if (nvme_enable_async_events(dev))
+		dev_info(dev->ctrl.device,
+			"failed to enable async events notification\n");
 	return;
 
  out:

2.1.0

AEN was already enabled by default in:

commit f866fc4282a81673ef973ad54c68235a3263b42e

Author: Christoph Hellwig <hch at lst.de>
Date:   Tue Apr 26 13:52:00 2016 +0200

nvme: move AER handling to common code

The transport driver still needs to do the actual submission, but
all the
higher level code can be shared.

(Assuming you have at least a single queue).

Sagi, I might be wrong in my understanding (sorry in advance if this is the case hehe) but I guess the above commit only re-factored the async events handling on driver - the functions reworked by the patch aim to handle the events once they are 'captured', which is feasible only if they are enabled by setting the feature (with nvme-cli, for example).

The goal of my patch is to do the 'job' of nvme-cli automatically from within the driver.

One thing I was thinking is to add a sysfs parameter to be able to avoid the automatic enablement of async events in case we have a bogus adapter showing too many events, for example...not sure if this is necessary though, so I didn't add the parameter on this submission.

Thanks,

Guilherme

Instead of simply logging a message, is there something better we can do to notify a user of an async event result? The event should be masked by the controller until the appropriate log page is read, and I don't think scanning the kernel logs is how such a program wants to find out which page to read.

Yes, I'm not too excited about handling the AERs in kernel space, having a userspace daemon to listen for them would generally be more useful.

The only exception is the namespace changed AER that we already handle. But while we're at it: why do we make use of the changed namespace list log page in this case instead of doing a full recan.

How do daemons typically want to be notified of an event like this? I'm thinking sysfs_notify or maybe a KOBJ_CHANGE uevent. I don't know, this is unfamiliar territory to me.

For IPR (IBM Hardware RAID SCSI controller) we use KOBJ_CHANGE events for a very similar situation, in which the driver signals a userspace daemon, which goes over a sysfs attribute to collect adapter data dumps.

I'm also working on another async notifications mechanism for IPR, similar to what is supported by nvme, in which I plan to use the same uevent interface for userspace notifications.

Gabriel Krisman Bertazi

Thanks very much for all the replies - sorry for the delay in my response.

I guess we have 2 ways of dealing with those events, one being from kernel, the other by working in a daemon like Krisman mentioned (the IPR case). Both approaches seems to have their importance; an automatic error reporting tool might schedule greps on dmesg to find errors, so it can be useful showing them on kernel logs. More yet, some critical errors should be reported on kernel logs always, IMHO (like the adapter persistent error).

On the other hand, daemons seem to be a better accepted solution (as per this thread's comments hehe) and they are more popular maybe. So 2 possible implementations come to my mind, one not excluding the other. I'll elaborate below, but since they can work concomitantly, I'd go for implementing both.

(i) We can implement a daemon like Krisman suggested, that uses uevents and reads the last async event from a sysfs entry, cleaning the necessary bit to allow more events to be reported;

(ii) We can implement a special sysfs parameter that defaults to 0, allowing the use of (i). This parameter is a timeout, in seconds - when this is set to a value >0, approach (i) is disabled, and we start to monitor the events from the driver. Once an event is captured, we print it on kernel log, wait the timeout and clear the necessary bit to allow new events to be reported, and so on...allowing this way a dmesg events approach, without flooding the log or inhibiting the users that prefer (i).

Comments and suggestions are much appreciated.
Thanks in advance,

Guilherme