[PATCH v4 0/1]Add Support for FW activation without reset

[PATCH v4 0/1]Add Support for FW activation without reset

[Arnav Dawn]

This adds Support for handling FW activation without reset.

changs since V3:
1. Moved timeout calculation to the work.
2. Added a delay of 100ms for scheduling fw_act_work
3. Added a check, that the controller is in NVME_CTRL_LIVE state
   before starting queues once PP is cleared or fw activation times out.

Arnav Dawn (1):
  nvme: Add support for FW activation without reset

 drivers/nvme/host/core.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h | 21 +++++++++++++++
 include/linux/nvme.h     | 10 +++++++
 3 files changed, 100 insertions(+)

-- 
1.9.1

[PATCH v4 1/1] nvme: Add support for FW activation without reset

[Arnav Dawn]

This patch adds support for handling Fw activation without reset
On completion of FW-activation-starting AER, all queues are
paused till CSTS.PP is cleared or timed out (exceeds max time for
fw activtion MTFA). If device fails to clear CSTS.PP within MTFA,
driver issues reset controller

Signed-off-by: Arnav Dawn <a.dawn at samsung.com>
---
 drivers/nvme/host/core.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h | 21 +++++++++++++++
 include/linux/nvme.h     | 10 +++++++
 3 files changed, 100 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d70df1d..b31c1a7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1809,6 +1809,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	nvme_set_queue_limits(ctrl, ctrl->admin_q);
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
+	ctrl->mtfa = le16_to_cpu(id->mtfa);
 
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
@@ -2523,6 +2524,64 @@ static void nvme_async_event_work(struct work_struct *work)
 	spin_unlock_irq(&ctrl->lock);
 }
 
