Skip to contentSkip to conversation

qla2xxx: Patches for kernel v4.7

PATCH Applied Bart Van Assche 4 participants 15 messages Mar 30, 2016 – Apr 1, 2016

Subsystems (2)
SCSI QLogic QLA2XXX FC-SCSI driver

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

cover qla2xxx: Patches for kernel v4.7

Hello James and Martin,

The following three patches are what I came up with after having analyzed the output of a static source code analysis tool (sparse) for the QLogic qla2xxx driver:

0001-qla2xxx-Indicate-out-of-memory-with-ENOMEM.patch 0002-qla2xxx-Remove-set-but-not-used-variables.patch 0003-qla2xxx-Assign-names-to-the-flags-in-cmd_flags.patch

Please consider these patches for inclusion in kernel v4.7.

Thanks,

Bart.

author
#1 lore raw

PATCH 1/3 qla2xxx: Indicate out-of-memory with -ENOMEM

In the Linux kernel it is preferred to return a meaningful error code instead of -1. This patch does not change the behavior of the caller of qla82xx_pinit_from_rom().

Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_nx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_nx.c +1 -1
index b6b4cfd..54380b4 100644
@@ -1229,7 +1229,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	if (buf == NULL) {
 		ql_log(ql_log_fatal, vha, 0x010c,
 		    "Unable to allocate memory.\n");
-		return -1;
+		return -ENOMEM;
 	}
 
 	for (i = 0; i < n; i++) {
Signature · 1 line
2.7.3
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] qla2xxx: Indicate out-of-memory with -ENOMEM

Quoted 17 lines · Bart Van Assche

In the Linux kernel it is preferred to return a meaningful error code instead of -1. This patch does not change the behavior of the caller of qla82xx_pinit_from_rom().

Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_nx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_nx.c +0 -0
index b6b4cfd..54380b4 100644

drivers/scsi/qla2xxx/qla_nx.c qla82xx_pinit_from_rom(scsi_qla_host_t *vha) · line 1235

Quoted 8 lines
drivers/scsi/qla2xxx/qla_nx.c +1 -1
@@ -1229,7 +1229,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	if (buf == NULL) {
 		ql_log(ql_log_fatal, vha, 0x010c,
 		    "Unable to allocate memory.\n");
-		return -1;
+		return -ENOMEM;
 	}
 
 	for (i = 0; i < n; i++) {

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>
#3 lore raw

Re: [PATCH 1/3] qla2xxx: Indicate out-of-memory with -ENOMEM

-----Original Message-----
From: Johannes Thumshirn <[email protected]>
Date: Thursday, March 31, 2016 at 12:30 AM
To: Bart Van Assche <[email protected]>
Cc: James Bottomley <[email protected]>, "Martin K. Petersen" <[email protected]>, Himanshu Madhani <[email protected]>, Quinn Tran <[email protected]>, Christoph Hellwig <[email protected]>, linux-scsi <[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [PATCH 1/3] qla2xxx: Indicate out-of-memory with -ENOMEM

Quoted 26 lines · Bart Van Assche
Quoted 25 lines

In the Linux kernel it is preferred to return a meaningful error code instead of -1. This patch does not change the behavior of the caller of qla82xx_pinit_from_rom().

Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_nx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_nx.c +1 -1
index b6b4cfd..54380b4 100644
@@ -1229,7 +1229,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	if (buf == NULL) {
 		ql_log(ql_log_fatal, vha, 0x010c,
 		    "Unable to allocate memory.\n");
-		return -1;
+		return -ENOMEM;
 	}
Quoted 4 lines

drivers/scsi/qla2xxx/qla_nx.c qla82xx_pinit_from_rom(scsi_qla_host_t *vha) · line 1235

Quoted 1 line
drivers/scsi/qla2xxx/qla_nx.c +0 -0
 	for (i = 0; i < n; i++) {

Looks good,

Reviewed-by Johannes Thumshirn <[email protected]>

ACK.

Quoted 1 line
Applied
#4 lore raw

Re: [PATCH 1/3] qla2xxx: Indicate out-of-memory with -ENOMEM

In reply to Bart Van Assche
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Bart" == Bart Van Assche <[email protected]> writes:

Bart> In the Linux kernel it is preferred to return a meaningful error Bart> code instead of -1. This patch does not change the behavior of the Bart> caller of qla82xx_pinit_from_rom().

Applied to 4.7/scsi-queue.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author
#5 lore raw

PATCH 2/3 qla2xxx: Remove set-but-not-used variables

In reply to Bart Van Assche

Detected these variables by building the qla2xxx driver with W=1.
Note: removing the code that sets BIT_14 is fine since that bit is never tested. The output of git grep -nH -- '->cmd_flags\s*&' drivers/scsi/qla2xxx is as follows:

drivers/scsi/qla2xxx/tcm_qla2xxx.c:285: WARN_ON(cmd->cmd_flags & BIT_16);
drivers/scsi/qla2xxx/tcm_qla2xxx.c:302: BUG_ON(cmd->cmd_flags & BIT_20);
drivers/scsi/qla2xxx/tcm_qla2xxx.c:627: if (cmd->cmd_flags & BIT_5) {

Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_attr.c    | 3 +--
 drivers/scsi/qla2xxx/qla_mbx.c     | 2 --
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 7 -------
 3 files changed, 1 insertion(+), 11 deletions(-)
drivers/scsi/qla2xxx/qla_attr.c +1 -2
index 4dc06a13..db85c5c 100644
@@ -839,7 +839,6 @@ qla2x00_issue_logo(struct file *filp, struct kobject *kobj,
 	struct scsi_qla_host *vha = shost_priv(dev_to_shost(container_of(kobj,
 	    struct device, kobj)));
 	int type;
-	int rval = 0;
 	port_id_t did;
 
 	type = simple_strtol(buf, NULL, 10);

@@ -853,7 +852,7 @@ qla2x00_issue_logo(struct file *filp, struct kobject *kobj,
 
 	ql_log(ql_log_info, vha, 0x70e4, "%s: %d\n", __func__, type);
 
-	rval = qla24xx_els_dcmd_iocb(vha, ELS_DCMD_LOGO, did);
+	qla24xx_els_dcmd_iocb(vha, ELS_DCMD_LOGO, did);
 	return count;
 }
drivers/scsi/qla2xxx/qla_mbx.c +0 -2
index 968b846..bf2d357 100644
@@ -607,7 +607,6 @@ qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha, dma_addr_t phys_addr)
 	mbx_cmd_t	mc;
 	mbx_cmd_t	*mcp = &mc;
 	struct qla_hw_data *ha = vha->hw;
-	int configured_count;
 
 	ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x111a,
 	    "Entered %s.\n", __func__);

@@ -630,7 +629,6 @@ qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha, dma_addr_t phys_addr)
 		/*EMPTY*/
 		ql_dbg(ql_dbg_mbx, vha, 0x111b, "Failed=%x.\n", rval);
 	} else {
-		configured_count = mcp->mb[11];
 		ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x118c,
 		    "Done %s.\n", __func__);
 	}
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -7
index c1461d2..97fcbf2 100644
@@ -311,13 +311,6 @@ static void tcm_qla2xxx_free_cmd(struct qla_tgt_cmd *cmd)
  */
 static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd)
 {
-	struct qla_tgt_cmd *cmd;
-
-	if ((se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) == 0) {
-		cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
-		cmd->cmd_flags |= BIT_14;
-	}
-
 	return target_put_sess_cmd(se_cmd);
 }
Signature · 1 line
2.7.3
Reviewed-by on v1 2/3
#6 lore raw

Re: [PATCH 2/3] qla2xxx: Remove set-but-not-used variables

Quoted 36 lines · Bart Van Assche

