[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[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. 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(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 74724329f430..e5d911dc96f4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4cd4e8f4db19..69366293ce57 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -166,6 +166,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	u32 aen;
 	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
-- 
2.13.6

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[Christoph Hellwig]

On Fri, Oct 20, 2017@04:37:46PM -0600, Keith Busch wrote:
> 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.

> 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.

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[Keith Busch]

On Sat, Oct 21, 2017@10:09:06AM +0200, Christoph Hellwig wrote:
> On Fri, Oct 20, 2017@04:37:46PM -0600, Keith Busch wrote:
> > 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.

> > 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.

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[Guan Junxiong]

On 2017/10/21 6:37, Keith Busch wrote:
>  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);
>  }

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

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[Keith Busch]

On Mon, Oct 30, 2017@06:14:30PM +0800, Guan Junxiong wrote:
> 
> On 2017/10/21 6:37, Keith Busch wrote:
> >  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);
> >  }
> 
> 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")

[PATCHv2 0/5] AEN and userspace updates

[Keith Busch]

This series cleans up some duplicate AEN code, and ultimately adds change
uevents for nvme devices that raise an AEN that the driver did not handle.

I have updated nvme-cli to have a more convenient way to retreive a log
based on the AEN result, and intend to provide a udev rule to wrap that
command when this goes through.

v1 -> v2;

  Merge with nvme-4.15.

  Squashed the AEN define centralization patches.

  Using kasprintf for the uevent.

  Removed async event helper function and made it inline.

  Filter AEN notification only for events the kernel does not handle.
  This would be useful to prevent userspace from reacting events and
  reading logs that may alter what the kernel sees.

Keith Busch (5):
  nvme: Centralize AEN defines
  nvme/fc: remove unused "queue_size" field
  nvme: Single AEN request
  nvme: Unexport starting async event work
  nvme: Send uevent for unhandled AEN completions

 drivers/nvme/host/core.c   | 57 ++++++++++++++++++++--------------------------
 drivers/nvme/host/fc.c     | 47 +++++++++++++-------------------------
 drivers/nvme/host/nvme.h   |  6 ++---
 drivers/nvme/host/pci.c    | 14 ++++--------
 drivers/nvme/host/rdma.c   | 19 ++++------------
 drivers/nvme/target/loop.c | 16 ++++---------
 include/linux/nvme.h       |  2 ++
 7 files changed, 57 insertions(+), 104 deletions(-)

-- 
2.13.6

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[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. 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 | 21 +++++++++++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 74724329f430..d05dcd172ad5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2574,11 +2574,27 @@ 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;
+
+	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 +2671,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 +2684,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);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4cd4e8f4db19..69366293ce57 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -166,6 +166,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	u32 aen;
 	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
-- 
2.13.6

[PATCHv2 5/5] nvme: Send uevent for unhandled AEN completions

[Keith Busch]

On Fri, Oct 20, 2017@04:19:24PM -0600, Keith Busch wrote:
> +static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
> +{
> +	char *envp[2] = {NULL, NULL};
> +	u32 aen = ctrl->aen;
> +
> +	ctrl->aen = 0;
> +	if (!aen)
> +		return;
> +
> +	if (!envp[0])
> +		return;
> +	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
> +	kfree(envp[0]);
> +}

Uh, the string format dissappeared on me. Resending...