+static int nvme_get_fw_slot_info(struct nvme_ctrl *dev,
+				struct nvme_fw_slot_info_log *log)
+{
+	struct nvme_command c = { };
+
+	c.common.opcode = nvme_admin_get_log_page;
+	c.common.nsid = cpu_to_le32(0xFFFFFFFF);
+	c.common.cdw10[0] = cpu_to_le32(
+		(((sizeof(struct nvme_fw_slot_info_log) / 4) - 1) << 16)
+		| NVME_LOG_FW_SLOT);
+
+	if (!log)
+		return -ENOMEM;
+
+	return nvme_submit_sync_cmd(dev->admin_q, &c, log,
+			sizeof(struct nvme_fw_slot_info_log));
+}
+
+static void nvme_fw_act_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
+				struct nvme_ctrl, fw_act_work);
+	struct nvme_fw_slot_info_log *log;
+	unsigned long fw_act_timeout;
+
+	if (ctrl->mtfa)
+		fw_act_timeout = jiffies +
+				msecs_to_jiffies(ctrl->mtfa * 100);
+	else
+		fw_act_timeout = jiffies +
+				msecs_to_jiffies(admin_timeout * 1000);
+
+	nvme_stop_queues(ctrl);
+	while (nvme_ctrl_pp_status(ctrl)) {
+		if (time_after(jiffies, fw_act_timeout)) {
+			dev_warn(ctrl->device,
+				"Fw activation timeout, reset controller\n");
+			nvme_reset_ctrl(ctrl);
+			break;
+		}
+		msleep(100);
+	}
+
+	if (ctrl->state != NVME_CTRL_LIVE)
+		return;
+
+	nvme_start_queues(ctrl);
+	/* read FW slot informationi to clear the AER*/
+	log = kmalloc(sizeof(struct nvme_fw_slot_info_log), GFP_KERNEL);
+	if (!log)
+		return;
+
+	if (nvme_get_fw_slot_info(ctrl, log))
+		dev_warn(ctrl->device,
+				"Get FW SLOT INFO log error\n");
+	kfree(log);
+}
+
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res)
 {
@@ -2549,6 +2608,14 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		dev_info(ctrl->device, "rescanning\n");
 		nvme_queue_scan(ctrl);
 		break;
+	case NVME_AER_NOTICE_FW_ACT_STARTING:
+		schedule_delayed_work(&ctrl->fw_act_work,
+					msecs_to_jiffies(100));
+		break;
+	case NVME_AER_ERR_FW_IMG_LOAD:
+		dev_warn(ctrl->device, "FW image load error\n");
+		cancel_delayed_work(&ctrl->fw_act_work);
+		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
@@ -2595,6 +2662,7 @@ void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
 	flush_work(&ctrl->async_event_work);
 	flush_work(&ctrl->scan_work);
+	cancel_delayed_work_sync(&ctrl->fw_act_work);
 	nvme_remove_namespaces(ctrl);
 
 	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
@@ -2642,6 +2710,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->quirks = quirks;
 	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
 	INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
+	INIT_DELAYED_WORK(&ctrl->fw_act_work, nvme_fw_act_work);
 
 	ret = nvme_set_instance(ctrl);
 	if (ret)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d70ff0f..580a660 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -142,6 +142,7 @@ struct nvme_ctrl {
 	u16 cntlid;
 
 	u32 ctrl_config;
+	u16 mtfa;
 
 	u32 page_size;
 	u32 max_hw_sectors;
@@ -165,6 +166,7 @@ struct nvme_ctrl {
 	struct work_struct scan_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
+	struct delayed_work fw_act_work;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
@@ -238,6 +240,25 @@ static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl)
 	return val & NVME_CSTS_RDY;
 }
 
+static inline bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
+{
+
+	u32 csts;
+
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
+		return false;
+
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CC,
+				&ctrl->ctrl_config))
+		return false;
+
+	if (csts == ~0)
+		return false;
+
+	return  ((ctrl->ctrl_config & NVME_CC_ENABLE)
+			&& (csts & NVME_CSTS_PP));
+}
+
 static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
 {
 	if (!ctrl->subsystem)
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 6b8ee9e..5ad58f6 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -146,6 +146,7 @@ enum {
 	NVME_CSTS_RDY		= 1 << 0,
 	NVME_CSTS_CFS		= 1 << 1,
 	NVME_CSTS_NSSRO		= 1 << 4,
+	NVME_CSTS_PP		= 1 << 5,
 	NVME_CSTS_SHST_NORMAL	= 0 << 2,
 	NVME_CSTS_SHST_OCCUR	= 1 << 2,
 	NVME_CSTS_SHST_CMPLT	= 2 << 2,
@@ -376,6 +377,13 @@ struct nvme_smart_log {
 	__u8			rsvd216[296];
 };
 
+struct nvme_fw_slot_info_log {
+	__u8			afi;
+	__u8			rsvd1[7];
+	__le64			frs[7];
+	__u8			rsvd64[448];
+};
+
 enum {
 	NVME_SMART_CRIT_SPARE		= 1 << 0,
 	NVME_SMART_CRIT_TEMPERATURE	= 1 << 1,
@@ -386,6 +394,8 @@ enum {
 
 enum {
 	NVME_AER_NOTICE_NS_CHANGED	= 0x0002,
+	NVME_AER_NOTICE_FW_ACT_STARTING = 0x0102,
+	NVME_AER_ERR_FW_IMG_LOAD	= 0x0500,
 };
 
 struct nvme_lba_range_type {
-- 
1.9.1

[PATCH v4 1/1] nvme: Add support for FW activation without reset

[Christoph Hellwig]

Hi Arnav,

this looks mostly good to me, but a few style nitpicks below:

> +static int nvme_get_fw_slot_info(struct nvme_ctrl *dev,
> +				struct nvme_fw_slot_info_log *log)
> +{
> +	struct nvme_command c = { };
> +
> +	c.common.opcode = nvme_admin_get_log_page;
> +	c.common.nsid = cpu_to_le32(0xFFFFFFFF);
> +	c.common.cdw10[0] = cpu_to_le32(
> +		(((sizeof(struct nvme_fw_slot_info_log) / 4) - 1) << 16)
> +		| NVME_LOG_FW_SLOT);

In Linux we always try to place the operations at the end of previous
line, e.g. in this case:

	(((sizeof(struct nvme_fw_slot_info_log) / 4) - 1) << 16) |
		NVME_LOG_FW_SLOT);

Thay being said I think a little helper for this calculation would
be even better, e.g.:

static __le32 nvme_get_log_dw10(u8 lid, size_t len)
{
	return cpu_to_le32(lid | ((size / 4) - 1) << 16);
}

...

	c.common.cdw10[0] = nvme_get_log_dw10(NVME_LOG_FW_SLOT, sizeof(*log));


> +	/* read FW slot informationi to clear the AER*/
> +	log = kmalloc(sizeof(struct nvme_fw_slot_info_log), GFP_KERNEL);
> +	if (!log)
> +		return;
> +
> +	if (nvme_get_fw_slot_info(ctrl, log))
> +		dev_warn(ctrl->device,
> +				"Get FW SLOT INFO log error\n");
> +	kfree(log);

Please move the allocation and freeing of log into nvme_get_fw_slot_info.
Maybe the warning as well.

> +static inline bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)

I don't think this should be inline, as it's not performane critical,
please move it to core.c.

> +	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CC,
> +				&ctrl->ctrl_config))

	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CC, &ctrl->ctrl_config))

