Skip to contentSkip to conversation

qla2xxx patches for kernel v5.7

PATCH In review Bart Van Assche 4 participants 14 messages Feb 20, 2020 – Feb 29, 2020

Subsystems (2)
SCSI QLogic QLA2XXX FC-SCSI driver
lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

v3 cover qla2xxx patches for kernel v5.7

Hi Martin,

Please consider these five qla2xxx patches for kernel v5.7.

Thanks,

Bart.

Changes compared to v2:
- Left out a patch that is already in Linus' tree.

Changes compared to v1:
- Dropped patch "Fix point-to-point mode for qla25xx and older"
- Added three new patches.

Bart Van Assche (5):
qla2xxx: Simplify the code for aborting SCSI commands
qla2xxx: Suppress endianness complaints in

    qla2x00_configure_local_loop()

qla2xxx: Fix sparse warnings triggered by the PCI state checking code
qla2xxx: Convert MAKE_HANDLE() from a define into an inline function
qla2xxx: Fix the endianness annotations for the port attribute
max_frame_size member

 drivers/scsi/qla2xxx/qla_def.h    | 14 +++++++-------
 drivers/scsi/qla2xxx/qla_gs.c     | 14 ++++++--------
 drivers/scsi/qla2xxx/qla_init.c   | 11 +++++------
 drivers/scsi/qla2xxx/qla_iocb.c   | 22 +++++++++++-----------
 drivers/scsi/qla2xxx/qla_isr.c    |  5 -----
 drivers/scsi/qla2xxx/qla_mbx.c    | 15 +++++++--------
 drivers/scsi/qla2xxx/qla_mr.c     | 13 ++++++-------
 drivers/scsi/qla2xxx/qla_nvme.c   |  2 +-
 drivers/scsi/qla2xxx/qla_os.c     | 27 ++++++++++++++-------------
 drivers/scsi/qla2xxx/qla_target.c |  6 +++---
 10 files changed, 60 insertions(+), 69 deletions(-)
author
#1 lore raw

PATCH v3 1/5 qla2xxx: Simplify the code for aborting SCSI commands

Since the SCSI core does not reuse the tag of the SCSI command that is being aborted by .eh_abort() before .eh_abort() has finished it is not necessary to check from inside that callback whether or not the SCSI command has already completed. Instead, rely on the firmware to return an error code when attempting to abort a command that has already completed. Additionally, rely on the firmware to return an error code when attempting to abort an already aborted command.

In qla2x00_abort_srb(), use blk_mq_request_started() instead of sp->completed and sp->aborted.

Acked-by Himanshu Madhani <[email protected]>
Reviewed-by Roman Bolshakov <[email protected]>
Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 drivers/scsi/qla2xxx/qla_def.h |  3 ---
 drivers/scsi/qla2xxx/qla_isr.c |  5 -----
 drivers/scsi/qla2xxx/qla_os.c  | 27 ++++++++++++++-------------
 3 files changed, 14 insertions(+), 21 deletions(-)
drivers/scsi/qla2xxx/qla_def.h +0 -3
index ed32e9715794..c5a067f45005 100644
@@ -597,9 +597,6 @@ typedef struct srb {
 	struct fc_port *fcport;
 	struct scsi_qla_host *vha;
 	unsigned int start_timer:1;
-	unsigned int abort:1;
-	unsigned int aborted:1;
-	unsigned int completed:1;
 
 	uint32_t handle;
 	uint16_t flags;
drivers/scsi/qla2xxx/qla_isr.c +0 -5
index e40705d38cea..c7c86a432b55 100644
@@ -2516,11 +2516,6 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
 		return;
 	}
 
-	if (sp->abort)
-		sp->aborted = 1;
-	else
-		sp->completed = 1;
-
 	if (sp->cmd_type != TYPE_SRB) {
 		req->outstanding_cmds[handle] = NULL;
 		ql_dbg(ql_dbg_io, vha, 0x3015,
drivers/scsi/qla2xxx/qla_os.c +14 -13
index 79387ac8936f..a34f27b2d602 100644
@@ -1253,17 +1253,6 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
 		return SUCCESS;
 
 	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
-	if (sp->completed) {
-		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
-		return SUCCESS;
-	}
-
-	if (sp->abort || sp->aborted) {
-		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
-		return FAILED;
-	}
-
-	sp->abort = 1;
 	sp->comp = &comp;
 	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
 

@@ -1688,6 +1677,10 @@ qla2x00_loop_reset(scsi_qla_host_t *vha)
 	return QLA_SUCCESS;
 }
 
+/*
+ * The caller must ensure that no completion interrupts will happen
+ * while this function is in progress.
+ */
 static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
 			      unsigned long *flags)
 	__releases(qp->qp_lock_ptr)

@@ -1696,6 +1689,7 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
 	DECLARE_COMPLETION_ONSTACK(comp);
 	scsi_qla_host_t *vha = qp->vha;
 	struct qla_hw_data *ha = vha->hw;
+	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
 	int rval;
 	bool ret_cmd;
 	uint32_t ratov_j;

@@ -1717,7 +1711,6 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
 		}
 
 		sp->comp = &comp;
-		sp->abort =  1;
 		spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
 
 		rval = ha->isp_ops->abort_command(sp);

@@ -1741,13 +1734,17 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
 		}
 
 		spin_lock_irqsave(qp->qp_lock_ptr, *flags);
-		if (ret_cmd && (!sp->completed || !sp->aborted))
+		if (ret_cmd && blk_mq_request_started(cmd->request))
 			sp->done(sp, res);
 	} else {
 		sp->done(sp, res);
 	}
 }
 
