Skip to contentSkip to conversation

nvme: Send uevent for unhandled AEN completions

PATCH New Keith Busch 3 participants 5 messages Oct 20, 2017 – Oct 30, 2017 NVMe

lore
Share & tools
lore

Keith Busch

author
#0 lore raw

PATCH v2 5/5 nvme: Send uevent for unhandled AEN completions

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send. This implementation saves the last AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.

AEN events that the kernel handles directly will not create a uevent. Such events will stop being sent to the user as new handlers are added to the kernel.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 22 ++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 23 insertions(+)
drivers/nvme/host/core.c +22 -0
index 74724329f430..e5d911dc96f4 100644
@@ -2574,11 +2574,28 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
+static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
+{
+	char *envp[2] = {NULL, NULL};
+	u32 aen = ctrl->aen;
+
+	ctrl->aen = 0;
+	if (!aen)
+		return;
+
+	envp[0] = kasprintf(GFP_KERNEL, "NVME_AEN=%#08x", aen);
+	if (!envp[0])
+		return;
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+	kfree(envp[0]);
+}
+
 static void nvme_async_event_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, async_event_work);
 
+	nvme_aen_uevent(ctrl);
 	ctrl->ops->submit_async_event(ctrl);
 }
 

@@ -2655,6 +2672,10 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	if (le16_to_cpu(status) >> 1 != NVME_SC_SUCCESS)
 		return;
 
+	/*
+	 * Set result for udev only for AEN types that the kernel does not
+	 * directly handle.
+	 */
 	switch (result & 0xff07) {
 	case NVME_AER_NOTICE_NS_CHANGED:
 		dev_info(ctrl->device, "rescanning\n");

@@ -2664,6 +2685,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		queue_work(nvme_wq, &ctrl->fw_act_work);
 		break;
 	default:
+		ctrl->aen = result;
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
 	queue_work(nvme_wq, &ctrl->async_event_work);
drivers/nvme/host/nvme.h +1 -0
index 4cd4e8f4db19..69366293ce57 100644
@@ -166,6 +166,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	u32 aen;
 	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
Signature · 1 line
2.13.6
maintainer
#1 lore raw

PATCH v2 5/5 nvme: Send uevent for unhandled AEN completions

Quoted 4 lines · Keith Busch

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send.

I don't want to encode that in the userspace ABI. With all kinds of new AEN uses coming up we might as well want to go for multiple AERs in the future.

Quoted 7 lines

This implementation saves the last
AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.

AEN events that the kernel handles directly will not create a uevent. Such events will stop being sent to the user as new handlers are added to the kernel.

I think we need an explicit whitelist of the ones we want to expose to userspace. Otherwise we'll get all kinds of complaints later that we'd take something away and break the ABI.

author
#2 lore raw

PATCH v2 5/5 nvme: Send uevent for unhandled AEN completions

Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Keith Busch

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send.

I don't want to encode that in the userspace ABI. With all kinds of new AEN uses coming up we might as well want to go for multiple AERs in the future.

Okay, that's certainly possible to do. The motivation for just one active AEN was just to avoid having a second method for tracking outstanding command tags.

Quoted 11 lines
Quoted 7 lines

This implementation saves the last
AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.

AEN events that the kernel handles directly will not create a uevent. Such events will stop being sent to the user as new handlers are added to the kernel.

I think we need an explicit whitelist of the ones we want to expose to userspace. Otherwise we'll get all kinds of complaints later that we'd take something away and break the ABI.

Oh, I was thinking of this differently. The ABI proposed here is saying all unhandled events are sent to the user, and the kernel has the right to handle these in the future as the need arises; user space can't depend on seeing an event if the kernel should handle it, but it gets to know about all events the kernel didn't handle.

While this changes user visible events when new driver handling is added, this is forward compatible with new events that aren't defined yet.

I'm not particularly concerned about it either way. I just thought it was easier treating the kernel handling as a blacklist to suppress notification rather than have a whitelist.

#3 lore raw

PATCH v2 5/5 nvme: Send uevent for unhandled AEN completions

In reply to Keith Busch

drivers/nvme/host/core.c void nvme_remove_namespaces(struct nvme_ctrl *ctrl) · line 2600

Quoted 8 lines · Keith Busch

static void nvme_async_event_work(struct work_struct *work) {

drivers/nvme/host/core.c +1 -0
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, async_event_work);
 
+	nvme_aen_uevent(ctrl);
 	ctrl->ops->submit_async_event(ctrl);
 }

Is this based on 4.15 or 4.14? I failed to apply it.

author
#4 lore raw

PATCH v2 5/5 nvme: Send uevent for unhandled AEN completions

Quoted 7 lines · Guan Junxiong
Quoted 5 lines · Keith Busch

static void nvme_async_event_work(struct work_struct *work) {

drivers/nvme/host/core.c +0 -0
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, async_event_work);
Quoted 6 lines

drivers/nvme/host/core.c void nvme_remove_namespaces(struct nvme_ctrl *ctrl) · line 2600

Quoted 3 lines
drivers/nvme/host/core.c +1 -0
+	nvme_aen_uevent(ctrl);
 	ctrl->ops->submit_async_event(ctrl);
 }

Is this based on 4.15 or 4.14? I failed to apply it.

This was based off this branch:

http://git.infradead.org/nvme.git/shortlog/refs/heads/nvme-4.15

starting from commit 134aedc9c ("nvme-fc: correct io timeout behavior")