> +	return  ((ctrl->ctrl_config & NVME_CC_ENABLE)
> +			&& (csts & NVME_CSTS_PP));

	return ((ctrl->ctrl_config & NVME_CC_ENABLE) && (csts & NVME_CSTS_PP));

[PATCH v4 1/1] nvme: Add support for FW activation without reset

[Keith Busch]

On Fri, Jul 07, 2017@05:12:47PM +0530, Arnav Dawn wrote:
> +static int nvme_get_fw_slot_info(struct nvme_ctrl *dev,
> +				struct nvme_fw_slot_info_log *log)
> +{
> +	struct nvme_command c = { };
> +
> +	c.common.opcode = nvme_admin_get_log_page;
> +	c.common.nsid = cpu_to_le32(0xFFFFFFFF);
> +	c.common.cdw10[0] = cpu_to_le32(
> +		(((sizeof(struct nvme_fw_slot_info_log) / 4) - 1) << 16)
> +		| NVME_LOG_FW_SLOT);
> +
> +	if (!log)
> +		return -ENOMEM;

You check if log is not NULL here, but you already checked it before
calling the function.

I think you should just move the log allocation to the above function
instead of taking it as a parameter, anyway.

>  void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
>  		union nvme_result *res)
>  {
> @@ -2549,6 +2608,14 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
>  		dev_info(ctrl->device, "rescanning\n");
>  		nvme_queue_scan(ctrl);
>  		break;
> +	case NVME_AER_NOTICE_FW_ACT_STARTING:
> +		schedule_delayed_work(&ctrl->fw_act_work,
> +					msecs_to_jiffies(100));
> +		break;
> +	case NVME_AER_ERR_FW_IMG_LOAD:
> +		dev_warn(ctrl->device, "FW image load error\n");
> +		cancel_delayed_work(&ctrl->fw_act_work);
> +		break;

I'm still not a fan of the delayed work. Just use normal work, and remove
the NVME_AER_ERR_FW_IMG_LOAD case.

> +static inline bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
> +{
> +
> +	u32 csts;
> +
> +	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
> +		return false;
> +
> +	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CC,
> +				&ctrl->ctrl_config))
> +		return false;

The above looks potentially racey. The ctrl->ctrl_config is the value
the driver wants to program CC to, not necessarily the current value. If
you're reading it back, that could race with a controller reset and
we'll get the wrong configuration.

