Re: [PATCH 06/14] scsi: qla2xxx: init/os: Remove in_interrupt().

[Himanshu Madhani <himanshu.madhani@oracle.com>]

> 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