+/*
+ * The caller must ensure that no completion interrupts will happen
+ * while this function is in progress.
+ */
 static void
 __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
 {

@@ -1794,6 +1791,10 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
 	spin_unlock_irqrestore(qp->qp_lock_ptr, flags);
 }
 
+/*
+ * The caller must ensure that no completion interrupts will happen
+ * while this function is in progress.
+ */
 void
 qla2x00_abort_all_cmds(scsi_qla_host_t *vha, int res)
 {
author
#2 lore raw

PATCH v3 2/5 qla2xxx: Suppress endianness complaints in qla2x00_configure_local_loop()

In reply to Bart Van Assche

Instead of changing endianness in-place, write the data in CPU endian format in another buffer and copy that buffer back. This patch does not change any functionality but silences some sparse endianness warnings.

Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 drivers/scsi/qla2xxx/qla_def.h  |  2 +-
 drivers/scsi/qla2xxx/qla_init.c | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)
drivers/scsi/qla2xxx/qla_def.h +1 -1
index c5a067f45005..b04d334933ef 100644
@@ -414,7 +414,7 @@ struct els_logo_payload {
 struct els_plogi_payload {
 	uint8_t opcode;
 	uint8_t rsvd[3];
-	uint8_t data[112];
+	__be32	data[112 / 4];
 };
 
 struct ct_arg {
drivers/scsi/qla2xxx/qla_init.c +5 -6
index 9e6b56527b25..880f2be062a9 100644
@@ -5060,7 +5060,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha)
 	if (N2N_TOPO(ha)) {
 		if (test_and_clear_bit(N2N_LOGIN_NEEDED, &vha->dpc_flags)) {
 			/* borrowing */
-			u32 *bp, i, sz;
+			u32 *bp, sz;
 
 			memset(ha->init_cb, 0, ha->init_cb_size);
 			sz = min_t(int, sizeof(struct els_plogi_payload),

@@ -5068,13 +5068,12 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha)
 			rval = qla24xx_get_port_login_templ(vha,
 			    ha->init_cb_dma, (void *)ha->init_cb, sz);
 			if (rval == QLA_SUCCESS) {
+				__be32 *q = &ha->plogi_els_payld.data[0];
+
 				bp = (uint32_t *)ha->init_cb;
-				for (i = 0; i < sz/4 ; i++, bp++)
-					*bp = cpu_to_be32(*bp);
+				cpu_to_be32_array(q, bp, sz / 4);
 
-				memcpy(&ha->plogi_els_payld.data,
-				    (void *)ha->init_cb,
-				    sizeof(ha->plogi_els_payld.data));
+				memcpy(bp, q, sizeof(ha->plogi_els_payld.data));
 			} else {
 				ql_dbg(ql_dbg_init, vha, 0x00d1,
 				    "PLOGI ELS param read fail.\n");
Reviewed-by on v3 2/5
#3 lore raw

Re: [PATCH v3 2/5] qla2xxx: Suppress endianness complaints in qla2x00_configure_local_loop()

Quoted 23 lines · Bart Van Assche

Instead of changing endianness in-place, write the data in CPU endian format in another buffer and copy that buffer back. This patch does not change any functionality but silences some sparse endianness warnings.

Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 drivers/scsi/qla2xxx/qla_def.h  |  2 +-
 drivers/scsi/qla2xxx/qla_init.c | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)
drivers/scsi/qla2xxx/qla_init.c +1 -0
index 9e6b56527b25..880f2be062a9 100644
@@ -5068,13 +5068,12 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha)
 			rval = qla24xx_get_port_login_templ(vha,
 			    ha->init_cb_dma, (void *)ha->init_cb, sz);
 			if (rval == QLA_SUCCESS) {
+				__be32 *q = &ha->plogi_els_payld.data[0];

drivers/scsi/qla2xxx/qla_init.c qla2x00_configure_local_loop(scsi_qla_host_t *vha) · line 5076

Quoted 10 lines
drivers/scsi/qla2xxx/qla_init.c +3 -5
+
 				bp = (uint32_t *)ha->init_cb;
-				for (i = 0; i < sz/4 ; i++, bp++)
-					*bp = cpu_to_be32(*bp);
+				cpu_to_be32_array(q, bp, sz / 4);
 
-				memcpy(&ha->plogi_els_payld.data,
-				    (void *)ha->init_cb,
-				    sizeof(ha->plogi_els_payld.data));
+				memcpy(bp, q, sizeof(ha->plogi_els_payld.data));

Hi Bart,

Honestly, I'd prefer to drop the memcpy as it has no purpose, but since you replicated a side-effect of the previous code and fixed sparse warnings,

Reviewed-by Roman Bolshakov <[email protected]>

Thank you,
Roman

P.S.

During the review, I looked again where init_cb is used. It seems the reuse and rewriting of init_cb contents by qla24xx_get_port_login_templ may corrupt init_cb values expected by qla2x00_fdmi_rpa and qla2x00_fdmiv2_rpa (they retrieve max frame size from Initialization Control Block), and qla2x00_async_event (recovers WWPN from init_cb in case of Fabric-assigned WWPN).

That _rpa functions are invoked if a port is in fabric topology and FDMI registration is enabled, so a change of topology from P2P to Switched Fabric may result in FDMI registration with wrong max frame size. I don't know how FC switches use the value though.

If the fabric assigns WWPNs (FA-WWPN) and a disconnect happens (Loop Down Event), the driver will try to recover WWPN from init_cb that is corrupted by qla24xx_get_port_login_templ.

Probably, the wrong state may be recovered by reclaiming the underlying PCI device. init_cb is set to correct values in probe() handler.

author
#4 lore raw

PATCH v3 3/5 qla2xxx: Fix sparse warnings triggered by the PCI state checking code

In reply to Bart Van Assche

This patch fixes the following sparse warnings:

drivers/scsi/qla2xxx/qla_mbx.c:120:21: warning: restricted pci_channel_state_t degrades to integer drivers/scsi/qla2xxx/qla_mbx.c:120:37: warning: restricted pci_channel_state_t degrades to integer

From include/linux/pci.h:

enum pci_channel_state {
	/* I/O channel is in normal state */
	pci_channel_io_normal = (__force pci_channel_state_t) 1,

	/* I/O to channel is blocked */
	pci_channel_io_frozen = (__force pci_channel_state_t) 2,

	/* PCI card is dead */
	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
};
Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 drivers/scsi/qla2xxx/qla_mbx.c | 5 ++---
 drivers/scsi/qla2xxx/qla_mr.c  | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)
drivers/scsi/qla2xxx/qla_mbx.c +2 -3
index 9e09964f5c0e..ff945d35ba8e 100644
@@ -117,10 +117,9 @@ qla2x00_mailbox_command(scsi_qla_host_t *vha, mbx_cmd_t *mcp)
 
 	ql_dbg(ql_dbg_mbx, vha, 0x1000, "Entered %s.\n", __func__);
 
-	if (ha->pdev->error_state > pci_channel_io_frozen) {
+	if (ha->pdev->error_state == pci_channel_io_perm_failure) {
 		ql_log(ql_log_warn, vha, 0x1001,
-		    "error_state is greater than pci_channel_io_frozen, "
-		    "exiting.\n");
+		    "PCI channel failed permanently, exiting.\n");
 		return QLA_FUNCTION_TIMEOUT;
 	}
drivers/scsi/qla2xxx/qla_mr.c +2 -3
index cad1fc2a1b28..6d120457478e 100644
@@ -53,10 +53,9 @@ qlafx00_mailbox_command(scsi_qla_host_t *vha, struct mbx_cmd_32 *mcp)
 	struct qla_hw_data *ha = vha->hw;
 	scsi_qla_host_t *base_vha = pci_get_drvdata(ha->pdev);
 
-	if (ha->pdev->error_state > pci_channel_io_frozen) {
+	if (ha->pdev->error_state == pci_channel_io_perm_failure) {
 		ql_log(ql_log_warn, vha, 0x115c,
-		    "error_state is greater than pci_channel_io_frozen, "
-		    "exiting.\n");
+		    "PCI channel failed permanently, exiting.\n");
 		return QLA_FUNCTION_TIMEOUT;
 	}
Reviewed-by on v3 3/5
#5 lore raw

Re: [PATCH v3 3/5] qla2xxx: Fix sparse warnings triggered by the PCI state checking code

Quoted 23 lines · Bart Van Assche

This patch fixes the following sparse warnings:

drivers/scsi/qla2xxx/qla_mbx.c:120:21: warning: restricted pci_channel_state_t degrades to integer drivers/scsi/qla2xxx/qla_mbx.c:120:37: warning: restricted pci_channel_state_t degrades to integer

From include/linux/pci.h:

enum pci_channel_state {
	/* I/O channel is in normal state */
	pci_channel_io_normal = (__force pci_channel_state_t) 1,

	/* I/O to channel is blocked */
	pci_channel_io_frozen = (__force pci_channel_state_t) 2,

	/* PCI card is dead */
	pci_channel_io_perm_failure = (__force pci_channel_state_t) 3,
};
Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
Quoted 5 lines
---
 drivers/scsi/qla2xxx/qla_mbx.c | 5 ++---
 drivers/scsi/qla2xxx/qla_mr.c  | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

I'm sorry, replied to wrong version in previous email,

Reviewed-by Roman Bolshakov <[email protected]>
author
#6 lore raw

PATCH v3 4/5 qla2xxx: Convert MAKE_HANDLE() from a define into an inline function

In reply to Bart Van Assche

This patch allows sparse to verify the endianness of the arguments passed to make_handle().

Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Daniel Wagner <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 drivers/scsi/qla2xxx/qla_def.h    |  5 ++++-
 drivers/scsi/qla2xxx/qla_iocb.c   | 22 +++++++++++-----------
 drivers/scsi/qla2xxx/qla_mbx.c    | 10 +++++-----
 drivers/scsi/qla2xxx/qla_mr.c     |  8 ++++----
 drivers/scsi/qla2xxx/qla_nvme.c   |  2 +-
 drivers/scsi/qla2xxx/qla_target.c |  6 +++---
 6 files changed, 28 insertions(+), 25 deletions(-)
drivers/scsi/qla2xxx/qla_def.h +4 -1
index b04d334933ef..cec0b5082927 100644
@@ -119,7 +119,10 @@ typedef struct {
 #define LSD(x)	((uint32_t)((uint64_t)(x)))
 #define MSD(x)	((uint32_t)((((uint64_t)(x)) >> 16) >> 16))
 
-#define MAKE_HANDLE(x, y) ((uint32_t)((((uint32_t)(x)) << 16) | (uint32_t)(y)))
+static inline uint32_t make_handle(uint16_t x, uint16_t y)
+{
+	return ((uint32_t)x << 16) | y;
+}
 
 /*
  * I/O register
drivers/scsi/qla2xxx/qla_iocb.c +11 -11
index 47bf60a9490a..1816660768da 100644
@@ -530,7 +530,7 @@ __qla2x00_marker(struct scsi_qla_host *vha, struct qla_qpair *qpair,
 			int_to_scsilun(lun, (struct scsi_lun *)&mrk24->lun);
 			host_to_fcp_swap(mrk24->lun, sizeof(mrk24->lun));
 			mrk24->vp_index = vha->vp_idx;
-			mrk24->handle = MAKE_HANDLE(req->id, mrk24->handle);
+			mrk24->handle = make_handle(req->id, mrk24->handle);
 		} else {
 			SET_TARGET_ID(ha, mrk->target, loop_id);
 			mrk->lun = cpu_to_le16((uint16_t)lun);

@@ -1655,7 +1655,7 @@ qla24xx_start_scsi(srb_t *sp)
 	req->cnt -= req_cnt;
 
 	cmd_pkt = (struct cmd_type_7 *)req->ring_ptr;
-	cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+	cmd_pkt->handle = make_handle(req->id, handle);
 
 	/* Zero out remaining portion of packet. */
 	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */

@@ -1843,7 +1843,7 @@ qla24xx_dif_start_scsi(srb_t *sp)
 
 	/* Fill-in common area */
 	cmd_pkt = (struct cmd_type_crc_2 *)req->ring_ptr;
-	cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+	cmd_pkt->handle = make_handle(req->id, handle);
 
 	clr_ptr = (uint32_t *)cmd_pkt + 2;
 	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);

@@ -1975,7 +1975,7 @@ qla2xxx_start_scsi_mq(srb_t *sp)
 	req->cnt -= req_cnt;
 
 	cmd_pkt = (struct cmd_type_7 *)req->ring_ptr;
-	cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+	cmd_pkt->handle = make_handle(req->id, handle);
 
 	/* Zero out remaining portion of packet. */
 	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */

@@ -2178,7 +2178,7 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
 
 	/* Fill-in common area */
 	cmd_pkt = (struct cmd_type_crc_2 *)req->ring_ptr;
-	cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+	cmd_pkt->handle = make_handle(req->id, handle);
 
 	clr_ptr = (uint32_t *)cmd_pkt + 2;
 	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);

@@ -2489,7 +2489,7 @@ qla24xx_tm_iocb(srb_t *sp, struct tsk_mgmt_entry *tsk)
 
 	tsk->entry_type = TSK_MGMT_IOCB_TYPE;
 	tsk->entry_count = 1;
-	tsk->handle = MAKE_HANDLE(req->id, tsk->handle);
+	tsk->handle = make_handle(req->id, tsk->handle);
 	tsk->nport_handle = cpu_to_le16(fcport->loop_id);
 	tsk->timeout = cpu_to_le16(ha->r_a_tov / 10 * 2);
 	tsk->control_flags = cpu_to_le32(flags);

@@ -3358,7 +3358,7 @@ qla82xx_start_scsi(srb_t *sp)
 		}
 
 		cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