Instead of reading the register, I think you can just rely on the value
of ctrl->ctrl_config to be accurate.

[PATCH v5 0/1] Add Support for FW activation without reset

[Arnav Dawn]

This adds Support for handling FW activation without reset.

Changes since V4:
1> Removed reading of CC register from devicei to ctrl_config. 
2> Moved Nvme_get_pp_statust to core.c
3> Moved buffer alloc and dealloc to nvme_get_fw_slot_info 
   added a helper function to calculate dw10 for get fw slot info.
4> Changed fw_act_work type from delayed worked to work.
5> Removed AER completion case for Image Load Error.

Arnav Dawn (1):
  nvme: Add support for FW activation without reset

 drivers/nvme/host/core.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 include/linux/nvme.h     |  9 ++++++
 3 files changed, 89 insertions(+)

-- 
1.9.1

[PATCH v5 1/1] nvme: Add support for FW activation without reset

[Arnav Dawn]

This patch adds support for handling Fw activation without reset
On completion of FW-activation-starting AER, all queues are
paused till CSTS.PP is cleared or timed out (exceeds max time for
fw activtion MTFA). If device fails to clear CSTS.PP within MTFA,
driver issues reset controller

Signed-off-by: Arnav Dawn <a.dawn at samsung.com>
---
 drivers/nvme/host/core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 include/linux/nvme.h     |  9 ++++++
 3 files changed, 88 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cb96f4a..b685021 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2523,6 +2523,78 @@ static void nvme_async_event_work(struct work_struct *work)
 	spin_unlock_irq(&ctrl->lock);
 }
 
+
+static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
+{
+
+	u32 csts;
+
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
+		return false;
+
+	if (csts == ~0)
+		return false;
+
+	return ((ctrl->ctrl_config & NVME_CC_ENABLE)
+			&& (csts & NVME_CSTS_PP));
+}
+
+static __le32 nvme_get_log_dw10(u8 lid, int size)
+{
+	return cpu_to_le32((((size / 4) - 1) << 16) | lid);
+}
+
+static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
+{
+	struct nvme_command c = { };
+	struct nvme_fw_slot_info_log *log;
+
+	log = kmalloc(sizeof(*log), GFP_KERNEL);
+	if (!log)
+		return;
+
+	c.common.opcode = nvme_admin_get_log_page;
+	c.common.nsid = cpu_to_le32(0xFFFFFFFF);
+	c.common.cdw10[0] = nvme_get_log_dw10(NVME_LOG_FW_SLOT, sizeof(*log));
+
+	if (!nvme_submit_sync_cmd(ctrl->admin_q, &c, log, sizeof(*log)))
+		dev_warn(ctrl->device,
+				"Get FW SLOT INFO log error\n");
+	kfree(log);
+}
+
+static void nvme_fw_act_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(work,
+				struct nvme_ctrl, fw_act_work);
+	unsigned long fw_act_timeout;
+
+	if (ctrl->mtfa)
+		fw_act_timeout = jiffies +
+				msecs_to_jiffies(ctrl->mtfa * 100);
+	else
+		fw_act_timeout = jiffies +
+				msecs_to_jiffies(admin_timeout * 1000);
+
+	nvme_stop_queues(ctrl);
+	while (nvme_ctrl_pp_status(ctrl)) {
+		if (time_after(jiffies, fw_act_timeout)) {
+			dev_warn(ctrl->device,
+				"Fw activation timeout, reset controller\n");
+			nvme_reset_ctrl(ctrl);
+			break;
+		}
+		msleep(100);
+	}
+
+	if (ctrl->state != NVME_CTRL_LIVE)
+		return;
+
+	nvme_start_queues(ctrl);
+	/* read FW slot informationi to clear the AER*/
+	nvme_get_fw_slot_info(ctrl);
+}
+
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res)
 {
@@ -2549,6 +2621,9 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		dev_info(ctrl->device, "rescanning\n");
 		nvme_queue_scan(ctrl);
 		break;
+	case NVME_AER_NOTICE_FW_ACT_STARTING:
+		schedule_work(&ctrl->fw_act_work);
+		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
@@ -2596,6 +2671,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
 	flush_work(&ctrl->scan_work);
+	cancel_work_sync(&ctrl->fw_act_work);
 }
 EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
 
