| Message ID | 20201126132952.2287996-7-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: Remove in_interrupt() usage. | expand |
On Thu, Nov 26, 2020 at 02:29:44PM +0100, Sebastian Andrzej Siewior wrote: > From: "Ahmed S. Darwish" <a.darwish@linutronix.de> > > qla83xx_wait_logic() is used to control the frequency of device IDC lock > retries. If in_interrupt() is true, it does 20 loops of cpu_relax(). > Otherwise, it sleeps for 100ms and yields the CPU. > > While in_interrupt() is ill-defined and does not provide what the name > suggests, it is not needed here: that qla83xx_wait_logic() is exclusively > called by qla83xx_idc_lock() / unlock(), and they always run from > process context. Below is an analysis of all the idc lock/unlock callers, > in order of appearance: > > - qla_os.c: > qla83xx_nic_core_unrecoverable_work(), > qla83xx_idc_state_handler_work(), > qla83xx_nic_core_reset_work(), > qla83xx_service_idc_aen(), all workqueue context > > - qla_os.c: qla83xx_check_nic_core_fw_alive(), has msleep() > > - qla_os.c: qla83xx_set_drv_presence(), called once from > qla2x00_abort_isp(), which is bound to process-context ->abort_isp() > hook. It also invokes wait_for_completion_timeout() through the > chain qla2x00_configure_hba() => qla24xx_link_initialize() => > qla2x00_mailbox_command(). > > - qla_os.c: qla83xx_clear_drv_presence(), which is called from > qla2x00_abort_isp() discussed above, and from qla2x00_remove_one() > which is PCI process-context ->remove() hook. > > - qla_os.c: qla83xx_need_reset_handler(), has a one second msleep() in > a loop. > > - qla_os.c: qla83xx_device_bootstrap(), called only by > qla83xx_idc_state_handler(), which has multiple msleep() > invocations. > > - qla_os.c: qla83xx_idc_state_handler(), multiple msleep() > invocations. > > - qla_attr.c: qla2x00_sysfs_write_reset(), sysfs bin_attribute > ->write() hook, process context > > - qla_init.c: qla83xx_nic_core_fw_load() > => qla_init.c: qla2x00_initialize_adapter() > => bound to isp_operations ->initialize_adapter() hook > ** => qla_os.c: qla2x00_probe_one(), PCI ->probe() process ctx > > - qla_init.c: qla83xx_initiating_reset(), msleep() in a loop. > > - qla_init.c: qla83xx_nic_core_reset(), called by > qla83xx_nic_core_reset_work(), workqueue context. > > Remove the in_interrupt() check, and thus replace the entirety of > qla83xx_wait_logic() with an msleep(QLA83XX_WAIT_LOGIC_MS). > > Mark qla83xx_idc_lock() / unlock() with "Context: task, can sleep". > > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Nilesh Javali <njavali@marvell.com> > Cc: GR-QLogic-Storage-Upstream@marvell.com Reviewed-by: Daniel Wagner <dwagner@suse.de>
> On Nov 26, 2020, at 7:29 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > From: "Ahmed S. Darwish" <a.darwish@linutronix.de> > > qla83xx_wait_logic() is used to control the frequency of device IDC lock > retries. If in_interrupt() is true, it does 20 loops of cpu_relax(). > Otherwise, it sleeps for 100ms and yields the CPU. > > While in_interrupt() is ill-defined and does not provide what the name > suggests, it is not needed here: that qla83xx_wait_logic() is exclusively > called by qla83xx_idc_lock() / unlock(), and they always run from > process context. Below is an analysis of all the idc lock/unlock callers, > in order of appearance: > > - qla_os.c: > qla83xx_nic_core_unrecoverable_work(), > qla83xx_idc_state_handler_work(), > qla83xx_nic_core_reset_work(), > qla83xx_service_idc_aen(), all workqueue context > > - qla_os.c: qla83xx_check_nic_core_fw_alive(), has msleep() > > - qla_os.c: qla83xx_set_drv_presence(), called once from > qla2x00_abort_isp(), which is bound to process-context ->abort_isp() > hook. It also invokes wait_for_completion_timeout() through the > chain qla2x00_configure_hba() => qla24xx_link_initialize() => > qla2x00_mailbox_command(). > > - qla_os.c: qla83xx_clear_drv_presence(), which is called from > qla2x00_abort_isp() discussed above, and from qla2x00_remove_one() > which is PCI process-context ->remove() hook. > > - qla_os.c: qla83xx_need_reset_handler(), has a one second msleep() in > a loop. > > - qla_os.c: qla83xx_device_bootstrap(), called only by > qla83xx_idc_state_handler(), which has multiple msleep() > invocations. > > - qla_os.c: qla83xx_idc_state_handler(), multiple msleep() > invocations. > > - qla_attr.c: qla2x00_sysfs_write_reset(), sysfs bin_attribute > ->write() hook, process context > > - qla_init.c: qla83xx_nic_core_fw_load() > => qla_init.c: qla2x00_initialize_adapter() > => bound to isp_operations ->initialize_adapter() hook > ** => qla_os.c: qla2x00_probe_one(), PCI ->probe() process ctx > > - qla_init.c: qla83xx_initiating_reset(), msleep() in a loop. > > - qla_init.c: qla83xx_nic_core_reset(), called by > qla83xx_nic_core_reset_work(), workqueue context. > > Remove the in_interrupt() check, and thus replace the entirety of > qla83xx_wait_logic() with an msleep(QLA83XX_WAIT_LOGIC_MS). > > Mark qla83xx_idc_lock() / unlock() with "Context: task, can sleep". > > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Nilesh Javali <njavali@marvell.com> > Cc: GR-QLogic-Storage-Upstream@marvell.com > --- > drivers/scsi/qla2xxx/qla_os.c | 43 ++++++++++++++++------------------- > 1 file changed, 19 insertions(+), 24 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c > index f9c8ae9d669ef..2a8e065b192c3 100644 > --- a/drivers/scsi/qla2xxx/qla_os.c > +++ b/drivers/scsi/qla2xxx/qla_os.c > @@ -5619,25 +5619,10 @@ qla83xx_service_idc_aen(struct work_struct *work) > } > } > > -static void > -qla83xx_wait_logic(void) > -{ > - int i; > - > - /* Yield CPU */ > - if (!in_interrupt()) { > - /* > - * Wait about 200ms before retrying again. > - * This controls the number of retries for single > - * lock operation. > - */ > - msleep(100); > - schedule(); > - } else { > - for (i = 0; i < 20; i++) > - cpu_relax(); /* This a nop instr on i386 */ > - } > -} > +/* > + * Control the frequency of IDC lock retries > + */ > +#define QLA83XX_WAIT_LOGIC_MS 100 > > static int > qla83xx_force_lock_recovery(scsi_qla_host_t *base_vha) > @@ -5727,7 +5712,7 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha) > goto exit; > > if (o_drv_lockid == n_drv_lockid) { > - qla83xx_wait_logic(); > + msleep(QLA83XX_WAIT_LOGIC_MS); > goto retry_lockid; > } else > return QLA_SUCCESS; > @@ -5736,6 +5721,9 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha) > return rval; > } > > +/* > + * Context: task, can sleep > + */ > void > qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id) > { > @@ -5743,6 +5731,8 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id) > uint32_t lock_owner; > struct qla_hw_data *ha = base_vha->hw; > > + might_sleep(); > + > /* IDC-lock implementation using driver-lock/lock-id remote registers */ > retry_lock: > if (qla83xx_rd_reg(base_vha, QLA83XX_DRIVER_LOCK, &data) > @@ -5761,7 +5751,7 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id) > /* Retry/Perform IDC-Lock recovery */ > if (qla83xx_idc_lock_recovery(base_vha) > == QLA_SUCCESS) { > - qla83xx_wait_logic(); > + msleep(QLA83XX_WAIT_LOGIC_MS); > goto retry_lock; > } else > ql_log(ql_log_warn, base_vha, 0xb075, > @@ -6259,6 +6249,9 @@ void qla24xx_process_purex_list(struct purex_list *list) > } > } > > +/* > + * Context: task, can sleep > + */ > void > qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) > { > @@ -6269,6 +6262,8 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) > uint32_t data; > struct qla_hw_data *ha = base_vha->hw; > > + might_sleep(); > + > /* IDC-unlock implementation using driver-unlock/lock-id > * remote registers > */ > @@ -6284,7 +6279,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) > /* SV: XXX: IDC unlock retrying needed here? */ > > /* Retry for IDC-unlock */ > - qla83xx_wait_logic(); > + msleep(QLA83XX_WAIT_LOGIC_MS); > retry++; > ql_dbg(ql_dbg_p3p, base_vha, 0xb064, > "Failed to release IDC lock, retrying=%d\n", retry); > @@ -6292,7 +6287,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) > } > } else if (retry < 10) { > /* Retry for IDC-unlock */ > - qla83xx_wait_logic(); > + msleep(QLA83XX_WAIT_LOGIC_MS); > retry++; > ql_dbg(ql_dbg_p3p, base_vha, 0xb065, > "Failed to read drv-lockid, retrying=%d\n", retry); > @@ -6308,7 +6303,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) > if (qla83xx_access_control(base_vha, options, 0, 0, NULL)) { > if (retry < 10) { > /* Retry for IDC-unlock */ > - qla83xx_wait_logic(); > + msleep(QLA83XX_WAIT_LOGIC_MS); > retry++; > ql_dbg(ql_dbg_p3p, base_vha, 0xb066, > "Failed to release IDC lock, retrying=%d\n", retry); > > 2.29.2 > Looks Good. Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> -- Himanshu Madhani Oracle Linux Engineering
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c index f9c8ae9d669ef..2a8e065b192c3 100644 --- a/drivers/scsi/qla2xxx/qla_os.c +++ b/drivers/scsi/qla2xxx/qla_os.c @@ -5619,25 +5619,10 @@ qla83xx_service_idc_aen(struct work_struct *work) } } -static void -qla83xx_wait_logic(void) -{ - int i; - - /* Yield CPU */ - if (!in_interrupt()) { - /* - * Wait about 200ms before retrying again. - * This controls the number of retries for single - * lock operation. - */ - msleep(100); - schedule(); - } else { - for (i = 0; i < 20; i++) - cpu_relax(); /* This a nop instr on i386 */ - } -} +/* + * Control the frequency of IDC lock retries + */ +#define QLA83XX_WAIT_LOGIC_MS 100 static int qla83xx_force_lock_recovery(scsi_qla_host_t *base_vha) @@ -5727,7 +5712,7 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha) goto exit; if (o_drv_lockid == n_drv_lockid) { - qla83xx_wait_logic(); + msleep(QLA83XX_WAIT_LOGIC_MS); goto retry_lockid; } else return QLA_SUCCESS; @@ -5736,6 +5721,9 @@ qla83xx_idc_lock_recovery(scsi_qla_host_t *base_vha) return rval; } +/* + * Context: task, can sleep + */ void qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id) { @@ -5743,6 +5731,8 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id) uint32_t lock_owner; struct qla_hw_data *ha = base_vha->hw; + might_sleep(); + /* IDC-lock implementation using driver-lock/lock-id remote registers */ retry_lock: if (qla83xx_rd_reg(base_vha, QLA83XX_DRIVER_LOCK, &data) @@ -5761,7 +5751,7 @@ qla83xx_idc_lock(scsi_qla_host_t *base_vha, uint16_t requester_id) /* Retry/Perform IDC-Lock recovery */ if (qla83xx_idc_lock_recovery(base_vha) == QLA_SUCCESS) { - qla83xx_wait_logic(); + msleep(QLA83XX_WAIT_LOGIC_MS); goto retry_lock; } else ql_log(ql_log_warn, base_vha, 0xb075, @@ -6259,6 +6249,9 @@ void qla24xx_process_purex_list(struct purex_list *list) } } +/* + * Context: task, can sleep + */ void qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) { @@ -6269,6 +6262,8 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) uint32_t data; struct qla_hw_data *ha = base_vha->hw; + might_sleep(); + /* IDC-unlock implementation using driver-unlock/lock-id * remote registers */ @@ -6284,7 +6279,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) /* SV: XXX: IDC unlock retrying needed here? */ /* Retry for IDC-unlock */ - qla83xx_wait_logic(); + msleep(QLA83XX_WAIT_LOGIC_MS); retry++; ql_dbg(ql_dbg_p3p, base_vha, 0xb064, "Failed to release IDC lock, retrying=%d\n", retry); @@ -6292,7 +6287,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) } } else if (retry < 10) { /* Retry for IDC-unlock */ - qla83xx_wait_logic(); + msleep(QLA83XX_WAIT_LOGIC_MS); retry++; ql_dbg(ql_dbg_p3p, base_vha, 0xb065, "Failed to read drv-lockid, retrying=%d\n", retry); @@ -6308,7 +6303,7 @@ qla83xx_idc_unlock(scsi_qla_host_t *base_vha, uint16_t requester_id) if (qla83xx_access_control(base_vha, options, 0, 0, NULL)) { if (retry < 10) { /* Retry for IDC-unlock */ - qla83xx_wait_logic(); + msleep(QLA83XX_WAIT_LOGIC_MS); retry++; ql_dbg(ql_dbg_p3p, base_vha, 0xb066, "Failed to release IDC lock, retrying=%d\n", retry);