nvme: Support for FW activation without reset
A message that is not in the archive (sent off-list or not yet received): CGME20170529141634epcas5p3913fde49b40fcce3cc15971688820834@epcas5p3.samsung.com
v2 cover nvme: Support for FW activation without reset
This series of two patches adds support for FW activation without reset.
Arnav Dawn (2):
nvme: Adding support for FW Slot info log
nvme: Add support for FW activation without reset
drivers/nvme/host/core.c | 109 ++++++++++++++++++++++++++++++++++++++++------- drivers/nvme/host/nvme.h | 5 ++- drivers/nvme/host/scsi.c | 26 ++++++++--- include/linux/nvme.h | 10 +++++ 4 files changed, 127 insertions(+), 23 deletions(-)
Signature · 1 line
1.9.1
PATCH v2 1/2 nvme: Adding support for FW Slot info log
This adds support for FW slot info log to nvme_get_log_page. The nvme_get_log_page takes log type as parameter. This will also be used to clear FW slot info log for Fw activation AER.
--- drivers/nvme/host/core.c | 38 +++++++++++++++++++++++--------------- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/host/scsi.c | 26 ++++++++++++++++++++------ include/linux/nvme.h | 7 +++++++ 4 files changed, 51 insertions(+), 22 deletions(-)
drivers/nvme/host/core.c +23 -15
@@ -713,26 +713,34 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,return ret; } -int nvme_get_log_page (struct nvme_ctrl *dev,struct nvme_smart_log **log) +int nvme_get_log_page (struct nvme_ctrl *dev,int log_type,void *log) {struct nvme_command c = { }; -int error; +int log_size; - 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_smart_log ) /4 ) -1 ) <<16 ) | -NVME_LOG_SMART ), +if (!log) +return -EINVAL ; - *log =kmalloc (sizeof (struct nvme_smart_log ),GFP_KERNEL ); -if (!*log) -return -ENOMEM ; + c.common.opcode = nvme_admin_get_log_page; - error =nvme_submit_sync_cmd (dev->admin_q, &c, *log, -sizeof (struct nvme_smart_log )); -if (error) -kfree (*log); -return error; +switch (log_type) { +case NVME_LOG_SMART : + log_size =sizeof (struct nvme_smart_log ); + c.common.nsid =cpu_to_le32 (0xFFFFFFFF ); + c.common.cdw10[0 ] =cpu_to_le32 ( + (((log_size /4 ) -1 ) <<16 ) |NVME_LOG_SMART ); +break ; +case NVME_LOG_FW_SLOT : + log_size =sizeof (struct nvme_fw_slot_info_log ); + c.common.nsid =cpu_to_le32 (0xFFFFFFFF ); + c.common.cdw10[0 ] =cpu_to_le32 ( + (((log_size /4 ) -1 ) <<16 ) |NVME_LOG_FW_SLOT ); +break ; +default : +return -EINVAL ; + } + +return nvme_submit_sync_cmd (dev->admin_q, &c, log, log_size); }int nvme_set_queue_count (struct nvme_ctrl *ctrl,int *count)
drivers/nvme/host/nvme.h +1 -1
@@ -313,7 +313,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,int nvme_identify_ctrl (struct nvme_ctrl *dev,struct nvme_id_ctrl **id);int nvme_identify_ns (struct nvme_ctrl *dev,unsigned nsid,struct nvme_id_ns **id); -int nvme_get_log_page (struct nvme_ctrl *dev,struct nvme_smart_log **log); +int nvme_get_log_page (struct nvme_ctrl *dev,int log_type,void *log);int nvme_get_features (struct nvme_ctrl *dev,unsigned fid,unsigned nsid,void *buffer,size_t buflen,u32 *result); int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
drivers/nvme/host/scsi.c +20 -6
@@ -846,9 +846,15 @@ static int nvme_trans_log_info_exceptions(struct nvme_ns *ns,if (log_response ==NULL )return -ENOMEM ; - res =nvme_get_log_page (ns->ctrl, &smart_log); -if (res <0 ) + smart_log =kmalloc (sizeof (struct nvme_smart_log ),GFP_KERNEL ); +if (smart_log ==NULL ) { + res = -ENOMEM ; goto out_free_response; + } + + res =nvme_get_log_page (ns->ctrl,NVME_LOG_SMART , smart_log); +if (res <0 ) + goto out_free_log;if (res !=NVME_SC_SUCCESS ) { temp_c =LOG_TEMP_UNKNOWN ;
@@ -857,7 +863,6 @@ static int nvme_trans_log_info_exceptions(struct nvme_ns *ns, (smart_log->temperature[0 ]); temp_c = temp_k -KELVIN_TEMP_FACTOR ; } -kfree (smart_log); log_response[0 ] =LOG_PAGE_INFORMATIONAL_EXCEPTIONS_PAGE ;/* Subpage=0x00, Page Length MSB=0 */
@@ -873,6 +878,8 @@ static int nvme_trans_log_info_exceptions(struct nvme_ns *ns, xfer_len =min (alloc_len,LOG_INFO_EXCP_PAGE_LENGTH ); res =nvme_trans_copy_to_user (hdr, log_response, xfer_len); + out_free_log: +kfree (smart_log); out_free_response:kfree (log_response);return res;
@@ -893,9 +900,15 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,if (log_response ==NULL )return -ENOMEM ; - res =nvme_get_log_page (ns->ctrl, &smart_log); -if (res <0 ) + smart_log =kmalloc (sizeof (struct nvme_smart_log ),GFP_KERNEL ); +if (smart_log ==NULL ) { + res = -ENOMEM ; goto out_free_response; + } + + res =nvme_get_log_page (ns->ctrl,NVME_LOG_SMART , smart_log); +if (res <0 ) + goto out_free_log;if (res !=NVME_SC_SUCCESS ) { temp_c_cur =LOG_TEMP_UNKNOWN ;
@@ -904,7 +917,6 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr, (smart_log->temperature[0 ]); temp_c_cur = temp_k -KELVIN_TEMP_FACTOR ; } -kfree (smart_log);/* Get Features for Temp Threshold */ res = nvme_get_features(ns->ctrl, NVME_FEAT_TEMP_THRESH, 0, NULL, 0,
@@ -933,6 +945,8 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr, xfer_len =min (alloc_len,LOG_TEMP_PAGE_LENGTH ); res =nvme_trans_copy_to_user (hdr, log_response, xfer_len); + out_free_log: +kfree (smart_log); out_free_response:kfree (log_response);return res;
include/linux/nvme.h +7 -0
@@ -337,6 +337,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 ,
Signature · 1 line
1.9.1
Christoph Hellwig
maintainerPATCH v2 1/2 nvme: Adding support for FW Slot info log
Quoted 18 lines · Arnav Dawn
drivers/nvme/host/core.c +13
-0+int nvme_get_log_page (struct nvme_ctrl *dev,int log_type,void *log) {struct nvme_command c = { }; +int log_size; +if (!log) +return -EINVAL ; + c.common.opcode = nvme_admin_get_log_page; +switch (log_type) { +case NVME_LOG_SMART : + log_size =sizeof (struct nvme_smart_log ); + c.common.nsid =cpu_to_le32 (0xFFFFFFFF ); + c.common.cdw10[0 ] =cpu_to_le32 ( + (((log_size /4 ) -1 ) <<16 ) |NVME_LOG_SMART ); +break ; +case NVME_LOG_FW_SLOT :
drivers/nvme/host/core.c int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11, · line 743
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+ log_size =sizeof (struct nvme_fw_slot_info_log ); + c.common.nsid =cpu_to_le32 (0xFFFFFFFF ); + c.common.cdw10[0 ] =cpu_to_le32 ( + (((log_size /4 ) -1 ) <<16 ) |NVME_LOG_FW_SLOT ); +break ; +default : +return -EINVAL ; + } + +return nvme_submit_sync_cmd (dev->admin_q, &c, log, log_size);
This functioin has basically no common code. I think the current nvme_get_log_page should be renamed to nvme_get_smart_log and moved to scsi.c, and you'll just add a new helper for your log page or even just open code it in the caller. A little inline helper for setting up dword10 would be nice, though.
Sagi Grimberg
maintainerPATCH v2 1/2 nvme: Adding support for FW Slot info log
Quoted 6 lines
This functioin has basically no common code. I think the current
nvme_get_log_pageshould be renamed tonvme_get_smart_logand moved toscsi.c, and you'll just add a new helper for your log page or even just open code it in the caller. A little inline helper for setting up dword10 would be nice, though.
Agree, that'd be cleaner.
PATCH v2 1/2 nvme: Adding support for FW Slot info log
In reply to Christoph HellwigQuoted 7 lines
This functioin has basically no common code. I think the current
nvme_get_log_pageshould be renamed tonvme_get_smart_logand moved toscsi.c, and you'll just add a new helper for your log page or even just open code it in the caller. A little inline helper for setting up dword10 would be nice, though.
Agree, i'll change it in next version.
PATCH v2 2/2 nvme: Add support for FW activation without reset
In reply to Arnav DawnThis 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.
--- drivers/nvme/host/core.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++- drivers/nvme/host/nvme.h | 3 ++ include/linux/nvme.h | 3 ++ 3 files changed, 76 insertions(+), 1 deletion(-)
drivers/nvme/host/core.c +70 -1
@@ -1610,7 +1610,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; prev_apsta = ctrl->apsta;if (ctrl->quirks &NVME_QUIRK_NO_APST ) {
@@ -2279,6 +2279,45 @@ static void nvme_async_event_work(struct work_struct *work)spin_unlock_irq (&ctrl->lock); } +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; +u32 csts; + +nvme_stop_queues (ctrl); +while (1 ) { +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +break ; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +break ; + +if (csts == ~0 || ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && !(csts &NVME_CSTS_PP ))) +break ; + +if (time_after (jiffies, ctrl->fw_act_timeout)) { +dev_warn (ctrl->device, +"Fw activation timeout, reset controller\n" ); + ctrl->ops->reset_ctrl (ctrl); +break ; + } +msleep (100 ); + } +nvme_start_queues (ctrl); + ctrl->fw_act_timeout =0 ; + log =kmalloc (sizeof (struct nvme_fw_slot_info_log ),GFP_KERNEL ); +if (!log) +return ; +if (nvme_get_log_page (ctrl,NVME_LOG_FW_SLOT , 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) {
@@ -2305,6 +2344,34 @@ 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 : + { +u32 csts; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +return ; +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +return ; + +if ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && (csts &NVME_CSTS_PP )) { +if (ctrl->mtfa) + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (ctrl->mtfa *100 ); +else + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (admin_timeout *1000 ); + +schedule_delayed_work (&ctrl->fw_act_work,0 ); + } +break ; + } +case NVME_AER_ERR_FW_IMG_LOAD : +dev_warn (ctrl->device,"FW image load error\n" ); +cancel_delayed_work (&ctrl->fw_act_work); + ctrl->fw_act_timeout =0 ; +break ;default :dev_warn (ctrl->device,"async event result %08x\n" , result); }
@@ -2351,6 +2418,7 @@ void nvme_uninit_ctrl(struct nvme_ctrl *ctrl) {flush_work (&ctrl->async_event_work);flush_work (&ctrl->scan_work); +cancel_delayed_work (&ctrl->fw_act_work);nvme_remove_namespaces (ctrl);device_destroy (nvme_class,MKDEV (nvme_char_major, ctrl->instance));
@@ -2398,6 +2466,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 +3 -0
@@ -141,6 +141,8 @@ struct nvme_ctrl {u16 cntlid;u32 ctrl_config; +u16 mtfa; +unsigned long fw_act_timeout;u32 page_size;u32 max_hw_sectors;
@@ -162,6 +164,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;
include/linux/nvme.h +3 -0
@@ -144,6 +144,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,
@@ -354,6 +355,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
Christoph Hellwig
maintainerPATCH v2 2/2 nvme: Add support for FW activation without reset
Quoted 39 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>--- drivers/nvme/host/core.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++- drivers/nvme/host/nvme.h | 3 ++ include/linux/nvme.h | 3 ++ 3 files changed, 76 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +10
-1@@ -1610,7 +1610,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; prev_apsta = ctrl->apsta;if (ctrl->quirks &NVME_QUIRK_NO_APST ) {@@ -2279,6 +2279,45 @@ static void nvme_async_event_work(struct work_struct *work)spin_unlock_irq (&ctrl->lock); } +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; +u32 csts; + +nvme_stop_queues (ctrl); +while (1 ) {
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2300
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +break ; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +break ; + +if (csts == ~0 || ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && !(csts &NVME_CSTS_PP ))) +break ;
Can you factor these checks into a helper that your loop checks in the while statement?
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2318
Quoted 9 lines
drivers/nvme/host/core.c +9
-0+nvme_start_queues (ctrl); + ctrl->fw_act_timeout =0 ; + log =kmalloc (sizeof (struct nvme_fw_slot_info_log ),GFP_KERNEL ); +if (!log) +return ; +if (nvme_get_log_page (ctrl,NVME_LOG_FW_SLOT , log)) +dev_warn (ctrl->device, +"Get FW SLOT INFO log error\n" ); +kfree (log);
So we don't do anything with the log. It might be worth having a comment like:
/* read the firmware activation log to clear the AER */
for readers of the code that don't know the NVMe spec inside out.
Quoted 12 lines
drivers/nvme/host/core.c +12
-0+case NVME_AER_NOTICE_FW_ACT_STARTING : + { +u32 csts; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +return ; +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +return ; + +if ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && (csts &NVME_CSTS_PP )) {
drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2368
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+if (ctrl->mtfa) + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (ctrl->mtfa *100 ); +else + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (admin_timeout *1000 ); + +schedule_delayed_work (&ctrl->fw_act_work,0 ); + } +break ;
Please split this code into a helper.
PATCH v2 2/2 nvme: Add support for FW activation without reset
Quoted 7 lines
Quoted 7 lines
drivers/nvme/host/core.c +7
-0+nvme_stop_queues (ctrl); +while (1 ) { +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +break ; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config))
Quoted 8 lines
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2300
Quoted 5 lines
drivers/nvme/host/core.c +5
-0+break ; + +if (csts == ~0 || ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && !(csts &NVME_CSTS_PP ))) +break ;Can you factor these checks into a helper that your loop checks in the while statement?
Sure, i'll take this in next version.
Quoted 13 lines
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2318
Quoted 9 lines
drivers/nvme/host/core.c +9
-0+nvme_start_queues (ctrl); + ctrl->fw_act_timeout =0 ; + log =kmalloc (sizeof (struct nvme_fw_slot_info_log ),GFP_KERNEL ); +if (!log) +return ; +if (nvme_get_log_page (ctrl,NVME_LOG_FW_SLOT , log)) +dev_warn (ctrl->device, +"Get FW SLOT INFO log error\n" ); +kfree (log);So we don't do anything with the log. It might be worth having a comment like:
Quoted 3 lines
/* read the firmware activation log to clear the AER */for readers of the code that don't know the NVMe spec inside out.
Agree, that'll be helpful. I'll add.
Quoted 19 lines
Quoted 8 lines
drivers/nvme/host/core.c +8
-0+case NVME_AER_NOTICE_FW_ACT_STARTING : + { +u32 csts; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +return ; +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config))drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+return ; + +if ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && (csts &NVME_CSTS_PP )) { +if (ctrl->mtfa) + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (ctrl->mtfa *100 ); +else + ctrl->fw_act_timeout = jiffies + + msecs_to_jiffies(admin_timeout *1000);
Quoted 6 lines
drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2368
Quoted 4 lines
drivers/nvme/host/core.c +4
-0+ +schedule_delayed_work (&ctrl->fw_act_work,0 ); + } +break ;Please split this code into a helper.
Agree, that'll be lot cleaner.
Sagi Grimberg
maintainerPATCH v2 2/2 nvme: Add support for FW activation without reset
In reply to Arnav DawnQuoted 45 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>--- drivers/nvme/host/core.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++- drivers/nvme/host/nvme.h | 3 ++ include/linux/nvme.h | 3 ++ 3 files changed, 76 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +16
-1@@ -1610,7 +1610,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; prev_apsta = ctrl->apsta;if (ctrl->quirks &NVME_QUIRK_NO_APST ) {@@ -2279,6 +2279,45 @@ static void nvme_async_event_work(struct work_struct *work)spin_unlock_irq (&ctrl->lock); } +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; +u32 csts; + +nvme_stop_queues (ctrl); +while (1 ) { +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +break ; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +break ;
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2306
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+ +if (csts == ~0 || ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && !(csts &NVME_CSTS_PP ))) +break ; + +if (time_after (jiffies, ctrl->fw_act_timeout)) { +dev_warn (ctrl->device, +"Fw activation timeout, reset controller\n" ); + ctrl->ops->reset_ctrl (ctrl); +break ;
this triggers controller reset with the request queues stopped, which is different than all the rest of the call sites. I didn't find anything wrong with stopping the queues twice although I vaguely remember having issues with these sort of flows in the past, but things changed since then.
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2311
Quoted 5 lines
drivers/nvme/host/core.c +5
-0+ } +msleep (100 ); + } +nvme_start_queues (ctrl); + ctrl->fw_act_timeout =0 ;
Why is this needed?
Quoted 33 lines
drivers/nvme/host/core.c +26
-0+ log =kmalloc (sizeof (struct nvme_fw_slot_info_log ),GFP_KERNEL ); +if (!log) +return ; +if (nvme_get_log_page (ctrl,NVME_LOG_FW_SLOT , 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) {@@ -2305,6 +2344,34 @@ 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 : + { +u32 csts; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +return ; +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +return ; + +if ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && (csts &NVME_CSTS_PP )) { +if (ctrl->mtfa) + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (ctrl->mtfa *100 ); +else + ctrl->fw_act_timeout = jiffies +
drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2373
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+msecs_to_jiffies (admin_timeout *1000 ); + +schedule_delayed_work (&ctrl->fw_act_work,0 ); + } +break ; + } +case NVME_AER_ERR_FW_IMG_LOAD : +dev_warn (ctrl->device,"FW image load error\n" ); +cancel_delayed_work (&ctrl->fw_act_work); + ctrl->fw_act_timeout =0 ;
Why is this needed?
drivers/nvme/host/core.c void nvme_uninit_ctrl(struct nvme_ctrl *ctrl) · line 2421
Quoted 9 lines
drivers/nvme/host/core.c +2
-0+break ;default :dev_warn (ctrl->device,"async event result %08x\n" , result); }@@ -2351,6 +2418,7 @@ void nvme_uninit_ctrl(struct nvme_ctrl *ctrl) {flush_work (&ctrl->async_event_work);flush_work (&ctrl->scan_work); +cancel_delayed_work (&ctrl->fw_act_work);
Shouldn't this be cancel_delayed_work_sync?
PATCH v2 2/2 nvme: Add support for FW activation without reset
Quoted 6 lines · Sagi Grimberg
Quoted 1 line · Arnav Dawn
Quoted 8 lines
drivers/nvme/host/core.c static void nvme_async_event_work(struct work_struct *work) · line 2311
Quoted 5 lines
drivers/nvme/host/core.c +5
-0+ } +msleep (100 ); + } +nvme_start_queues (ctrl); + ctrl->fw_act_timeout =0 ;Why is this needed?
Fw_act_timeout is reset to initial state once done.
Quoted 38 lines
Quoted 38 lines
drivers/nvme/host/core.c +30
-0+ log =kmalloc (sizeof (struct nvme_fw_slot_info_log ),GFP_KERNEL ); +if (!log) +return ; +if (nvme_get_log_page (ctrl,NVME_LOG_FW_SLOT , 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) {@@ -2305,6 +2344,34 @@ 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 : + { +u32 csts; + +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CSTS , &csts)) +return ; +if (ctrl->ops->reg_read32 (ctrl,NVME_REG_CC , + &ctrl->ctrl_config)) +return ; + +if ((ctrl->ctrl_config &NVME_CC_ENABLE ) + && (csts &NVME_CSTS_PP )) { +if (ctrl->mtfa) + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (ctrl->mtfa *100 ); +else + ctrl->fw_act_timeout = jiffies + +msecs_to_jiffies (admin_timeout *1000 ); + +schedule_delayed_work (&ctrl->fw_act_work,0 ); + }
Quoted 8 lines
drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2373
Quoted 6 lines
drivers/nvme/host/core.c +6
-0+break ; + } +case NVME_AER_ERR_FW_IMG_LOAD : +dev_warn (ctrl->device,"FW image load error\n" ); +cancel_delayed_work (&ctrl->fw_act_work); + ctrl->fw_act_timeout =0 ;Why is this needed?
In case of fw load fail AER, fw_act_timeout is reset to initial state.
Quoted 12 lines
drivers/nvme/host/core.c void nvme_uninit_ctrl(struct nvme_ctrl *ctrl) · line 2421
Quoted 9 lines
drivers/nvme/host/core.c +1
-0+break ;default :dev_warn (ctrl->device,"async event result %08x\n" , result);}drivers/nvme/host/core.c +1
-0@@ -2351,6 +2418,7 @@ void nvme_uninit_ctrl(struct nvme_ctrl *ctrl) {flush_work (&ctrl->async_event_work);flush_work (&ctrl->scan_work); +cancel_delayed_work (&ctrl->fw_act_work);Shouldn't this be
cancel_delayed_work_sync?
yes it should be. Thank you.
Quoted 2 lines
Regards
Arnav Dawn
Sagi Grimberg
maintainerPATCH v2 2/2 nvme: Add support for FW activation without reset
Quoted 2 lines
Fw_act_timeoutis reset to initial state once done.
But you set it before scheduling the delayed work, you either set it always or you don't (AFACIT)