@@ -2659,6 +2735,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->quirks = quirks;
 	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
 	INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
+	INIT_WORK(&ctrl->fw_act_work, nvme_fw_act_work);
 
 	ret = nvme_set_instance(ctrl);
 	if (ret)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8f2a168..b40b9af 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -142,6 +142,7 @@ struct nvme_ctrl {
 	u16 cntlid;
 
 	u32 ctrl_config;
+	u16 mtfa;
 	u32 queue_count;
 
 	u64 cap;
@@ -167,6 +168,7 @@ struct nvme_ctrl {
 	struct work_struct scan_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
+	struct work_struct fw_act_work;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 6b8ee9e..220fa58 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -146,6 +146,7 @@ enum {
 	NVME_CSTS_RDY		= 1 << 0,
 	NVME_CSTS_CFS		= 1 << 1,
 	NVME_CSTS_NSSRO		= 1 << 4,
+	NVME_CSTS_PP		= 1 << 5,
 	NVME_CSTS_SHST_NORMAL	= 0 << 2,
 	NVME_CSTS_SHST_OCCUR	= 1 << 2,
 	NVME_CSTS_SHST_CMPLT	= 2 << 2,
@@ -376,6 +377,13 @@ struct nvme_smart_log {
 	__u8			rsvd216[296];
 };
 
+struct nvme_fw_slot_info_log {
+	__u8			afi;
+	__u8			rsvd1[7];
+	__le64			frs[7];
+	__u8			rsvd64[448];
+};
+
 enum {
 	NVME_SMART_CRIT_SPARE		= 1 << 0,
 	NVME_SMART_CRIT_TEMPERATURE	= 1 << 1,
@@ -386,6 +394,7 @@ enum {
 
 enum {
 	NVME_AER_NOTICE_NS_CHANGED	= 0x0002,
+	NVME_AER_NOTICE_FW_ACT_STARTING = 0x0102,
 };
 
 struct nvme_lba_range_type {
-- 
1.9.1

[PATCH v5 1/1] nvme: Add support for FW activation without reset

[Keith Busch]

On Tue, Jul 11, 2017@07:18:32PM +0530, Arnav Dawn wrote:
> This patch adds support for handling Fw activation without reset
> On completion of FW-activation-starting AER, all queues are
> paused till CSTS.PP is cleared or timed out (exceeds max time for
> fw activtion MTFA). If device fails to clear CSTS.PP within MTFA,
> driver issues reset controller
> 
> Signed-off-by: Arnav Dawn <a.dawn at samsung.com>

This looks okay to me, thanks for doing this feature.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH v5 1/1] nvme: Add support for FW activation without reset

[Christoph Hellwig]

>  }
>  
> +
> +static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)

This adds a duplicate empty line.

> +{
> +
> +	u32 csts;
> +
> +	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
> +		return false;
> +
> +	if (csts == ~0)
> +		return false;
> +
> +	return ((ctrl->ctrl_config & NVME_CC_ENABLE)
> +			&& (csts & NVME_CSTS_PP));

These should be on one line, and if if they weren't the "&&" placement
is wrong.

> +}

> +static __le32 nvme_get_log_dw10(u8 lid, int size)
> +{
> +	return cpu_to_le32((((size / 4) - 1) << 16) | lid);
> +}

Can you move this up the file a bit so that it's not hidden inside
the FW activation bits.  Also size should be of type size_t.

> +	c.common.nsid = cpu_to_le32(0xFFFFFFFF);

Except for one weird spot we use lower-cases hex numbers.  But in
the longer run we really should have an NVME_NSID_ALL define for this
constant..

Otherwise this looks fine to me:

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH v5 1/1] nvme: Add support for FW activation without reset

[Sagi Grimberg]

On 12/07/17 10:34, Christoph Hellwig wrote:
>>   }
>>   
>> +
>> +static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
> 
> This adds a duplicate empty line.
> 
>> +{
>> +
>> +	u32 csts;
>> +
>> +	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
>> +		return false;
>> +
>> +	if (csts == ~0)
>> +		return false;
>> +
>> +	return ((ctrl->ctrl_config & NVME_CC_ENABLE)
>> +			&& (csts & NVME_CSTS_PP));
> 
> These should be on one line, and if if they weren't the "&&" placement
> is wrong.
> 
>> +}
> 
>> +static __le32 nvme_get_log_dw10(u8 lid, int size)
>> +{
>> +	return cpu_to_le32((((size / 4) - 1) << 16) | lid);
>> +}
> 
> Can you move this up the file a bit so that it's not hidden inside
> the FW activation bits.  Also size should be of type size_t.
> 
>> +	c.common.nsid = cpu_to_le32(0xFFFFFFFF);
> 
> Except for one weird spot we use lower-cases hex numbers.  But in
> the longer run we really should have an NVME_NSID_ALL define for this
> constant..
> 
> Otherwise this looks fine to me:
> 
> Reviewed-by: Christoph Hellwig <hch at lst.de>

Arnav, care to respin with minor cleanups?

[PATCH v5 0/2] Add Support for FW activation without reset

[Arnav Dawn]

This adds Support for handling FW activation without reset.

Changes since last version:
1> minor clean up
2> define  NVME_NSID_ALL for nsid "0xffffffff"

Arnav Dawn (2):
  nvme: Add support for FW activation without reset
  nvme: Define NVME_NSID_ALL

 drivers/nvme/host/core.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  2 ++
 include/linux/nvme.h     | 11 +++++++
 3 files changed, 90 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH v5 1/2] nvme: Add support for FW activation without reset

