Skip to contentSkip to conversation

Add Support for FW activation without reset

PATCH Superseded Arnav Dawn 4 participants 14 messages Jul 7, 2017 – Jul 12, 2017 1 message not in the archive NVMe

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Arnav Dawn

author
#1 lore raw

v4 cover Add Support for FW activation without reset

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(+)
Signature · 1 line
1.9.1
author
#2 lore raw

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

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(+)
drivers/nvme/host/core.c +69 -0
index d70df1d..b31c1a7 100644
@@ -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)
drivers/nvme/host/nvme.h +21 -0
index d70ff0f..580a660 100644
@@ -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)
include/linux/nvme.h +10 -0
index 6b8ee9e..5ad58f6 100644
@@ -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 {
Signature · 1 line
1.9.1
maintainer
#3 lore raw

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

Hi Arnav,

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

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2536

Quoted 10 lines
drivers/nvme/host/core.c +10 -0
+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));

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2582

Quoted 9 lines
drivers/nvme/host/core.c +9 -0
+	/* 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.

Quoted 1 line

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

drivers/nvme/host/nvme.h static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl) · line 252

Quoted 2 lines
drivers/nvme/host/nvme.h +2 -0
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CC,
+				&ctrl->ctrl_config))

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

drivers/nvme/host/nvme.h static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl) · line 259

Quoted 2 lines
drivers/nvme/host/nvme.h +2 -0
+	return  ((ctrl->ctrl_config & NVME_CC_ENABLE)
+			&& (csts & NVME_CSTS_PP));

	return ((ctrl->ctrl_config & NVME_CC_ENABLE) && (csts & NVME_CSTS_PP));
#4 lore raw

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

In reply to Arnav Dawn

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2539

Quoted 13 lines · Arnav Dawn
drivers/nvme/host/core.c +13 -0
+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.

Quoted 5 lines

void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,

drivers/nvme/host/core.c +0 -0
 		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");

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2618

Quoted 10 lines
drivers/nvme/host/core.c +8 -0
 		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.

drivers/nvme/host/nvme.h static inline bool nvme_ctrl_ready(struct nvme_ctrl *ctrl) · line 253

Quoted 11 lines
drivers/nvme/host/nvme.h +11 -0
+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.

author
#5 lore raw

v5 cover Add Support for FW activation without reset

In reply to 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(+)
Signature · 1 line
1.9.1
author
#6 lore raw

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

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(+)
drivers/nvme/host/core.c +77 -0
index cb96f4a..b685021 100644
@@ -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)
drivers/nvme/host/nvme.h +2 -0
index 8f2a168..b40b9af 100644
@@ -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;
include/linux/nvme.h +9 -0
index 6b8ee9e..220fa58 100644
@@ -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 {
Signature · 1 line
1.9.1
#7 lore raw

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

Quoted 7 lines · 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>

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

Reviewed-by Keith Busch <keith.busch at intel.com>
maintainer
#8 lore raw

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

In reply to Arnav Dawn

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2527

Quoted 4 lines

}

drivers/nvme/host/core.c +2 -0
+
+static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)

This adds a duplicate empty line.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2539

Quoted 12 lines
drivers/nvme/host/core.c +12 -0
+{
+
+	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.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2545

Quoted 6 lines
drivers/nvme/host/core.c +5 -0
+}
 
+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.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2557

Quoted 1 line
drivers/nvme/host/core.c +1 -0
+	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>
maintainer
#9 lore raw

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

Quoted 39 lines · Christoph Hellwig

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2527

Quoted 4 lines
  }
drivers/nvme/host/core.c +2 -0
+
+static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)

This adds a duplicate empty line.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2539

Quoted 12 lines
drivers/nvme/host/core.c +12 -0
+{
+
+	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.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2545

Quoted 6 lines
drivers/nvme/host/core.c +5 -0
+}
 
+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.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2557

Quoted 1 line
drivers/nvme/host/core.c +1 -0
+	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..

Quoted 3 lines

Otherwise this looks fine to me:

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

Arnav, care to respin with minor cleanups?

author
#10 lore raw

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

Quoted 41 lines · Sagi Grimberg
Quoted 38 lines · Christoph Hellwig
Quoted 3 lines

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

This adds a duplicate empty line.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2539

Quoted 12 lines
drivers/nvme/host/core.c +12 -0
+{
+
+    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.

drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2545

Quoted 6 lines
drivers/nvme/host/core.c +5 -0
+}
 
+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.

Quoted 1 line

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

Quoted 8 lines
Quoted 3 lines

Otherwise this looks fine to me:

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

Arnav, care to respin with minor cleanups?

sure

author
#11 lore raw

v5 cover Add Support for FW activation without reset

In reply to 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(-)
Signature · 1 line
1.9.1
maintainer
#12 lore raw

v5 cover Add Support for FW activation without reset

Here it is...

taking to nvme-4.13-rc

author
#13 lore raw

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

In reply to 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(+)
drivers/nvme/host/core.c +75 -0
index cb96f4a..39fdb57 100644
@@ -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)
drivers/nvme/host/nvme.h +2 -0
index 8f2a168..b40b9af 100644
@@ -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;
include/linux/nvme.h +9 -0
index 6b8ee9e..220fa58 100644
@@ -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 {
Signature · 1 line
1.9.1
author
#14 lore raw

PATCH v5 2/2 nvme: Define NVME_NSID_ALL

In reply to 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(-)
drivers/nvme/host/core.c +3 -3
index 39fdb57..4a07a98 100644
@@ -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)))
include/linux/nvme.h +2 -0
index 220fa58..983975b 100644
@@ -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 */
Signature · 1 line
1.9.1