Re: [PATCH v2 13/17] qla2xxx: Remove redundant code

[Madhani, Himanshu]

> On May 31, 2017, at 4:37 PM, Bart Van Assche  
> wrote:
> 
> On Tue, 2017-05-30 at 10:54 -0700, Himanshu Madhani wrote:
>> The reason for hard coding LUN ID to 0 is that, from the FC
>> protocol perspective, ABTS does not have any knowledge of
>> LUN ID. So, there is no reason for qla2xxx driver to
>> manufacture the LUN ID.
> 
> Sorry but I think this is completely wrong. Are you aware that
> target_submit_tmr() performs an exact match on the LUN specified as the fourth
> argument? This patch will cause ABTS requests to be ignored that apply to
> commands that have been submitted to another LUN than LUN 0.
> 
> Please note that I had proposed a better approach three months ago on the
> target-devel mailing list and that I'm still waiting for someone from Cavium 
> to
> review these patches:
> * [PATCH v6 09/33] target: Make it possible to specify I_T nexus for SCSI
>   abort (http://www.spinics.net/lists/target-devel/msg14534.html).
> * [PATCH v6 10/33] tcm_qla2xxx: Let the target core look up the LUN of the
>   aborted cmd (http://www.spinics.net/lists/target-devel/msg14563.html).
> 
> Bart.

We will drop this patch from current series and will look at the alternatives 
you have posted.

Thanks,
- Himanshu

Re: [PATCH v2 13/17] qla2xxx: Remove redundant code

[Bart Van Assche]

On Tue, 2017-05-30 at 10:54 -0700, Himanshu Madhani wrote:
> The reason for hard coding LUN ID to 0 is that, from the FC
> protocol perspective, ABTS does not have any knowledge of
> LUN ID. So, there is no reason for qla2xxx driver to
> manufacture the LUN ID.

Sorry but I think this is completely wrong. Are you aware that
target_submit_tmr() performs an exact match on the LUN specified as the fourth
argument? This patch will cause ABTS requests to be ignored that apply to
commands that have been submitted to another LUN than LUN 0.

Please note that I had proposed a better approach three months ago on the
target-devel mailing list and that I'm still waiting for someone from Cavium to
review these patches:
* [PATCH v6 09/33] target: Make it possible to specify I_T nexus for SCSI
  abort (http://www.spinics.net/lists/target-devel/msg14534.html).
* [PATCH v6 10/33] tcm_qla2xxx: Let the target core look up the LUN of the
  aborted cmd (http://www.spinics.net/lists/target-devel/msg14563.html).

Bart.

[PATCH v2 13/17] qla2xxx: Remove redundant code

[Himanshu Madhani]

From: Quinn Tran 

During ABTS or Abort task, qla2xxx does a pre-search for
the se_cmd, based on command's tag. The same search is
performed by TCM. Remove the extra search from qla2xxx.

The reason for hard coding LUN ID to 0 is that, from the FC
protocol perspective, ABTS does not have any knowledge of
LUN ID. So, there is no reason for qla2xxx driver to
manufacture the LUN ID.

Signed-off-by: Quinn Tran 
Signed-off-by: Himanshu Madhani 
Acked-by: Nicholas Bellinger 
---
 drivers/scsi/qla2xxx/qla_target.c | 39 +++
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 765145151fa0..d2c9b565ca00 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1838,40 +1838,24 @@ static void abort_cmds_for_lun(struct scsi_qla_host 
*vha,
spin_unlock_irqrestore(>cmd_list_lock, flags);
 }
 
+/*
+ * From FC protocol perspective, ABTS does not have any knowledge of LUN ID.
+ * Passing default LUN ID of 0.
+ */
+#define DEFAULT_LUN_ID 0
+
 /* ha->hardware_lock supposed to be held on entry */
 static int __qlt_24xx_handle_abts(struct scsi_qla_host *vha,
struct abts_recv_from_24xx *abts, struct fc_port *sess)
 {
struct qla_hw_data *ha = vha->hw;
-   struct se_session *se_sess = sess->se_sess;
struct qla_tgt_mgmt_cmd *mcmd;
-   struct qla_tgt_cmd *cmd;
-   struct se_cmd *se_cmd;
int rc;
-   bool found_lun = false;
-   unsigned long flags;
 
-   spin_lock_irqsave(_sess->sess_cmd_lock, flags);
-   list_for_each_entry(se_cmd, _sess->sess_cmd_list, se_cmd_list) {
-   if (se_cmd->tag == abts->exchange_addr_to_abort) {
-   found_lun = true;
-   break;
-   }
-   }
-   spin_unlock_irqrestore(_sess->sess_cmd_lock, flags);
-
-   /* cmd not in LIO lists, look in qla list */
-   if (!found_lun) {
-   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
-   /* send TASK_ABORT response immediately */
-   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
-   return 0;
-   } else {
-   ql_dbg(ql_dbg_tgt_mgt, vha, 0xf081,
-   "unable to find cmd in driver or LIO for tag 
0x%x\n",
-   abts->exchange_addr_to_abort);
-   return -ENOENT;
-   }
+   if (abort_cmd_for_tag(vha, abts->exchange_addr_to_abort)) {
+   /* send TASK_ABORT response immediately */
+   qlt_24xx_send_abts_resp(vha, abts, FCP_TMF_CMPL, false);
+   return 0;
}
 
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
@@ -1887,13 +1871,12 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host 
*vha,
}
memset(mcmd, 0, sizeof(*mcmd));
 
-   cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
mcmd->sess = sess;
memcpy(>orig_iocb.abts, abts, sizeof(mcmd->orig_iocb.abts));
mcmd->reset_count = vha->hw->chip_reset;
mcmd->tmr_func = QLA_TGT_ABTS;
 
-   rc = ha->tgt.tgt_ops->handle_tmr(mcmd, cmd->unpacked_lun, 
mcmd->tmr_func,
+   rc = ha->tgt.tgt_ops->handle_tmr(mcmd, DEFAULT_LUN_ID, mcmd->tmr_func,
abts->exchange_addr_to_abort);
if (rc != 0) {
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf052,
-- 
2.12.0