[Arnav Dawn]

This patch adds support for handling Fw activation without reset
On completion of FW-activation-starting AER, all queues are
paused till CSTS.PP is cleared or timed out (exceeds max time for
fw activtion MTFA). If device fails to clear CSTS.PP within MTFA,
driver issues reset controller.

Signed-off-by: Arnav Dawn <a.dawn at samsung.com>
Reviewed-by: Keith Busch <keith.busch at intel.com>
Reviewed-by: Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  2 ++
 include/linux/nvme.h     |  9 ++++++
 3 files changed, 86 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cb96f4a..39fdb57 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -76,6 +76,11 @@
 
 static struct class *nvme_class;
 
+static __le32 nvme_get_log_dw10(u8 lid, size_t size)
+{
+	return cpu_to_le32((((size / 4) - 1) << 16) | lid);
+}
+
 int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
 {
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
@@ -2523,6 +2528,71 @@ static void nvme_async_event_work(struct work_struct *work)
 	spin_unlock_irq(&ctrl->lock);
 }
 
+static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
+{
+
+	u32 csts;
+
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
+		return false;
+
+	if (csts == ~0)
+		return false;
+
+	return ((ctrl->ctrl_config & NVME_CC_ENABLE) && (csts & NVME_CSTS_PP));
+}
+
+static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
+{
+	struct nvme_command c = { };
+	struct nvme_fw_slot_info_log *log;
+
+	log = kmalloc(sizeof(*log), GFP_KERNEL);
+	if (!log)
+		return;
+
+	c.common.opcode = nvme_admin_get_log_page;
+	c.common.nsid = cpu_to_le32(0xffffffff);
+	c.common.cdw10[0] = nvme_get_log_dw10(NVME_LOG_FW_SLOT, sizeof(*log));
+
+	if (!nvme_submit_sync_cmd(ctrl->admin_q, &c, log, sizeof(*log)))
+		dev_warn(ctrl->device,
+				"Get FW SLOT INFO log error\n");
+	kfree(log);
+}
+
+static void nvme_fw_act_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(work,
+				struct nvme_ctrl, fw_act_work);
+	unsigned long fw_act_timeout;
+
+	if (ctrl->mtfa)
+		fw_act_timeout = jiffies +
+				msecs_to_jiffies(ctrl->mtfa * 100);
+	else
+		fw_act_timeout = jiffies +
+				msecs_to_jiffies(admin_timeout * 1000);
+
+	nvme_stop_queues(ctrl);
+	while (nvme_ctrl_pp_status(ctrl)) {
+		if (time_after(jiffies, fw_act_timeout)) {
+			dev_warn(ctrl->device,
+				"Fw activation timeout, reset controller\n");
+			nvme_reset_ctrl(ctrl);
+			break;
+		}
+		msleep(100);
+	}
+
+	if (ctrl->state != NVME_CTRL_LIVE)
+		return;
+
+	nvme_start_queues(ctrl);
+	/* read FW slot informationi to clear the AER*/
+	nvme_get_fw_slot_info(ctrl);
+}
+
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res)
 {
@@ -2549,6 +2619,9 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		dev_info(ctrl->device, "rescanning\n");
 		nvme_queue_scan(ctrl);
 		break;
+	case NVME_AER_NOTICE_FW_ACT_STARTING:
+		schedule_work(&ctrl->fw_act_work);
+		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
@@ -2596,6 +2669,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
 	flush_work(&ctrl->scan_work);
+	cancel_work_sync(&ctrl->fw_act_work);
 }
 EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
 