-		cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+		cmd_pkt->handle = make_handle(req->id, handle);
 
 		/* Zero out remaining portion of packet. */
 		/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */

@@ -3429,7 +3429,7 @@ qla82xx_start_scsi(srb_t *sp)
 			goto queuing_error;
 
 		cmd_pkt = (struct cmd_type_7 *)req->ring_ptr;
-		cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+		cmd_pkt->handle = make_handle(req->id, handle);
 
 		/* Zero out remaining portion of packet. */
 		/* tagged queuing modifier -- default is TSK_SIMPLE (0).*/

@@ -3534,7 +3534,7 @@ qla24xx_abort_iocb(srb_t *sp, struct abort_entry_24xx *abt_iocb)
 	memset(abt_iocb, 0, sizeof(struct abort_entry_24xx));
 	abt_iocb->entry_type = ABORT_IOCB_TYPE;
 	abt_iocb->entry_count = 1;
-	abt_iocb->handle = cpu_to_le32(MAKE_HANDLE(req->id, sp->handle));
+	abt_iocb->handle = cpu_to_le32(make_handle(req->id, sp->handle));
 	if (sp->fcport) {
 		abt_iocb->nport_handle = cpu_to_le16(sp->fcport->loop_id);
 		abt_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;

@@ -3542,7 +3542,7 @@ qla24xx_abort_iocb(srb_t *sp, struct abort_entry_24xx *abt_iocb)
 		abt_iocb->port_id[2] = sp->fcport->d_id.b.domain;
 	}
 	abt_iocb->handle_to_abort =