Detected these variables by building the qla2xxx driver with W=1.
Note: removing the code that sets BIT_14 is fine since that bit is never tested. The output of git grep -nH -- '->cmd_flags\s*&' drivers/scsi/qla2xxx
is as follows:

drivers/scsi/qla2xxx/tcm_qla2xxx.c:285: WARN_ON(cmd->cmd_flags & BIT_16);
drivers/scsi/qla2xxx/tcm_qla2xxx.c:302: BUG_ON(cmd->cmd_flags & BIT_20);
drivers/scsi/qla2xxx/tcm_qla2xxx.c:627: if (cmd->cmd_flags & BIT_5) {

Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_attr.c    | 3 +--
 drivers/scsi/qla2xxx/qla_mbx.c     | 2 --
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 7 -------
 3 files changed, 1 insertion(+), 11 deletions(-)
drivers/scsi/qla2xxx/qla_attr.c +0 -1
index 4dc06a13..db85c5c 100644
@@ -839,7 +839,6 @@ qla2x00_issue_logo(struct file *filp, struct 
 kobject *kobj,
 	struct scsi_qla_host *vha = 
 shost_priv(dev_to_shost(container_of(kobj,
 	    struct device, kobj)));
 	int type;
-	int rval = 0;
 	port_id_t did;
drivers/scsi/qla2xxx/qla_attr.c +0 -0
 	type = simple_strtol(buf, NULL, 10);

drivers/scsi/qla2xxx/qla_attr.c qla2x00_issue_logo(struct file *filp, struct · line 858

Quoted 8 lines
drivers/scsi/qla2xxx/qla_attr.c +1 -1
@@ -853,7 +852,7 @@ qla2x00_issue_logo(struct file *filp, struct 
 kobject *kobj,
 
 	ql_log(ql_log_info, vha, 0x70e4, "%s: %d\n", __func__, type);
 
-	rval = qla24xx_els_dcmd_iocb(vha, ELS_DCMD_LOGO, did);
+	qla24xx_els_dcmd_iocb(vha, ELS_DCMD_LOGO, did);
 	return count;
 }

OK, qla24xx_els_dcmd_iocb() is a bit scary. It can return -ENOMEM, but no caller checks it (the 2nd caller _only_ checks the return value in a debug message). Maybe we should think about that.

Quoted 31 lines
drivers/scsi/qla2xxx/qla_mbx.c +0 -1
index 968b846..bf2d357 100644
@@ -607,7 +607,6 @@ qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha,
 dma_addr_t phys_addr)
 	mbx_cmd_t	mc;
 	mbx_cmd_t	*mcp = &mc;
 	struct qla_hw_data *ha = vha->hw;
-	int configured_count;
 
 	ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x111a,
drivers/scsi/qla2xxx/qla_mbx.c +0 -1
 	    "Entered %s.\n", __func__);

@@ -630,7 +629,6 @@ qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha,
 dma_addr_t phys_addr)
 		/*EMPTY*/
 		ql_dbg(ql_dbg_mbx, vha, 0x111b, "Failed=%x.\n", rval);
 	} else {
-		configured_count = mcp->mb[11];
 		ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x118c,
 		    "Done %s.\n", __func__);
drivers/scsi/qla2xxx/qla_mbx.c +0 -0
 	}
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -0
index c1461d2..97fcbf2 100644
@@ -311,13 +311,6 @@ static void tcm_qla2xxx_free_cmd(struct 
 qla_tgt_cmd *cmd)
  */
 static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd)

drivers/scsi/qla2xxx/tcm_qla2xxx.c static void tcm_qla2xxx_free_cmd(struct · line 316

Quoted 10 lines
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -7
 {
-	struct qla_tgt_cmd *cmd;
-
-	if ((se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) == 0) {
-		cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
-		cmd->cmd_flags |= BIT_14;
-	}
-
 	return target_put_sess_cmd(se_cmd);
 }

Apart from the qla24xx_els_dcmd_iocb() thingy (which has nothing to do with your patch)

Reviewed-by Johannes Thumshirn <[email protected]>
#7 lore raw

Re: [PATCH 2/3] qla2xxx: Remove set-but-not-used variables

-----Original Message-----
From: Johannes Thumshirn <[email protected]>
Date: Thursday, March 31, 2016 at 12:37 AM
To: Bart Van Assche <[email protected]>
Cc: James Bottomley <[email protected]>, "Martin K. Petersen" <[email protected]>, Himanshu Madhani <[email protected]>, Quinn Tran <[email protected]>, Christoph Hellwig <[email protected]>, linux-scsi <[email protected]>, "[email protected]" <[email protected]>
Subject: Re: [PATCH 2/3] qla2xxx: Remove set-but-not-used variables

Quoted 98 lines · Bart Van Assche
Quoted 39 lines

Detected these variables by building the qla2xxx driver with W=1.
Note: removing the code that sets BIT_14 is fine since that bit is never tested. The output of git grep -nH -- '->cmd_flags\s*&' drivers/scsi/qla2xxx
is as follows:

drivers/scsi/qla2xxx/tcm_qla2xxx.c:285: WARN_ON(cmd->cmd_flags &
BIT_16);
drivers/scsi/qla2xxx/tcm_qla2xxx.c:302: BUG_ON(cmd->cmd_flags &
BIT_20);
drivers/scsi/qla2xxx/tcm_qla2xxx.c:627: if (cmd->cmd_flags & BIT_5) {

Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_attr.c    | 3 +--
 drivers/scsi/qla2xxx/qla_mbx.c     | 2 --
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 7 -------
 3 files changed, 1 insertion(+), 11 deletions(-)
drivers/scsi/qla2xxx/qla_attr.c +0 -1
index 4dc06a13..db85c5c 100644
@@ -839,7 +839,6 @@ qla2x00_issue_logo(struct file *filp, struct 
 kobject *kobj,
 	struct scsi_qla_host *vha = 
 shost_priv(dev_to_shost(container_of(kobj,
 	    struct device, kobj)));
 	int type;
-	int rval = 0;
 	port_id_t did;
drivers/scsi/qla2xxx/qla_attr.c +0 -0
 	type = simple_strtol(buf, NULL, 10);

drivers/scsi/qla2xxx/qla_attr.c qla2x00_issue_logo(struct file *filp, struct · line 858

Quoted 8 lines
drivers/scsi/qla2xxx/qla_attr.c +1 -1
@@ -853,7 +852,7 @@ qla2x00_issue_logo(struct file *filp, struct 
 kobject *kobj,
 
 	ql_log(ql_log_info, vha, 0x70e4, "%s: %d\n", __func__, type);
 
-	rval = qla24xx_els_dcmd_iocb(vha, ELS_DCMD_LOGO, did);
+	qla24xx_els_dcmd_iocb(vha, ELS_DCMD_LOGO, did);
 	return count;
 }

OK, qla24xx_els_dcmd_iocb() is a bit scary. It can return -ENOMEM, but no caller checks it (the 2nd caller _only_ checks the return value in a debug message). Maybe we should think about that.

Quoted 32 lines
drivers/scsi/qla2xxx/qla_mbx.c +0 -1
index 968b846..bf2d357 100644
@@ -607,7 +607,6 @@ qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha,
 dma_addr_t phys_addr)
 	mbx_cmd_t	mc;
 	mbx_cmd_t	*mcp = &mc;
 	struct qla_hw_data *ha = vha->hw;
-	int configured_count;
 
 	ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x111a,
drivers/scsi/qla2xxx/qla_mbx.c +0 -1
 	    "Entered %s.\n", __func__);

@@ -630,7 +629,6 @@ qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha,
 dma_addr_t phys_addr)
 		/*EMPTY*/
 		ql_dbg(ql_dbg_mbx, vha, 0x111b, "Failed=%x.\n", rval);
 	} else {
-		configured_count = mcp->mb[11];
 		ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x118c,
 		    "Done %s.\n", __func__);