@@ -2659,6 +2733,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->quirks = quirks;
 	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
 	INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
+	INIT_WORK(&ctrl->fw_act_work, nvme_fw_act_work);
 
 	ret = nvme_set_instance(ctrl);
 	if (ret)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8f2a168..b40b9af 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -142,6 +142,7 @@ struct nvme_ctrl {
 	u16 cntlid;
 
 	u32 ctrl_config;
+	u16 mtfa;
 	u32 queue_count;
 
 	u64 cap;
@@ -167,6 +168,7 @@ struct nvme_ctrl {
 	struct work_struct scan_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
+	struct work_struct fw_act_work;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 6b8ee9e..220fa58 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -146,6 +146,7 @@ enum {
 	NVME_CSTS_RDY		= 1 << 0,
 	NVME_CSTS_CFS		= 1 << 1,
 	NVME_CSTS_NSSRO		= 1 << 4,
+	NVME_CSTS_PP		= 1 << 5,
 	NVME_CSTS_SHST_NORMAL	= 0 << 2,
 	NVME_CSTS_SHST_OCCUR	= 1 << 2,
 	NVME_CSTS_SHST_CMPLT	= 2 << 2,
@@ -376,6 +377,13 @@ struct nvme_smart_log {
 	__u8			rsvd216[296];
 };
 
+struct nvme_fw_slot_info_log {
+	__u8			afi;
+	__u8			rsvd1[7];
+	__le64			frs[7];
+	__u8			rsvd64[448];
+};
+
 enum {
 	NVME_SMART_CRIT_SPARE		= 1 << 0,
 	NVME_SMART_CRIT_TEMPERATURE	= 1 << 1,
@@ -386,6 +394,7 @@ enum {
 
 enum {
 	NVME_AER_NOTICE_NS_CHANGED	= 0x0002,
+	NVME_AER_NOTICE_FW_ACT_STARTING = 0x0102,
 };
 
 struct nvme_lba_range_type {
-- 
1.9.1

[PATCH v5 2/2] nvme: Define NVME_NSID_ALL

[Arnav Dawn]

Define the constant "0xffffffff" (used as nsid for all namespaces)
as NVME_NSID_ALL.

Signed-off-by: Arnav Dawn <a.dawn at samsung.com>
---
 drivers/nvme/host/core.c | 6 +++---
 include/linux/nvme.h     | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 39fdb57..4a07a98 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -312,7 +312,7 @@ static int nvme_toggle_streams(struct nvme_ctrl *ctrl, bool enable)
 	memset(&c, 0, sizeof(c));
 
 	c.directive.opcode = nvme_admin_directive_send;
-	c.directive.nsid = cpu_to_le32(0xffffffff);
+	c.directive.nsid = cpu_to_le32(NVME_NSID_ALL);
 	c.directive.doper = NVME_DIR_SND_ID_OP_ENABLE;
 	c.directive.dtype = NVME_DIR_IDENTIFY;
 	c.directive.tdtype = NVME_DIR_STREAMS;
@@ -362,7 +362,7 @@ static int nvme_configure_directives(struct nvme_ctrl *ctrl)
 	if (ret)
 		return ret;
 
-	ret = nvme_get_stream_params(ctrl, &s, 0xffffffff);
+	ret = nvme_get_stream_params(ctrl, &s, NVME_NSID_ALL);
 	if (ret)
 		return ret;
 
@@ -2552,7 +2552,7 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
 		return;
 
 	c.common.opcode = nvme_admin_get_log_page;
-	c.common.nsid = cpu_to_le32(0xffffffff);
+	c.common.nsid = cpu_to_le32(NVME_NSID_ALL);
 	c.common.cdw10[0] = nvme_get_log_dw10(NVME_LOG_FW_SLOT, sizeof(*log));
 
 	if (!nvme_submit_sync_cmd(ctrl->admin_q, &c, log, sizeof(*log)))
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 220fa58..983975b 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -32,6 +32,8 @@
 
 #define NVME_RDMA_IP_PORT	4420
 
+#define NVME_NSID_ALL		0xffffffff
+
 enum nvme_subsys_type {
 	NVME_NQN_DISC	= 1,		/* Discovery type target subsystem */
 	NVME_NQN_NVME	= 2,		/* NVME type target subsystem */
-- 
1.9.1

[PATCH v5 0/2] Add Support for FW activation without reset

[Sagi Grimberg]

Here it is...

taking to nvme-4.13-rc

[PATCH v5 1/1] nvme: Add support for FW activation without reset

[Arnav Dawn]

On Wednesday 12 July 2017 04:02 PM, Sagi Grimberg wrote:
>
>
> On 12/07/17 10:34, Christoph Hellwig wrote:
>>>   }
>>>   +
>>> +static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
>>
>> This adds a duplicate empty line.
>>
>>> +{
>>> +
>>> +    u32 csts;
>>> +
>>> +    if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
>>> +        return false;
>>> +
>>> +    if (csts == ~0)
>>> +        return false;
>>> +
>>> +    return ((ctrl->ctrl_config & NVME_CC_ENABLE)
>>> +            && (csts & NVME_CSTS_PP));
>>
>> These should be on one line, and if if they weren't the "&&" placement
>> is wrong.
>>
>>> +}
>>
>>> +static __le32 nvme_get_log_dw10(u8 lid, int size)
>>> +{
>>> +    return cpu_to_le32((((size / 4) - 1) << 16) | lid);
>>> +}
>>
>> Can you move this up the file a bit so that it's not hidden inside
>> the FW activation bits.  Also size should be of type size_t.
>>
>>> +    c.common.nsid = cpu_to_le32(0xFFFFFFFF);
>>
>> Except for one weird spot we use lower-cases hex numbers.  But in
>> the longer run we really should have an NVME_NSID_ALL define for this
>> constant..
>>
>> Otherwise this looks fine to me:
>>
>> Reviewed-by: Christoph Hellwig <hch at lst.de>
>
> Arnav, care to respin with minor cleanups?
>
>
>
sure