-	    cpu_to_le32(MAKE_HANDLE(aio->u.abt.req_que_no,
+	    cpu_to_le32(make_handle(aio->u.abt.req_que_no,
 				    aio->u.abt.cmd_hndl));
 	abt_iocb->vp_index = vha->vp_idx;
 	abt_iocb->req_que_no = cpu_to_le16(aio->u.abt.req_que_no);

@@ -3905,7 +3905,7 @@ qla2x00_start_bidir(srb_t *sp, struct scsi_qla_host *vha, uint32_t tot_dsds)
 	}
 
 	cmd_pkt = (struct cmd_bidir *)req->ring_ptr;
-	cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+	cmd_pkt->handle = make_handle(req->id, handle);
 
 	/* Zero out remaining portion of packet. */
 	/* tagged queuing modifier -- default is TSK_SIMPLE (0).*/
drivers/scsi/qla2xxx/qla_mbx.c +5 -5
index ff945d35ba8e..1563396ad658 100644
@@ -2383,7 +2383,7 @@ qla24xx_login_fabric(scsi_qla_host_t *vha, uint16_t loop_id, uint8_t domain,
 
 	lg->entry_type = LOGINOUT_PORT_IOCB_TYPE;
 	lg->entry_count = 1;
-	lg->handle = MAKE_HANDLE(req->id, lg->handle);
+	lg->handle = make_handle(req->id, lg->handle);
 	lg->nport_handle = cpu_to_le16(loop_id);
 	lg->control_flags = cpu_to_le16(LCF_COMMAND_PLOGI);
 	if (opt & BIT_0)

@@ -2653,7 +2653,7 @@ qla24xx_fabric_logout(scsi_qla_host_t *vha, uint16_t loop_id, uint8_t domain,
 	req = vha->req;
 	lg->entry_type = LOGINOUT_PORT_IOCB_TYPE;
 	lg->entry_count = 1;
-	lg->handle = MAKE_HANDLE(req->id, lg->handle);
+	lg->handle = make_handle(req->id, lg->handle);
 	lg->nport_handle = cpu_to_le16(loop_id);
 	lg->control_flags =
 	    cpu_to_le16(LCF_COMMAND_LOGO|LCF_IMPL_LOGO|

@@ -3144,9 +3144,9 @@ qla24xx_abort_command(srb_t *sp)
 
 	abt->entry_type = ABORT_IOCB_TYPE;
 	abt->entry_count = 1;
-	abt->handle = MAKE_HANDLE(req->id, abt->handle);
+	abt->handle = make_handle(req->id, abt->handle);
 	abt->nport_handle = cpu_to_le16(fcport->loop_id);
-	abt->handle_to_abort = MAKE_HANDLE(req->id, handle);
+	abt->handle_to_abort = make_handle(req->id, handle);
 	abt->port_id[0] = fcport->d_id.b.al_pa;
 	abt->port_id[1] = fcport->d_id.b.area;
 	abt->port_id[2] = fcport->d_id.b.domain;

@@ -3223,7 +3223,7 @@ __qla24xx_issue_tmf(char *name, uint32_t type, struct fc_port *fcport,
 
 	tsk->p.tsk.entry_type = TSK_MGMT_IOCB_TYPE;
 	tsk->p.tsk.entry_count = 1;
-	tsk->p.tsk.handle = MAKE_HANDLE(req->id, tsk->p.tsk.handle);
+	tsk->p.tsk.handle = make_handle(req->id, tsk->p.tsk.handle);
 	tsk->p.tsk.nport_handle = cpu_to_le16(fcport->loop_id);
 	tsk->p.tsk.timeout = cpu_to_le16(ha->r_a_tov / 10 * 2);
 	tsk->p.tsk.control_flags = cpu_to_le32(type);
drivers/scsi/qla2xxx/qla_mr.c +4 -4
index 6d120457478e..df99911b8bb9 100644
@@ -3135,7 +3135,7 @@ qlafx00_start_scsi(srb_t *sp)
 
 	memset(&lcmd_pkt, 0, REQUEST_ENTRY_SIZE);
 
-	lcmd_pkt.handle = MAKE_HANDLE(req->id, sp->handle);
+	lcmd_pkt.handle = make_handle(req->id, sp->handle);
 	lcmd_pkt.reserved_0 = 0;
 	lcmd_pkt.port_path_ctrl = 0;
 	lcmd_pkt.reserved_1 = 0;

@@ -3205,7 +3205,7 @@ qlafx00_tm_iocb(srb_t *sp, struct tsk_mgmt_entry_fx00 *ptm_iocb)
 	memset(&tm_iocb, 0, sizeof(struct tsk_mgmt_entry_fx00));
 	tm_iocb.entry_type = TSK_MGMT_IOCB_TYPE_FX00;
 	tm_iocb.entry_count = 1;
-	tm_iocb.handle = cpu_to_le32(MAKE_HANDLE(req->id, sp->handle));
+	tm_iocb.handle = cpu_to_le32(make_handle(req->id, sp->handle));
 	tm_iocb.reserved_0 = 0;
 	tm_iocb.tgt_id = cpu_to_le16(sp->fcport->tgt_id);
 	tm_iocb.control_flags = cpu_to_le32(fxio->u.tmf.flags);

@@ -3231,9 +3231,9 @@ qlafx00_abort_iocb(srb_t *sp, struct abort_iocb_entry_fx00 *pabt_iocb)
 	memset(&abt_iocb, 0, sizeof(struct abort_iocb_entry_fx00));
 	abt_iocb.entry_type = ABORT_IOCB_TYPE_FX00;
 	abt_iocb.entry_count = 1;
-	abt_iocb.handle = cpu_to_le32(MAKE_HANDLE(req->id, sp->handle));
+	abt_iocb.handle = cpu_to_le32(make_handle(req->id, sp->handle));
 	abt_iocb.abort_handle =
-	    cpu_to_le32(MAKE_HANDLE(req->id, fxio->u.abt.cmd_hndl));
+	    cpu_to_le32(make_handle(req->id, fxio->u.abt.cmd_hndl));
 	abt_iocb.tgt_id_sts = cpu_to_le16(sp->fcport->tgt_id);
 	abt_iocb.req_que_no = cpu_to_le16(req->id);
drivers/scsi/qla2xxx/qla_nvme.c +1 -1
index bfcd02fdf2b8..84e2a980dea0 100644
@@ -413,7 +413,7 @@ static inline int qla2x00_start_nvme_mq(srb_t *sp)
 	req->cnt -= req_cnt;
 
 	cmd_pkt = (struct cmd_nvme *)req->ring_ptr;
-	cmd_pkt->handle = MAKE_HANDLE(req->id, handle);
+	cmd_pkt->handle = make_handle(req->id, handle);
 
 	/* Zero out remaining portion of packet. */
 	clr_ptr = (uint32_t *)cmd_pkt + 2;
drivers/scsi/qla2xxx/qla_target.c +3 -3
index 243f87df3d2b..d0dbddcef70f 100644
@@ -1758,7 +1758,7 @@ static int qlt_build_abts_resp_iocb(struct qla_tgt_mgmt_cmd *mcmd)
 		qpair->req->outstanding_cmds[h] = (srb_t *)mcmd;
 	}
 
-	resp->handle = MAKE_HANDLE(qpair->req->id, h);
+	resp->handle = make_handle(qpair->req->id, h);
 	resp->entry_type = ABTS_RESP_24XX;
 	resp->entry_count = 1;
 	resp->nport_handle = abts->nport_handle;

@@ -2580,7 +2580,7 @@ static int qlt_24xx_build_ctio_pkt(struct qla_qpair *qpair,
 	} else
 		qpair->req->outstanding_cmds[h] = (srb_t *)prm->cmd;
 
-	pkt->handle = MAKE_HANDLE(qpair->req->id, h);
+	pkt->handle = make_handle(qpair->req->id, h);
 	pkt->handle |= CTIO_COMPLETION_HANDLE_MARK;
 	pkt->nport_handle = cpu_to_le16(prm->cmd->loop_id);
 	pkt->timeout = cpu_to_le16(QLA_TGT_TIMEOUT);

@@ -3093,7 +3093,7 @@ qlt_build_ctio_crc2_pkt(struct qla_qpair *qpair, struct qla_tgt_prm *prm)
 	} else
 		qpair->req->outstanding_cmds[h] = (srb_t *)prm->cmd;
 
-	pkt->handle  = MAKE_HANDLE(qpair->req->id, h);
+	pkt->handle  = make_handle(qpair->req->id, h);
 	pkt->handle |= CTIO_COMPLETION_HANDLE_MARK;
 	pkt->nport_handle = cpu_to_le16(prm->cmd->loop_id);
 	pkt->timeout = cpu_to_le16(QLA_TGT_TIMEOUT);
#7 lore raw

Re: [PATCH v3 4/5] qla2xxx: Convert MAKE_HANDLE() from a define into an inline function

Hi Bart,

Quoted 13 lines · Bart Van Assche
drivers/scsi/qla2xxx/qla_def.h +4 -1
-#define MAKE_HANDLE(x, y) ((uint32_t)((((uint32_t)(x)) << 16) | (uint32_t)(y)))
+static inline uint32_t make_handle(uint16_t x, uint16_t y)
+{
+	return ((uint32_t)x << 16) | y;
+}
 
 /*
  * I/O register
drivers/scsi/qla2xxx/qla_iocb.c +0 -0
index 47bf60a9490a..1816660768da 100644

drivers/scsi/qla2xxx/qla_iocb.c __qla2x00_marker(struct scsi_qla_host *vha, struct qla_qpair *qpair, · line 533

Quoted 5 lines
drivers/scsi/qla2xxx/qla_iocb.c +1 -1
@@ -530,7 +530,7 @@ __qla2x00_marker(struct scsi_qla_host *vha, struct qla_qpair *qpair,
 			int_to_scsilun(lun, (struct scsi_lun *)&mrk24->lun);
 			host_to_fcp_swap(mrk24->lun, sizeof(mrk24->lun));
 			mrk24->vp_index = vha->vp_idx;
-			mrk24->handle = MAKE_HANDLE(req->id, mrk24->handle);
+			mrk24->handle = make_handle(req->id, mrk24->handle);

The type of mrk24->handle is uint32_t and make_handle() is using type uint16_t. Shouldn't the argument type for the y argument be uint32_t as well?

Thanks,
Daniel

author
#8 lore raw

Re: [PATCH v3 4/5] qla2xxx: Convert MAKE_HANDLE() from a define into an inline function

Quoted 7 lines · Daniel Wagner
Quoted 6 lines · Bart Van Assche
drivers/scsi/qla2xxx/qla_def.h +4 -1
-#define MAKE_HANDLE(x, y) ((uint32_t)((((uint32_t)(x)) << 16) | (uint32_t)(y)))
+static inline uint32_t make_handle(uint16_t x, uint16_t y)
+{
+	return ((uint32_t)x << 16) | y;
+}
Quoted 16 lines

drivers/scsi/qla2xxx/qla_iocb.c __qla2x00_marker(struct scsi_qla_host *vha, struct qla_qpair *qpair, · line 533

Quoted 12 lines

/*
* I/O register

drivers/scsi/qla2xxx/qla_iocb.c +1 -1
index 47bf60a9490a..1816660768da 100644
@@ -530,7 +530,7 @@ __qla2x00_marker(struct scsi_qla_host *vha, struct qla_qpair *qpair,
  			int_to_scsilun(lun, (struct scsi_lun *)&mrk24->lun);
  			host_to_fcp_swap(mrk24->lun, sizeof(mrk24->lun));
  			mrk24->vp_index = vha->vp_idx;
-			mrk24->handle = MAKE_HANDLE(req->id, mrk24->handle);
+			mrk24->handle = make_handle(req->id, mrk24->handle);

The type of mrk24->handle is uint32_t and make_handle() is using type uint16_t. Shouldn't the argument type for the y argument be uint32_t as well?

Hi Daniel,

As one can see in __qla2x00_marker() a value is assigned to mrk24->handle() by __qla2x00_alloc_iocbs(). That function calls qla2xxx_get_next_handle() to determine the 'handle' value. The implementation of that last function is as follows:

uint32_t qla2xxx_get_next_handle(struct req_que *req)

{
	uint32_t index, handle = req->current_outstanding_cmd;

	for (index = 1; index < req->num_outstanding_cmds; index++) {
		handle++;
		if (handle == req->num_outstanding_cmds)
			handle = 1;
		if (!req->outstanding_cmds[handle])
			return handle;
	}

	return 0;
}

Since 'num_outstanding_cmds' is a 16-bit variable I think that guarantees that the code quoted in your e-mail passes a 16-bit value as the second argument to make_handle().

Additionally, if the second argument to make_handle() would be larger than 0x10000, the following code from qla2x00_status_entry() would map sts->handle to another queue and another command than those through wich the command was submitted to the firmware:

	handle = (uint32_t) LSW(sts->handle);
	que = MSW(sts->handle);
	req = ha->req_q_map[que];

Bart.

Reviewed-by
#9 lore raw

Re: [PATCH v3 4/5] qla2xxx: Convert MAKE_HANDLE() from a define into an inline function

Hi Bart,

Quoted 24 lines · Bart Van Assche

As one can see in __qla2x00_marker() a value is assigned to mrk24->handle() by __qla2x00_alloc_iocbs(). That function calls qla2xxx_get_next_handle() to determine the 'handle' value. The implementation of that last function is as follows:

uint32_t qla2xxx_get_next_handle(struct req_que *req)

{
	uint32_t index, handle = req->current_outstanding_cmd;

	for (index = 1; index < req->num_outstanding_cmds; index++) {
		handle++;
		if (handle == req->num_outstanding_cmds)
			handle = 1;
		if (!req->outstanding_cmds[handle])
			return handle;
	}

	return 0;
}

Since 'num_outstanding_cmds' is a 16-bit variable I think that guarantees that the code quoted in your e-mail passes a 16-bit value as the second argument to make_handle().

Quoted 8 lines

Additionally, if the second argument to make_handle() would be larger than 0x10000, the following code from qla2x00_status_entry() would map sts->handle to another queue and another command than those through wich the command was submitted to the firmware:

	handle = (uint32_t) LSW(sts->handle);
	que = MSW(sts->handle);
	req = ha->req_q_map[que];

Thanks for digging through it. I stopped at the function signature :)

Changing the return type of qla2xxx_get_next_handle() would be a new patch.

In this case this patch is good.

Reviewed-by Daniel Wagner <[email protected]>
Reviewed-by on v3 4/5
#10 lore raw

Re: [PATCH v3 4/5] qla2xxx: Convert MAKE_HANDLE() from a define into an inline function

In reply to Bart Van Assche
Quoted 8 lines · Bart Van Assche

This patch allows sparse to verify the endianness of the arguments passed to make_handle().

Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Daniel Wagner <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
Quoted 9 lines
---
 drivers/scsi/qla2xxx/qla_def.h    |  5 ++++-
 drivers/scsi/qla2xxx/qla_iocb.c   | 22 +++++++++++-----------
 drivers/scsi/qla2xxx/qla_mbx.c    | 10 +++++-----
 drivers/scsi/qla2xxx/qla_mr.c     |  8 ++++----
 drivers/scsi/qla2xxx/qla_nvme.c   |  2 +-
 drivers/scsi/qla2xxx/qla_target.c |  6 +++---
 6 files changed, 28 insertions(+), 25 deletions(-)
Reviewed-by Roman Bolshakov <[email protected]>

Thanks,
Roman

author
#11 lore raw

PATCH v3 5/5 qla2xxx: Fix the endianness annotations for the port attribute max_frame_size member

In reply to Bart Van Assche

Make sure that sparse doesn't complain about the statements that load or store the port attribute max_frame_size member. The port attribute data structures represent FC protocol data and hence use big endian format. This patch only changes the meaning of two ql_dbg() statements.

Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 drivers/scsi/qla2xxx/qla_def.h |  4 ++--
 drivers/scsi/qla2xxx/qla_gs.c  | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)
drivers/scsi/qla2xxx/qla_def.h +2 -2
index cec0b5082927..c5c3a532f299 100644
@@ -2741,7 +2741,7 @@ struct ct_fdmiv2_port_attr {
 		uint8_t fc4_types[32];
 		uint32_t sup_speed;
 		uint32_t cur_speed;
-		uint32_t max_frame_size;
+		__be32	max_frame_size;
 		uint8_t os_dev_name[32];
 		uint8_t host_name[256];
 		uint8_t node_name[WWN_SIZE];

@@ -2772,7 +2772,7 @@ struct ct_fdmi_port_attr {
 		uint8_t fc4_types[32];
 		uint32_t sup_speed;
 		uint32_t cur_speed;
-		uint32_t max_frame_size;
+		__be32	max_frame_size;
 		uint8_t os_dev_name[32];
 		uint8_t host_name[256];
 	} a;
drivers/scsi/qla2xxx/qla_gs.c +6 -8
index aaa4a5bbf2ff..594b366db0ef 100644
@@ -1848,14 +1848,13 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha)
 	eiter = entries + size;
 	eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE);
 	eiter->len = cpu_to_be16(4 + 4);
-	eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ?
+	eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ?
 	    le16_to_cpu(icb24->frame_payload_size) :
-	    le16_to_cpu(ha->init_cb->frame_payload_size);
-	eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
+	    le16_to_cpu(ha->init_cb->frame_payload_size));
 	size += 4 + 4;
 
 	ql_dbg(ql_dbg_disc, vha, 0x203c,
-	    "Max_Frame_Size=%x.\n", eiter->a.max_frame_size);
+	       "Max_Frame_Size=%x.\n", be32_to_cpu(eiter->a.max_frame_size));
 
 	/* OS device name. */
 	eiter = entries + size;

@@ -2419,14 +2418,13 @@ qla2x00_fdmiv2_rpa(scsi_qla_host_t *vha)
 	eiter = entries + size;
 	eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE);
 	eiter->len = cpu_to_be16(4 + 4);
-	eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ?
+	eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ?
 	    le16_to_cpu(icb24->frame_payload_size) :
-	    le16_to_cpu(ha->init_cb->frame_payload_size);
-	eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
+	    le16_to_cpu(ha->init_cb->frame_payload_size));
 	size += 4 + 4;
 
 	ql_dbg(ql_dbg_disc, vha, 0x20bc,
-	    "Max_Frame_Size = %x.\n", eiter->a.max_frame_size);
+	       "Max_Frame_Size = %x.\n", be32_to_cpu(eiter->a.max_frame_size));
 
 	/* OS device name. */
 	eiter = entries + size;