drivers/scsi/qla2xxx/qla_mbx.c +0 -0
 	}
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -0
index c1461d2..97fcbf2 100644
@@ -311,13 +311,6 @@ static void tcm_qla2xxx_free_cmd(struct 
 qla_tgt_cmd *cmd)
  */
 static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd)

drivers/scsi/qla2xxx/tcm_qla2xxx.c static void tcm_qla2xxx_free_cmd(struct · line 316

Quoted 10 lines
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -7
 {
-	struct qla_tgt_cmd *cmd;
-
-	if ((se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) == 0) {
-		cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
-		cmd->cmd_flags |= BIT_14;
-	}
-
 	return target_put_sess_cmd(se_cmd);
 }

Apart from the qla24xx_els_dcmd_iocb() thingy (which has nothing to do with your patch)

Quoted 1 line
Reviewed-by Johannes Thumshirn <[email protected]>

QT: We use the cmd_flags portion for debugging crashes. It’s our form of tracing. Please do not delete. Thanks.

Quoted 1 line
author
#8 lore raw

Re: [PATCH 2/3] qla2xxx: Remove set-but-not-used variables

Quoted 24 lines · Quinn Tran
Quoted 22 lines
Quoted 9 lines
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -0
index c1461d2..97fcbf2 100644
@@ -311,13 +311,6 @@ static void tcm_qla2xxx_free_cmd(struct
 qla_tgt_cmd *cmd)
   */
  static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd)

drivers/scsi/qla2xxx/tcm_qla2xxx.c static void tcm_qla2xxx_free_cmd(struct · line 316

Quoted 10 lines
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -7
  {
-	struct qla_tgt_cmd *cmd;
-
-	if ((se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB) == 0) {
-		cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
-		cmd->cmd_flags |= BIT_14;
-	}
-
  	return target_put_sess_cmd(se_cmd);
  }

Apart from the qla24xx_els_dcmd_iocb() thingy (which has nothing to do with your patch)

Quoted 1 line
Reviewed-by Johannes Thumshirn <[email protected]>
Quoted 1 line

QT: We use the cmd_flags portion for debugging crashes. It’s our form of tracing. Please do not delete. Thanks.

Hello Quinn,

OK, I will restore that code.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#9 lore raw

PATCH 3/3 qla2xxx: Assign names to the flags in cmd_flags

In reply to Bart Van Assche

Use a C bitfield instead of emulating this functionality with bit manipulations. Since only bits 5, 16 and 20 are ever tested, remove the code that sets the other bits. This is easy to verify by running the following command:

$ git grep -nH 'BIT_[0-9]' drivers/scsi/qla2xxx | grep cmd_flags
Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_target.c  | 23 +++++------------------
 drivers/scsi/qla2xxx/qla_target.h  | 36 +++++-------------------------------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 33 ++++++++++++++++-----------------
 3 files changed, 26 insertions(+), 66 deletions(-)
drivers/scsi/qla2xxx/qla_target.c +5 -18
index 8a44d15..8b8622d 100644
@@ -3291,7 +3291,6 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd)
 		return EIO;
 	}
 	cmd->aborted = 1;
-	cmd->cmd_flags |= BIT_6;
 	spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 
 	qlt_send_term_exchange(vha, cmd, &cmd->atio, 0, 1);

@@ -3339,7 +3338,6 @@ static int qlt_prepare_srr_ctio(struct scsi_qla_host *vha,
 	struct qla_tgt_srr_imm *imm;
 
 	tgt->ctio_srr_id++;
-	cmd->cmd_flags |= BIT_15;
 
 	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf019,
 	    "qla_target(%d): CTIO with SRR status received\n", vha->vp_idx);

@@ -3522,7 +3520,6 @@ qlt_abort_cmd_on_host_reset(struct scsi_qla_host *vha, struct qla_tgt_cmd *cmd)
 		dump_stack();
 	}
 
-	cmd->cmd_flags |= BIT_17;
 	ha->tgt.tgt_ops->free_cmd(cmd);
 }
 