Reviewed-by on v3 5/5
#12 lore raw

Re: [PATCH v3 5/5] qla2xxx: Fix the endianness annotations for the port attribute max_frame_size member

Quoted 10 lines · Bart Van Assche

Make sure that sparse doesn't complain about the statements that load or store the port attribute max_frame_size member. The port attribute data structures represent FC protocol data and hence use big endian format. This patch only changes the meaning of two ql_dbg() statements.

Reviewed-by Daniel Wagner <[email protected]>
Cc Quinn Tran <[email protected]>
Cc Martin Wilck <[email protected]>
Cc Roman Bolshakov <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
Quoted 5 lines
---
 drivers/scsi/qla2xxx/qla_def.h |  4 ++--
 drivers/scsi/qla2xxx/qla_gs.c  | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

Hi Bart,

As I said in reply to previous version, IMO, all four-byte binary fields and bitmasks in RPA request should be made __be32 rather than only one. Probably that may go into a patch series where each field in the structure is fixed patch-by-patch or one patch that fixes all fields at once.

According to Table 402 – Port Attribute Values in FC-GS-7 and RPA request attributes in the structure above, that includes:
Supported Speed
Current Port Speed
Maximum Frame Size
Port Type
Supported Classes of Service
Port State
Number of Discovered Ports
Port Identifier

But the patch itself looks good,

Reviewed-by Roman Bolshakov <[email protected]>

Regards,
Roman

Quoted 62 lines
drivers/scsi/qla2xxx/qla_def.h +2 -2
index cec0b5082927..c5c3a532f299 100644
@@ -2741,7 +2741,7 @@ struct ct_fdmiv2_port_attr {
 		uint8_t fc4_types[32];
 		uint32_t sup_speed;
 		uint32_t cur_speed;
-		uint32_t max_frame_size;
+		__be32	max_frame_size;
 		uint8_t os_dev_name[32];
 		uint8_t host_name[256];
 		uint8_t node_name[WWN_SIZE];

@@ -2772,7 +2772,7 @@ struct ct_fdmi_port_attr {
 		uint8_t fc4_types[32];
 		uint32_t sup_speed;
 		uint32_t cur_speed;
-		uint32_t max_frame_size;
+		__be32	max_frame_size;
 		uint8_t os_dev_name[32];
 		uint8_t host_name[256];
 	} a;
drivers/scsi/qla2xxx/qla_gs.c +6 -8
index aaa4a5bbf2ff..594b366db0ef 100644
@@ -1848,14 +1848,13 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha)
 	eiter = entries + size;
 	eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE);
 	eiter->len = cpu_to_be16(4 + 4);