@@ -3688,7 +3685,6 @@ static void qlt_do_ctio_completion(struct scsi_qla_host *vha, uint32_t handle,
 		 */
 		if ((cmd->state != QLA_TGT_STATE_NEED_DATA) &&
 		    (!cmd->aborted)) {
-			cmd->cmd_flags |= BIT_13;
 			if (qlt_term_ctio_exchange(vha, ctio, cmd, status))
 				return;
 		}

@@ -3696,7 +3692,7 @@ static void qlt_do_ctio_completion(struct scsi_qla_host *vha, uint32_t handle,
 skip_term:
 
 	if (cmd->state == QLA_TGT_STATE_PROCESSED) {
-		cmd->cmd_flags |= BIT_12;
+		;
 	} else if (cmd->state == QLA_TGT_STATE_NEED_DATA) {
 		cmd->state = QLA_TGT_STATE_DATA_IN;
 

@@ -3706,11 +3702,9 @@ skip_term:
 		ha->tgt.tgt_ops->handle_data(cmd);
 		return;
 	} else if (cmd->aborted) {
-		cmd->cmd_flags |= BIT_18;
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf01e,
 		  "Aborted command %p (tag %lld) finished\n", cmd, se_cmd->tag);
 	} else {
-		cmd->cmd_flags |= BIT_19;
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf05c,
 		    "qla_target(%d): A command in state (%d) should "
 		    "not return a CTIO complete\n", vha->vp_idx, cmd->state);

@@ -3775,7 +3769,6 @@ static void __qlt_do_work(struct qla_tgt_cmd *cmd)
 	int ret, fcp_task_attr, data_dir, bidi = 0;
 
 	cmd->cmd_in_wq = 0;
-	cmd->cmd_flags |= BIT_1;
 	if (tgt->tgt_stop)
 		goto out_term;
 

@@ -3827,7 +3820,6 @@ out_term:
 	 * cmd has not sent to target yet, so pass NULL as the second
 	 * argument to qlt_send_term_exchange() and free the memory here.
 	 */
-	cmd->cmd_flags |= BIT_2;
 	spin_lock_irqsave(&ha->hardware_lock, flags);
 	qlt_send_term_exchange(vha, NULL, &cmd->atio, 1, 0);
 

@@ -3878,7 +3870,8 @@ static struct qla_tgt_cmd *qlt_get_tag(scsi_qla_host_t *vha,
 	cmd->loop_id = sess->loop_id;
 	cmd->conf_compl_supported = sess->conf_compl_supported;
 
-	cmd->cmd_flags = 0;
+	cmd->status_queued = cmd->cmd_freed = cmd->complete_free = 0;
+	cmd->data_work = cmd->data_work_free = 0;
 	cmd->jiffies_at_alloc = get_jiffies_64();
 
 	cmd->reset_count = vha->hw->chip_reset;

@@ -4014,7 +4007,6 @@ static int qlt_handle_cmd_for_atio(struct scsi_qla_host *vha,
 	}
 
 	cmd->cmd_in_wq = 1;
-	cmd->cmd_flags |= BIT_0;
 	cmd->se_cmd.cpuid = ha->msix_count ?
 		ha->tgt.rspq_vector_cpuid : WORK_CPU_UNBOUND;
 

@@ -4767,10 +4759,8 @@ static void qlt_handle_srr(struct scsi_qla_host *vha,
 			qlt_send_notify_ack(vha, ntfy,
 			    0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
-			if (xmit_type & QLA_TGT_XMIT_DATA) {
-				cmd->cmd_flags |= BIT_8;
+			if (xmit_type & QLA_TGT_XMIT_DATA)
 				qlt_rdy_to_xfer(cmd);
-			}
 		} else {
 			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf066,
 			    "qla_target(%d): SRR for out data for cmd without them (tag %lld, SCSI status %d), reject",

@@ -4786,10 +4776,8 @@ static void qlt_handle_srr(struct scsi_qla_host *vha,
 	}
 
 	/* Transmit response in case of status and data-in cases */
-	if (resp) {
-		cmd->cmd_flags |= BIT_7;
+	if (resp)
 		qlt_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
-	}
 
 	return;
 

@@ -4803,7 +4791,6 @@ out_reject:
 		cmd->state = QLA_TGT_STATE_DATA_IN;
 		dump_stack();
 	} else {
-		cmd->cmd_flags |= BIT_9;
 		qlt_send_term_exchange(vha, cmd, &cmd->atio, 1, 0);
 	}
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
drivers/scsi/qla2xxx/qla_target.h +5 -31
index d857fee..9cf2aef 100644
@@ -943,36 +943,6 @@ struct qla_tgt_sess {
 	qlt_plogi_ack_t *plogi_link[QLT_PLOGI_LINK_MAX];
 };
 
-typedef enum {
-	/*
-	 * BIT_0 - Atio Arrival / schedule to work
-	 * BIT_1 - qlt_do_work
-	 * BIT_2 - qlt_do work failed
-	 * BIT_3 - xfer rdy/tcm_qla2xxx_write_pending
-	 * BIT_4 - read respond/tcm_qla2xx_queue_data_in
-	 * BIT_5 - status respond / tcm_qla2xx_queue_status
-	 * BIT_6 - tcm request to abort/Term exchange.
-	 *	pre_xmit_response->qlt_send_term_exchange
-	 * BIT_7 - SRR received (qlt_handle_srr->qlt_xmit_response)
-	 * BIT_8 - SRR received (qlt_handle_srr->qlt_rdy_to_xfer)
-	 * BIT_9 - SRR received (qla_handle_srr->qlt_send_term_exchange)
-	 * BIT_10 - Data in - hanlde_data->tcm_qla2xxx_handle_data
-
-	 * BIT_12 - good completion - qlt_ctio_do_completion -->free_cmd
-	 * BIT_13 - Bad completion -
-	 *	qlt_ctio_do_completion --> qlt_term_ctio_exchange
-	 * BIT_14 - Back end data received/sent.
-	 * BIT_15 - SRR prepare ctio
-	 * BIT_16 - complete free
-	 * BIT_17 - flush - qlt_abort_cmd_on_host_reset
-	 * BIT_18 - completion w/abort status
-	 * BIT_19 - completion w/unknown status
-	 * BIT_20 - tcm_qla2xxx_free_cmd
-	 */
-	CMD_FLAG_DATA_WORK = BIT_11,
-	CMD_FLAG_DATA_WORK_FREE = BIT_21,
-} cmd_flags_t;
-
 struct qla_tgt_cmd {
 	struct se_cmd se_cmd;
 	struct qla_tgt_sess *sess;

@@ -1018,7 +988,11 @@ struct qla_tgt_cmd {
 	uint64_t jiffies_at_alloc;
 	uint64_t jiffies_at_free;
 
-	cmd_flags_t cmd_flags;
+	unsigned status_queued:1;
+	unsigned cmd_freed:1;
+	unsigned complete_free:1;
+	unsigned data_work:1;
+	unsigned data_work_free:1;
 };
 
 struct qla_tgt_sess_work_param {
drivers/scsi/qla2xxx/tcm_qla2xxx.c +16 -17
index 97fcbf2..36500b3 100644
@@ -282,10 +282,10 @@ static void tcm_qla2xxx_complete_free(struct work_struct *work)
 
 	cmd->cmd_in_wq = 0;
 
-	WARN_ON(cmd->cmd_flags &  BIT_16);
+	WARN_ON(cmd->complete_free);
 
 	cmd->vha->tgt_counters.qla_core_ret_sta_ctio++;
-	cmd->cmd_flags |= BIT_16;
+	cmd->complete_free = 1;
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 

@@ -299,8 +299,8 @@ static void tcm_qla2xxx_free_cmd(struct qla_tgt_cmd *cmd)
 	cmd->vha->tgt_counters.core_qla_free_cmd++;
 	cmd->cmd_in_wq = 1;
 
-	BUG_ON(cmd->cmd_flags & BIT_20);
-	cmd->cmd_flags |= BIT_20;
+	BUG_ON(cmd->cmd_freed);
+	cmd->cmd_freed = 1;
 
 	INIT_WORK(&cmd->work, tcm_qla2xxx_complete_free);
 	queue_work_on(smp_processor_id(), tcm_qla2xxx_free_wq, &cmd->work);

@@ -385,7 +385,6 @@ static int tcm_qla2xxx_write_pending(struct se_cmd *se_cmd)
 			cmd->se_cmd.se_cmd_flags);
 		return 0;
 	}
-	cmd->cmd_flags |= BIT_3;
 	cmd->bufflen = se_cmd->data_length;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
 

@@ -488,9 +487,9 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
 	cmd->cmd_in_wq = 0;
 
 	spin_lock_irqsave(&cmd->cmd_lock, flags);
-	cmd->cmd_flags |= CMD_FLAG_DATA_WORK;
+	cmd->data_work = 1;
 	if (cmd->aborted) {
-		cmd->cmd_flags |= CMD_FLAG_DATA_WORK_FREE;
+		cmd->data_work_free = 1;
 		spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 
 		tcm_qla2xxx_free_cmd(cmd);

@@ -527,7 +526,6 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
  */
 static void tcm_qla2xxx_handle_data(struct qla_tgt_cmd *cmd)
 {
-	cmd->cmd_flags |= BIT_10;
 	cmd->cmd_in_wq = 1;
 	INIT_WORK(&cmd->work, tcm_qla2xxx_handle_data_work);
 	queue_work_on(smp_processor_id(), tcm_qla2xxx_free_wq, &cmd->work);

@@ -586,7 +584,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
 		return 0;
 	}
 
-	cmd->cmd_flags |= BIT_4;
 	cmd->bufflen = se_cmd->data_length;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
 

@@ -617,11 +614,11 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 	cmd->sg_cnt = 0;
 	cmd->offset = 0;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
-	if (cmd->cmd_flags &  BIT_5) {
-		pr_crit("Bit_5 already set for cmd = %p.\n", cmd);
+	if (cmd->status_queued) {
+		pr_crit("Status already queued for cmd = %p.\n", cmd);
 		dump_stack();
 	}
-	cmd->cmd_flags |= BIT_5;
+	cmd->status_queued = 1;
 
 	if (se_cmd->data_direction == DMA_FROM_DEVICE) {
 		/*

@@ -678,9 +675,11 @@ static void tcm_qla2xxx_queue_tm_rsp(struct se_cmd *se_cmd)
 }
 
 
-#define DATA_WORK_NOT_FREE(_flags) \
-	(( _flags & (CMD_FLAG_DATA_WORK|CMD_FLAG_DATA_WORK_FREE)) == \
-	 CMD_FLAG_DATA_WORK)
+static inline bool DATA_WORK_NOT_FREE(struct qla_tgt_cmd *cmd)
+{
+	return cmd->data_work && !cmd->data_work_free;
+}
+
 static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,

@@ -693,9 +692,9 @@ static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
 	spin_lock_irqsave(&cmd->cmd_lock, flags);
 	if ((cmd->state == QLA_TGT_STATE_NEW)||
 		((cmd->state == QLA_TGT_STATE_DATA_IN) &&
-		 DATA_WORK_NOT_FREE(cmd->cmd_flags)) ) {
+		 DATA_WORK_NOT_FREE(cmd))) {
 
-		cmd->cmd_flags |= CMD_FLAG_DATA_WORK_FREE;
+		cmd->data_work_free = 1;
 		spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 		/* Cmd have not reached firmware.
 		 * Use this trigger to free it. */
Signature · 1 line
2.7.3
#10 lore raw

Re: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

Quoted 61 lines · Bart Van Assche

Use a C bitfield instead of emulating this functionality with bit manipulations. Since only bits 5, 16 and 20 are ever tested, remove the code that sets the other bits. This is easy to verify by running the following command:

$ git grep -nH 'BIT_[0-9]' drivers/scsi/qla2xxx | grep cmd_flags
Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 drivers/scsi/qla2xxx/qla_target.c  | 23 +++++------------------
 drivers/scsi/qla2xxx/qla_target.h  | 36 
+++++-------------------------------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 33 
++++++++++++++++-----------------
 3 files changed, 26 insertions(+), 66 deletions(-)
drivers/scsi/qla2xxx/qla_target.c +0 -2
index 8a44d15..8b8622d 100644
@@ -3291,7 +3291,6 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd)
 		return EIO;
 	}
 	cmd->aborted = 1;
-	cmd->cmd_flags |= BIT_6;
 	spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 
 	qlt_send_term_exchange(vha, cmd, &cmd->atio, 0, 1);

@@ -3339,7 +3338,6 @@ static int qlt_prepare_srr_ctio(struct 
 scsi_qla_host *vha,
 	struct qla_tgt_srr_imm *imm;
 
 	tgt->ctio_srr_id++;
-	cmd->cmd_flags |= BIT_15;
 
 	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf019,
drivers/scsi/qla2xxx/qla_target.c +0 -2
 	    "qla_target(%d): CTIO with SRR status received\n", vha->vp_idx);

@@ -3522,7 +3520,6 @@ qlt_abort_cmd_on_host_reset(struct scsi_qla_host
 *vha, struct qla_tgt_cmd *cmd)
 		dump_stack();
 	}
 
-	cmd->cmd_flags |= BIT_17;
 	ha->tgt.tgt_ops->free_cmd(cmd);
 }

@@ -3688,7 +3685,6 @@ static void qlt_do_ctio_completion(struct
 scsi_qla_host *vha, uint32_t handle,
 		 */
 		if ((cmd->state != QLA_TGT_STATE_NEED_DATA) &&
 		    (!cmd->aborted)) {
-			cmd->cmd_flags |= BIT_13;
 			if (qlt_term_ctio_exchange(vha, ctio, cmd, status))
 				return;
drivers/scsi/qla2xxx/qla_target.c +0 -0
 		}

drivers/scsi/qla2xxx/qla_target.c static void qlt_do_ctio_completion(struct · line 3698

Quoted 8 lines
drivers/scsi/qla2xxx/qla_target.c +1 -1
@@ -3696,7 +3692,7 @@ static void qlt_do_ctio_completion(struct
 scsi_qla_host *vha, uint32_t handle,
 skip_term:
 
 	if (cmd->state == QLA_TGT_STATE_PROCESSED) {
-		cmd->cmd_flags |= BIT_12;
+		;
 	} else if (cmd->state == QLA_TGT_STATE_NEED_DATA) {
 		cmd->state = QLA_TGT_STATE_DATA_IN;

What about:

if (cmd->state == QLA_TGT_STATE_NEED_DATA) {
        cmd->state = QLA_TGT_STATE_DATA_IN;

[...]

I.e. kick that nop
if (cmd->state == QLA_TGT_STATE_NEED_DATA)

      ;
Quoted 230 lines
drivers/scsi/qla2xxx/qla_target.c +2 -5
@@ -3706,11 +3702,9 @@ skip_term:
 		ha->tgt.tgt_ops->handle_data(cmd);
 		return;
 	} else if (cmd->aborted) {
-		cmd->cmd_flags |= BIT_18;
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf01e,
 		  "Aborted command %p (tag %lld) finished\n", cmd, se_cmd->tag);
 	} else {
-		cmd->cmd_flags |= BIT_19;
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf05c,
 		    "qla_target(%d): A command in state (%d) should "
 		    "not return a CTIO complete\n", vha->vp_idx, cmd->state);

@@ -3775,7 +3769,6 @@ static void __qlt_do_work(struct qla_tgt_cmd 
 *cmd)
 	int ret, fcp_task_attr, data_dir, bidi = 0;
 
 	cmd->cmd_in_wq = 0;
-	cmd->cmd_flags |= BIT_1;
 	if (tgt->tgt_stop)
 		goto out_term;

@@ -3827,7 +3820,6 @@ out_term:
 	 * cmd has not sent to target yet, so pass NULL as the second
 	 * argument to qlt_send_term_exchange() and free the memory here.
 	 */
-	cmd->cmd_flags |= BIT_2;
 	spin_lock_irqsave(&ha->hardware_lock, flags);
 	qlt_send_term_exchange(vha, NULL, &cmd->atio, 1, 0);
 

@@ -3878,7 +3870,8 @@ static struct qla_tgt_cmd
 *qlt_get_tag(scsi_qla_host_t *vha,
 	cmd->loop_id = sess->loop_id;
 	cmd->conf_compl_supported = sess->conf_compl_supported;
 
-	cmd->cmd_flags = 0;
+	cmd->status_queued = cmd->cmd_freed = cmd->complete_free = 0;
+	cmd->data_work = cmd->data_work_free = 0;
 	cmd->jiffies_at_alloc = get_jiffies_64();
drivers/scsi/qla2xxx/qla_target.c +1 -4
 	cmd->reset_count = vha->hw->chip_reset;

@@ -4014,7 +4007,6 @@ static int qlt_handle_cmd_for_atio(struct
 scsi_qla_host *vha,
 	}
 
 	cmd->cmd_in_wq = 1;
-	cmd->cmd_flags |= BIT_0;
 	cmd->se_cmd.cpuid = ha->msix_count ?
 		ha->tgt.rspq_vector_cpuid : WORK_CPU_UNBOUND;

@@ -4767,10 +4759,8 @@ static void qlt_handle_srr(struct scsi_qla_host 
 *vha,
 			qlt_send_notify_ack(vha, ntfy,
 			    0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
-			if (xmit_type & QLA_TGT_XMIT_DATA) {
-				cmd->cmd_flags |= BIT_8;
+			if (xmit_type & QLA_TGT_XMIT_DATA)
 				qlt_rdy_to_xfer(cmd);
-			}
 		} else {
 			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf066,

"qla_target(%d): SRR for out data for cmd without them (tag
%lld, SCSI status %d), reject",

drivers/scsi/qla2xxx/qla_target.c +1 -4
@@ -4786,10 +4776,8 @@ static void qlt_handle_srr(struct scsi_qla_host 
 *vha,
 	}
 
 	/* Transmit response in case of status and data-in cases */
-	if (resp) {
-		cmd->cmd_flags |= BIT_7;
+	if (resp)
 		qlt_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
-	}
 
 	return;

@@ -4803,7 +4791,6 @@ out_reject:
 		cmd->state = QLA_TGT_STATE_DATA_IN;
 		dump_stack();
 	} else {
-		cmd->cmd_flags |= BIT_9;
 		qlt_send_term_exchange(vha, cmd, &cmd->atio, 1, 0);
 	}
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
drivers/scsi/qla2xxx/qla_target.h +5 -31
index d857fee..9cf2aef 100644
@@ -943,36 +943,6 @@ struct qla_tgt_sess {
 	qlt_plogi_ack_t *plogi_link[QLT_PLOGI_LINK_MAX];
 };
 
-typedef enum {
-	/*
-	 * BIT_0 - Atio Arrival / schedule to work
-	 * BIT_1 - qlt_do_work
-	 * BIT_2 - qlt_do work failed
-	 * BIT_3 - xfer rdy/tcm_qla2xxx_write_pending
-	 * BIT_4 - read respond/tcm_qla2xx_queue_data_in
-	 * BIT_5 - status respond / tcm_qla2xx_queue_status
-	 * BIT_6 - tcm request to abort/Term exchange.
-	 *	pre_xmit_response->qlt_send_term_exchange
-	 * BIT_7 - SRR received (qlt_handle_srr->qlt_xmit_response)
-	 * BIT_8 - SRR received (qlt_handle_srr->qlt_rdy_to_xfer)
-	 * BIT_9 - SRR received (qla_handle_srr->qlt_send_term_exchange)
-	 * BIT_10 - Data in - hanlde_data->tcm_qla2xxx_handle_data
-
-	 * BIT_12 - good completion - qlt_ctio_do_completion -->free_cmd
-	 * BIT_13 - Bad completion -
-	 *	qlt_ctio_do_completion --> qlt_term_ctio_exchange
-	 * BIT_14 - Back end data received/sent.
-	 * BIT_15 - SRR prepare ctio
-	 * BIT_16 - complete free
-	 * BIT_17 - flush - qlt_abort_cmd_on_host_reset
-	 * BIT_18 - completion w/abort status
-	 * BIT_19 - completion w/unknown status
-	 * BIT_20 - tcm_qla2xxx_free_cmd
-	 */
-	CMD_FLAG_DATA_WORK = BIT_11,
-	CMD_FLAG_DATA_WORK_FREE = BIT_21,
-} cmd_flags_t;
-
 struct qla_tgt_cmd {
 	struct se_cmd se_cmd;
 	struct qla_tgt_sess *sess;

@@ -1018,7 +988,11 @@ struct qla_tgt_cmd {
 	uint64_t jiffies_at_alloc;
 	uint64_t jiffies_at_free;
 
-	cmd_flags_t cmd_flags;
+	unsigned status_queued:1;
+	unsigned cmd_freed:1;
+	unsigned complete_free:1;
+	unsigned data_work:1;
+	unsigned data_work_free:1;
 };
 
 struct qla_tgt_sess_work_param {
drivers/scsi/qla2xxx/tcm_qla2xxx.c +4 -4
index 97fcbf2..36500b3 100644
@@ -282,10 +282,10 @@ static void tcm_qla2xxx_complete_free(struct
 work_struct *work)
 
 	cmd->cmd_in_wq = 0;
 
-	WARN_ON(cmd->cmd_flags &  BIT_16);
+	WARN_ON(cmd->complete_free);
 
 	cmd->vha->tgt_counters.qla_core_ret_sta_ctio++;
-	cmd->cmd_flags |= BIT_16;
+	cmd->complete_free = 1;
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }

@@ -299,8 +299,8 @@ static void tcm_qla2xxx_free_cmd(struct qla_tgt_cmd 
 *cmd)
 	cmd->vha->tgt_counters.core_qla_free_cmd++;
 	cmd->cmd_in_wq = 1;
 
-	BUG_ON(cmd->cmd_flags & BIT_20);
-	cmd->cmd_flags |= BIT_20;
+	BUG_ON(cmd->cmd_freed);
+	cmd->cmd_freed = 1;
 
 	INIT_WORK(&cmd->work, tcm_qla2xxx_complete_free);
drivers/scsi/qla2xxx/tcm_qla2xxx.c +2 -3
 	queue_work_on(smp_processor_id(), tcm_qla2xxx_free_wq, &cmd->work);

@@ -385,7 +385,6 @@ static int tcm_qla2xxx_write_pending(struct se_cmd 
 *se_cmd)
 			cmd->se_cmd.se_cmd_flags);
 		return 0;
 	}
-	cmd->cmd_flags |= BIT_3;
 	cmd->bufflen = se_cmd->data_length;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);

@@ -488,9 +487,9 @@ static void tcm_qla2xxx_handle_data_work(struct
 work_struct *work)
 	cmd->cmd_in_wq = 0;
 
 	spin_lock_irqsave(&cmd->cmd_lock, flags);
-	cmd->cmd_flags |= CMD_FLAG_DATA_WORK;
+	cmd->data_work = 1;
 	if (cmd->aborted) {
-		cmd->cmd_flags |= CMD_FLAG_DATA_WORK_FREE;
+		cmd->data_work_free = 1;
 		spin_unlock_irqrestore(&cmd->cmd_lock, flags);
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -1
 		tcm_qla2xxx_free_cmd(cmd);

@@ -527,7 +526,6 @@ static void tcm_qla2xxx_handle_data_work(struct
 work_struct *work)
  */
 static void tcm_qla2xxx_handle_data(struct qla_tgt_cmd *cmd)
 {
-	cmd->cmd_flags |= BIT_10;
 	cmd->cmd_in_wq = 1;
 	INIT_WORK(&cmd->work, tcm_qla2xxx_handle_data_work);
drivers/scsi/qla2xxx/tcm_qla2xxx.c +3 -4
 	queue_work_on(smp_processor_id(), tcm_qla2xxx_free_wq, &cmd->work);

@@ -586,7 +584,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd 
 *se_cmd)
 		return 0;
 	}
 
-	cmd->cmd_flags |= BIT_4;
 	cmd->bufflen = se_cmd->data_length;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);

@@ -617,11 +614,11 @@ static int tcm_qla2xxx_queue_status(struct se_cmd 
 *se_cmd)
 	cmd->sg_cnt = 0;
 	cmd->offset = 0;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
-	if (cmd->cmd_flags &  BIT_5) {
-		pr_crit("Bit_5 already set for cmd = %p.\n", cmd);
+	if (cmd->status_queued) {
+		pr_crit("Status already queued for cmd = %p.\n", cmd);
 		dump_stack();
 	}
-	cmd->cmd_flags |= BIT_5;
+	cmd->status_queued = 1;
 
 	if (se_cmd->data_direction == DMA_FROM_DEVICE) {
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -0
 		/*

@@ -678,9 +675,11 @@ static void tcm_qla2xxx_queue_tm_rsp(struct se_cmd 
 *se_cmd)
 }

drivers/scsi/qla2xxx/tcm_qla2xxx.c static void tcm_qla2xxx_queue_tm_rsp(struct se_cmd · line 683

Quoted 10 lines
drivers/scsi/qla2xxx/tcm_qla2xxx.c +5 -3
 
 
-#define DATA_WORK_NOT_FREE(_flags) \
-	(( _flags & (CMD_FLAG_DATA_WORK|CMD_FLAG_DATA_WORK_FREE)) == \
-	 CMD_FLAG_DATA_WORK)
+static inline bool DATA_WORK_NOT_FREE(struct qla_tgt_cmd *cmd)
+{
+	return cmd->data_work && !cmd->data_work_free;
+}
+

Can you make that lower case?

Quoted 16 lines
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -0
 static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
 {
drivers/scsi/qla2xxx/tcm_qla2xxx.c +2 -2
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,

@@ -693,9 +692,9 @@ static void tcm_qla2xxx_aborted_task(struct se_cmd 
 *se_cmd)
 	spin_lock_irqsave(&cmd->cmd_lock, flags);
 	if ((cmd->state == QLA_TGT_STATE_NEW)||
 		((cmd->state == QLA_TGT_STATE_DATA_IN) &&
-		 DATA_WORK_NOT_FREE(cmd->cmd_flags)) ) {
+		 DATA_WORK_NOT_FREE(cmd))) {
 
-		cmd->cmd_flags |= CMD_FLAG_DATA_WORK_FREE;
+		cmd->data_work_free = 1;
 		spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 		/* Cmd have not reached firmware.

* Use this trigger to free it. */

#11 lore raw

Re: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

In reply to Bart Van Assche

Bart,

We use these flags for debugging purpose in crash cases.

Please drop the patch. Thanks.

Regards,
Quinn Tran

-----Original Message-----
From: Bart Van Assche <[email protected]>
Date: Wednesday, March 30, 2016 at 4:26 PM
To: James Bottomley <[email protected]>, "Martin K. Petersen" <[email protected]>
Cc: Himanshu Madhani <[email protected]>, Quinn Tran <[email protected]>, Christoph Hellwig <[email protected]>, linux-scsi <[email protected]>
Subject: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

Quoted 304 lines

Use a C bitfield instead of emulating this functionality with bit manipulations. Since only bits 5, 16 and 20 are ever tested, remove the code that sets the other bits. This is easy to verify by running the following command:

$ git grep -nH 'BIT_[0-9]' drivers/scsi/qla2xxx | grep cmd_flags
Signed-off-by Bart Van Assche <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>

---

drivers/scsi/qla2xxx/qla_target.c  | 23 +++++------------------
drivers/scsi/qla2xxx/qla_target.h  | 36 +++++-------------------------------
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 33 ++++++++++++++++-----------------
3 files changed, 26 insertions(+), 66 deletions(-)
drivers/scsi/qla2xxx/qla_target.c +0 -4
index 8a44d15..8b8622d 100644
@@ -3291,7 +3291,6 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd)
 		return EIO;
 	}
 	cmd->aborted = 1;
-	cmd->cmd_flags |= BIT_6;
 	spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 
 	qlt_send_term_exchange(vha, cmd, &cmd->atio, 0, 1);

@@ -3339,7 +3338,6 @@ static int qlt_prepare_srr_ctio(struct scsi_qla_host *vha,
 	struct qla_tgt_srr_imm *imm;
 
 	tgt->ctio_srr_id++;
-	cmd->cmd_flags |= BIT_15;
 
 	ql_dbg(ql_dbg_tgt_mgt, vha, 0xf019,
 	    "qla_target(%d): CTIO with SRR status received\n", vha->vp_idx);

@@ -3522,7 +3520,6 @@ qlt_abort_cmd_on_host_reset(struct scsi_qla_host *vha, struct qla_tgt_cmd *cmd)
 		dump_stack();
 	}
 
-	cmd->cmd_flags |= BIT_17;
 	ha->tgt.tgt_ops->free_cmd(cmd);
 }
 

@@ -3688,7 +3685,6 @@ static void qlt_do_ctio_completion(struct scsi_qla_host *vha, uint32_t handle,
 		 */
 		if ((cmd->state != QLA_TGT_STATE_NEED_DATA) &&
 		    (!cmd->aborted)) {
-			cmd->cmd_flags |= BIT_13;
 			if (qlt_term_ctio_exchange(vha, ctio, cmd, status))
 				return;
 		}

@@ -3696,7 +3692,7 @@ static void qlt_do_ctio_completion(struct scsi_qla_host *vha, uint32_t handle,

skip_term:

	if (cmd->state == QLA_TGT_STATE_PROCESSED) {
drivers/scsi/qla2xxx/qla_target.c +1 -1
-		cmd->cmd_flags |= BIT_12;
+		;

	} else if (cmd->state == QLA_TGT_STATE_NEED_DATA) {
		cmd->state = QLA_TGT_STATE_DATA_IN;
drivers/scsi/qla2xxx/qla_target.c +4 -13
@@ -3706,11 +3702,9 @@ skip_term:
 		ha->tgt.tgt_ops->handle_data(cmd);
 		return;
 	} else if (cmd->aborted) {
-		cmd->cmd_flags |= BIT_18;
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf01e,
 		  "Aborted command %p (tag %lld) finished\n", cmd, se_cmd->tag);
 	} else {
-		cmd->cmd_flags |= BIT_19;
 		ql_dbg(ql_dbg_tgt_mgt, vha, 0xf05c,
 		    "qla_target(%d): A command in state (%d) should "
 		    "not return a CTIO complete\n", vha->vp_idx, cmd->state);

@@ -3775,7 +3769,6 @@ static void __qlt_do_work(struct qla_tgt_cmd *cmd)
 	int ret, fcp_task_attr, data_dir, bidi = 0;
 
 	cmd->cmd_in_wq = 0;
-	cmd->cmd_flags |= BIT_1;
 	if (tgt->tgt_stop)
 		goto out_term;
 

@@ -3827,7 +3820,6 @@ out_term:
 	 * cmd has not sent to target yet, so pass NULL as the second
 	 * argument to qlt_send_term_exchange() and free the memory here.
 	 */
-	cmd->cmd_flags |= BIT_2;
 	spin_lock_irqsave(&ha->hardware_lock, flags);
 	qlt_send_term_exchange(vha, NULL, &cmd->atio, 1, 0);
 

@@ -3878,7 +3870,8 @@ static struct qla_tgt_cmd *qlt_get_tag(scsi_qla_host_t *vha,
 	cmd->loop_id = sess->loop_id;
 	cmd->conf_compl_supported = sess->conf_compl_supported;
 
-	cmd->cmd_flags = 0;
+	cmd->status_queued = cmd->cmd_freed = cmd->complete_free = 0;
+	cmd->data_work = cmd->data_work_free = 0;
 	cmd->jiffies_at_alloc = get_jiffies_64();
 
 	cmd->reset_count = vha->hw->chip_reset;

@@ -4014,7 +4007,6 @@ static int qlt_handle_cmd_for_atio(struct scsi_qla_host *vha,
 	}
 
 	cmd->cmd_in_wq = 1;
-	cmd->cmd_flags |= BIT_0;
 	cmd->se_cmd.cpuid = ha->msix_count ?
 		ha->tgt.rspq_vector_cpuid : WORK_CPU_UNBOUND;
 

@@ -4767,10 +4759,8 @@ static void qlt_handle_srr(struct scsi_qla_host *vha,
 			qlt_send_notify_ack(vha, ntfy,
 			    0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
-			if (xmit_type & QLA_TGT_XMIT_DATA) {
-				cmd->cmd_flags |= BIT_8;
+			if (xmit_type & QLA_TGT_XMIT_DATA)
 				qlt_rdy_to_xfer(cmd);
-			}
 		} else {
 			ql_dbg(ql_dbg_tgt_mgt, vha, 0xf066,
 			    "qla_target(%d): SRR for out data for cmd without them (tag %lld, SCSI status %d), reject",

@@ -4786,10 +4776,8 @@ static void qlt_handle_srr(struct scsi_qla_host *vha,
 	}
 
 	/* Transmit response in case of status and data-in cases */
-	if (resp) {
-		cmd->cmd_flags |= BIT_7;
+	if (resp)
 		qlt_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
-	}
 
 	return;
 

@@ -4803,7 +4791,6 @@ out_reject:
 		cmd->state = QLA_TGT_STATE_DATA_IN;
 		dump_stack();
 	} else {
-		cmd->cmd_flags |= BIT_9;
 		qlt_send_term_exchange(vha, cmd, &cmd->atio, 1, 0);
 	}
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
drivers/scsi/qla2xxx/qla_target.h +5 -31
index d857fee..9cf2aef 100644
@@ -943,36 +943,6 @@ struct qla_tgt_sess {
 	qlt_plogi_ack_t *plogi_link[QLT_PLOGI_LINK_MAX];
 };
 
-typedef enum {
-	/*
-	 * BIT_0 - Atio Arrival / schedule to work
-	 * BIT_1 - qlt_do_work
-	 * BIT_2 - qlt_do work failed
-	 * BIT_3 - xfer rdy/tcm_qla2xxx_write_pending
-	 * BIT_4 - read respond/tcm_qla2xx_queue_data_in
-	 * BIT_5 - status respond / tcm_qla2xx_queue_status
-	 * BIT_6 - tcm request to abort/Term exchange.
-	 *	pre_xmit_response->qlt_send_term_exchange
-	 * BIT_7 - SRR received (qlt_handle_srr->qlt_xmit_response)
-	 * BIT_8 - SRR received (qlt_handle_srr->qlt_rdy_to_xfer)
-	 * BIT_9 - SRR received (qla_handle_srr->qlt_send_term_exchange)
-	 * BIT_10 - Data in - hanlde_data->tcm_qla2xxx_handle_data
-
-	 * BIT_12 - good completion - qlt_ctio_do_completion -->free_cmd
-	 * BIT_13 - Bad completion -
-	 *	qlt_ctio_do_completion --> qlt_term_ctio_exchange
-	 * BIT_14 - Back end data received/sent.
-	 * BIT_15 - SRR prepare ctio
-	 * BIT_16 - complete free
-	 * BIT_17 - flush - qlt_abort_cmd_on_host_reset
-	 * BIT_18 - completion w/abort status
-	 * BIT_19 - completion w/unknown status
-	 * BIT_20 - tcm_qla2xxx_free_cmd
-	 */
-	CMD_FLAG_DATA_WORK = BIT_11,
-	CMD_FLAG_DATA_WORK_FREE = BIT_21,
-} cmd_flags_t;
-
 struct qla_tgt_cmd {
 	struct se_cmd se_cmd;
 	struct qla_tgt_sess *sess;

@@ -1018,7 +988,11 @@ struct qla_tgt_cmd {
 	uint64_t jiffies_at_alloc;
 	uint64_t jiffies_at_free;
 
-	cmd_flags_t cmd_flags;
+	unsigned status_queued:1;
+	unsigned cmd_freed:1;
+	unsigned complete_free:1;
+	unsigned data_work:1;
+	unsigned data_work_free:1;
 };
 
 struct qla_tgt_sess_work_param {
drivers/scsi/qla2xxx/tcm_qla2xxx.c +16 -17
index 97fcbf2..36500b3 100644
@@ -282,10 +282,10 @@ static void tcm_qla2xxx_complete_free(struct work_struct *work)
 
 	cmd->cmd_in_wq = 0;
 
-	WARN_ON(cmd->cmd_flags &  BIT_16);
+	WARN_ON(cmd->complete_free);
 
 	cmd->vha->tgt_counters.qla_core_ret_sta_ctio++;
-	cmd->cmd_flags |= BIT_16;
+	cmd->complete_free = 1;
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 

@@ -299,8 +299,8 @@ static void tcm_qla2xxx_free_cmd(struct qla_tgt_cmd *cmd)
 	cmd->vha->tgt_counters.core_qla_free_cmd++;
 	cmd->cmd_in_wq = 1;
 
-	BUG_ON(cmd->cmd_flags & BIT_20);
-	cmd->cmd_flags |= BIT_20;
+	BUG_ON(cmd->cmd_freed);
+	cmd->cmd_freed = 1;
 
 	INIT_WORK(&cmd->work, tcm_qla2xxx_complete_free);
 	queue_work_on(smp_processor_id(), tcm_qla2xxx_free_wq, &cmd->work);

@@ -385,7 +385,6 @@ static int tcm_qla2xxx_write_pending(struct se_cmd *se_cmd)
 			cmd->se_cmd.se_cmd_flags);
 		return 0;
 	}
-	cmd->cmd_flags |= BIT_3;
 	cmd->bufflen = se_cmd->data_length;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
 

@@ -488,9 +487,9 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
 	cmd->cmd_in_wq = 0;
 
 	spin_lock_irqsave(&cmd->cmd_lock, flags);
-	cmd->cmd_flags |= CMD_FLAG_DATA_WORK;
+	cmd->data_work = 1;
 	if (cmd->aborted) {
-		cmd->cmd_flags |= CMD_FLAG_DATA_WORK_FREE;
+		cmd->data_work_free = 1;
 		spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 
 		tcm_qla2xxx_free_cmd(cmd);

@@ -527,7 +526,6 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
 */
 static void tcm_qla2xxx_handle_data(struct qla_tgt_cmd *cmd)
 {
-	cmd->cmd_flags |= BIT_10;
 	cmd->cmd_in_wq = 1;
 	INIT_WORK(&cmd->work, tcm_qla2xxx_handle_data_work);
 	queue_work_on(smp_processor_id(), tcm_qla2xxx_free_wq, &cmd->work);

@@ -586,7 +584,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
 		return 0;
 	}
 
-	cmd->cmd_flags |= BIT_4;
 	cmd->bufflen = se_cmd->data_length;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
 

@@ -617,11 +614,11 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 	cmd->sg_cnt = 0;
 	cmd->offset = 0;
 	cmd->dma_data_direction = target_reverse_dma_direction(se_cmd);
-	if (cmd->cmd_flags &  BIT_5) {
-		pr_crit("Bit_5 already set for cmd = %p.\n", cmd);
+	if (cmd->status_queued) {
+		pr_crit("Status already queued for cmd = %p.\n", cmd);
 		dump_stack();
 	}
-	cmd->cmd_flags |= BIT_5;
+	cmd->status_queued = 1;
 
 	if (se_cmd->data_direction == DMA_FROM_DEVICE) {
 		/*

@@ -678,9 +675,11 @@ static void tcm_qla2xxx_queue_tm_rsp(struct se_cmd *se_cmd)
 }
 
 
-#define DATA_WORK_NOT_FREE(_flags) \
-	(( _flags & (CMD_FLAG_DATA_WORK|CMD_FLAG_DATA_WORK_FREE)) == \
-	 CMD_FLAG_DATA_WORK)
+static inline bool DATA_WORK_NOT_FREE(struct qla_tgt_cmd *cmd)
+{
+	return cmd->data_work && !cmd->data_work_free;
+}
+
 static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,

@@ -693,9 +692,9 @@ static void tcm_qla2xxx_aborted_task(struct se_cmd *se_cmd)
 	spin_lock_irqsave(&cmd->cmd_lock, flags);
 	if ((cmd->state == QLA_TGT_STATE_NEW)||
 		((cmd->state == QLA_TGT_STATE_DATA_IN) &&
-		 DATA_WORK_NOT_FREE(cmd->cmd_flags)) ) {
+		 DATA_WORK_NOT_FREE(cmd))) {
 
-		cmd->cmd_flags |= CMD_FLAG_DATA_WORK_FREE;
+		cmd->data_work_free = 1;
 		spin_unlock_irqrestore(&cmd->cmd_lock, flags);
 		/* Cmd have not reached firmware.
 		 * Use this trigger to free it. */
Signature · 1 line
2.7.3
author
#12 lore raw

Re: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

Quoted 1 line · Quinn Tran

We use these flags for debugging purpose in crash cases.

Hello Quinn,

It must be inconvenient to have to look up the meaning of the individual bits of the cmd_flags variable when analyzing a kernel crash. How about preserving all these flags and to assign a meaningful name to each flag instead of using bit numbers?

Thanks,

Bart.

#13 lore raw

Re: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

Agreed. It’s one of our pending action items for cleanup.

Regards,
Quinn Tran

-----Original Message-----
From: Bart Van Assche <[email protected]>
Date: Thursday, March 31, 2016 at 8:22 AM
To: Quinn Tran <[email protected]>, James Bottomley <[email protected]>, "Martin K. Petersen" <[email protected]>
Cc: Himanshu Madhani <[email protected]>, Christoph Hellwig <[email protected]>, linux-scsi <[email protected]>
Subject: Re: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

Quoted 14 lines · Quinn Tran
Quoted 1 line

We use these flags for debugging purpose in crash cases.

Hello Quinn,

It must be inconvenient to have to look up the meaning of the individual bits of the cmd_flags variable when analyzing a kernel crash. How about preserving all these flags and to assign a meaningful name to each flag instead of using bit numbers?

Thanks,

Bart.

author
#14 lore raw

Re: [PATCH 3/3] qla2xxx: Assign names to the flags in cmd_flags

Quoted 1 line · Quinn Tran

Agreed. It’s one of our pending action items for cleanup.

Hello Quinn,

Thanks for the feedback. I will drop my patch.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html