-	eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ?
+	eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ?
 	    le16_to_cpu(icb24->frame_payload_size) :
-	    le16_to_cpu(ha->init_cb->frame_payload_size);
-	eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
+	    le16_to_cpu(ha->init_cb->frame_payload_size));
 	size += 4 + 4;
 
 	ql_dbg(ql_dbg_disc, vha, 0x203c,
-	    "Max_Frame_Size=%x.\n", eiter->a.max_frame_size);
+	       "Max_Frame_Size=%x.\n", be32_to_cpu(eiter->a.max_frame_size));
 
 	/* OS device name. */
 	eiter = entries + size;

@@ -2419,14 +2418,13 @@ qla2x00_fdmiv2_rpa(scsi_qla_host_t *vha)
 	eiter = entries + size;
 	eiter->type = cpu_to_be16(FDMI_PORT_MAX_FRAME_SIZE);
 	eiter->len = cpu_to_be16(4 + 4);
-	eiter->a.max_frame_size = IS_FWI2_CAPABLE(ha) ?
+	eiter->a.max_frame_size = cpu_to_be32(IS_FWI2_CAPABLE(ha) ?
 	    le16_to_cpu(icb24->frame_payload_size) :
-	    le16_to_cpu(ha->init_cb->frame_payload_size);
-	eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
+	    le16_to_cpu(ha->init_cb->frame_payload_size));
 	size += 4 + 4;
 
 	ql_dbg(ql_dbg_disc, vha, 0x20bc,
-	    "Max_Frame_Size = %x.\n", eiter->a.max_frame_size);
+	       "Max_Frame_Size = %x.\n", be32_to_cpu(eiter->a.max_frame_size));
 
 	/* OS device name. */
 	eiter = entries + size;
#13 lore raw
In reply to Bart Van Assche

Bart,

Quoted 1 line

Please consider these five qla2xxx patches for kernel v5.7.

Applied 1-4 to 5.7/scsi-queue. Patch 5 doesn't apply on top of the latest driver update so please rebase.

Thanks!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering