[PATCH v2 0/6] qla2xxx patches for kernel v5.6

[PATCH v2 0/6] qla2xxx patches for kernel v5.6

[Bart Van Assche]

Hi Martin,

Please consider these six small qla2xxx patches for kernel v5.6.

Thanks,

Bart.

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

Bart Van Assche (6):
  qla2xxx: Check locking assumptions at runtime in qla2x00_abort_srb()
  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_isr.c  |  5 -----
 drivers/scsi/qla2xxx/qla_mbx.c  |  5 ++---
 drivers/scsi/qla2xxx/qla_mr.c   |  5 ++---
 drivers/scsi/qla2xxx/qla_os.c   | 29 ++++++++++++++++-------------
 7 files changed, 38 insertions(+), 45 deletions(-)

[PATCH v2 1/6] qla2xxx: Check locking assumptions at runtime in qla2x00_abort_srb()

[Bart Van Assche]

Document the locking assumptions this function relies on and also verify these
locking assumptions at runtime.

Acked-by: Himanshu Madhani <hmadhani@marvell.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_os.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index b520a980d1dc..79387ac8936f 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1700,6 +1700,8 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
 	bool ret_cmd;
 	uint32_t ratov_j;
 
+	lockdep_assert_held(qp->qp_lock_ptr);
+
 	if (qla2x00_chip_is_down(vha)) {
 		sp->done(sp, res);
 		return;

[PATCH v2 2/6] qla2xxx: Simplify the code for aborting SCSI commands

[Bart Van Assche]

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.

This patch eliminates several race conditions triggered by the removed member
variables.

Acked-by: Himanshu Madhani <hmadhani@marvell.com>
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 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(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index ed32e9715794..c5a067f45005 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -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;
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index e7bad0bfffda..0c9bfe77ba8a 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2504,11 +2504,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,
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 79387ac8936f..a34f27b2d602 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -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)
 {

[PATCH v2 3/6] qla2xxx: Suppress endianness complaints in qla2x00_configure_local_loop()

[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.

Cc: Quinn Tran <qutran@marvell.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_def.h  |  2 +-
 drivers/scsi/qla2xxx/qla_init.c | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index c5a067f45005..b04d334933ef 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -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 {
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 9e6b56527b25..880f2be062a9 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -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");

[PATCH v2 4/6] qla2xxx: Fix sparse warnings triggered by the PCI state checking code

[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,
};

Cc: Quinn Tran <qutran@marvell.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_mbx.c | 5 ++---
 drivers/scsi/qla2xxx/qla_mr.c  | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
index 9e09964f5c0e..ff945d35ba8e 100644
--- a/drivers/scsi/qla2xxx/qla_mbx.c
+++ b/drivers/scsi/qla2xxx/qla_mbx.c
@@ -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;
 	}
 
diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index bad043c40622..33161af2c16a 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -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;
 	}
 

[PATCH v2 5/6] qla2xxx: Convert MAKE_HANDLE() from a define into an inline function

[Bart Van Assche]

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

Cc: Quinn Tran <qutran@marvell.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_def.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index b04d334933ef..968f19995063 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -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

[PATCH v2 6/6] qla2xxx: Fix the endianness annotations for the port attribute max_frame_size member

[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.

Cc: Quinn Tran <qutran@marvell.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_def.h |  4 ++--
 drivers/scsi/qla2xxx/qla_gs.c  | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 968f19995063..5c6bae116b58 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -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;
diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index aaa4a5bbf2ff..594b366db0ef 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -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;

Re: [PATCH v2 1/6] qla2xxx: Check locking assumptions at runtime in qla2x00_abort_srb()

[Daniel Wagner]

On Wed, Jan 22, 2020 at 08:23:40PM -0800, Bart Van Assche wrote:
> Document the locking assumptions this function relies on and also verify these
> locking assumptions at runtime.
> 
> Acked-by: Himanshu Madhani <hmadhani@marvell.com>
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH v2 2/6] qla2xxx: Simplify the code for aborting SCSI commands

[Daniel Wagner]

On Wed, Jan 22, 2020 at 08:23:41PM -0800, Bart Van Assche wrote:
> 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.
> 
> This patch eliminates several race conditions triggered by the removed member
> variables.

I can only guess here what the races are but I agree removing the
logic here and relying on the SCSI layer to handle it correctly makes
sense. 

> Acked-by: Himanshu Madhani <hmadhani@marvell.com>
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

> +/*
> + * The caller must ensure that no completion interrupts will happen
> + * while this function is in progress.
> + */

So could we add something like WARN_ON(irqs_disabled())?

Re: [PATCH v2 3/6] qla2xxx: Suppress endianness complaints in qla2x00_configure_local_loop()

[Daniel Wagner]

On Wed, Jan 22, 2020 at 08:23:42PM -0800, Bart Van Assche wrote:
> 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.
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH v2 4/6] qla2xxx: Fix sparse warnings triggered by the PCI state checking code

[Daniel Wagner]

On Wed, Jan 22, 2020 at 08:23:43PM -0800, Bart Van Assche wrote:
> 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,
> };
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

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

[Daniel Wagner]

On Wed, Jan 22, 2020 at 08:23:44PM -0800, Bart Van Assche wrote:
> This patch allows sparse to verify the endianness of the arguments passed
> to MAKE_HANDLE().
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_def.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index b04d334933ef..968f19995063 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -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;
> +}

s/MAKE_HANDLE/make_handle/ ?

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

[Daniel Wagner]

On Wed, Jan 22, 2020 at 08:23:45PM -0800, Bart Van Assche wrote:
> 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.
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de?

Re: [PATCH v2 2/6] qla2xxx: Simplify the code for aborting SCSI commands

[Bart Van Assche]

On 2020-01-23 02:17, Daniel Wagner wrote:
> On Wed, Jan 22, 2020 at 08:23:41PM -0800, Bart Van Assche wrote:
>> 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.
>>
>> This patch eliminates several race conditions triggered by the removed member
>> variables.
> 
> I can only guess here what the races are but I agree removing the
> logic here and relying on the SCSI layer to handle it correctly makes
> sense. 

I will make the patch description more clear when I repost this patch.

>> +/*
>> + * The caller must ensure that no completion interrupts will happen
>> + * while this function is in progress.
>> + */
> 
> So could we add something like WARN_ON(irqs_disabled())?

qla2x00_abort_all_cmds() is typically called after the kernel driver
discovered that the firmware stopped processing commands. If the
firmware has stopped processing commands it is safe to call this
function without disabling interrupts. Even if the caller would disable
interrupts, that would only disable interrupts on the local CPU but not
on other CPUs. Additionally, disabling interrupts is not a proper
solution because if a completion interrupt arrives after a command has
been aborted that will cause trouble if the command handle has already
been associated with another command.

Bart.

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

[Bart Van Assche]

On 2020-01-23 02:39, Daniel Wagner wrote:
> s/MAKE_HANDLE/make_handle/ ?

That will make the function name conformant with the coding style. I
will look into this.

Bart.

Re: [PATCH v2 2/6] qla2xxx: Simplify the code for aborting SCSI commands

[Daniel Wagner]

Hi Bart,

On Thu, Jan 23, 2020 at 07:12:04PM -0800, Bart Van Assche wrote:
> >> +/*
> >> + * The caller must ensure that no completion interrupts will happen
> >> + * while this function is in progress.
> >> + */
> > 
> > So could we add something like WARN_ON(irqs_disabled())?
> 
> qla2x00_abort_all_cmds() is typically called after the kernel driver
> discovered that the firmware stopped processing commands. If the
> firmware has stopped processing commands it is safe to call this
> function without disabling interrupts. Even if the caller would disable
> interrupts, that would only disable interrupts on the local CPU but not
> on other CPUs. Additionally, disabling interrupts is not a proper
> solution because if a completion interrupt arrives after a command has
> been aborted that will cause trouble if the command handle has already
> been associated with another command.

Thanks for the explenation. I understood the comment as 'interrupts'
have to be disabled when calling this function.

Thanks,
Daniel

Re: [PATCH v2 0/6] qla2xxx patches for kernel v5.6

[Martin K. Petersen]

Bart,

> Please consider these six small qla2xxx patches for kernel v5.6.

I applied patch #1. #2-#6 need a repost and some reviews.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 4/6] qla2xxx: Fix sparse warnings triggered by the PCI state checking code

[Roman Bolshakov]

On Wed, Jan 22, 2020 at 08:23:43PM -0800, Bart Van Assche wrote:
> 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,
> };
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_mbx.c | 5 ++---
>  drivers/scsi/qla2xxx/qla_mr.c  | 5 ++---
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 

Hi Bart,

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

Best regards,
Roman

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

[Roman Bolshakov]

On Wed, Jan 22, 2020 at 08:23:45PM -0800, Bart Van Assche wrote:
> 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.
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_def.h |  4 ++--
>  drivers/scsi/qla2xxx/qla_gs.c  | 14 ++++++--------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 968f19995063..5c6bae116b58 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -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;
> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> index aaa4a5bbf2ff..594b366db0ef 100644
> --- a/drivers/scsi/qla2xxx/qla_gs.c
> +++ b/drivers/scsi/qla2xxx/qla_gs.c
> @@ -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;

Hi Bart,

I'm not an expert of FDMI feature but it seems to introduce an
inconsistency with regards to structure definition. IMO, All multi-byte
binary fields and bitmasks in RPA request should be made __be32 rather
than only one. Probably that should go into one patch series where all
fields in the structure are fixed patch-by-patch or one patch that fixes
all fields at once (the latter is harder to review though).

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 <r.bolshakov@yadro.com>

Regards,
Roman

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

[Bart Van Assche]

On 2/25/20 10:43 AM, Roman Bolshakov wrote:
> I'm not an expert of FDMI feature but it seems to introduce an
> inconsistency with regards to structure definition. IMO, All multi-byte
> binary fields and bitmasks in RPA request should be made __be32 rather
> than only one. Probably that should go into one patch series where all
> fields in the structure are fixed patch-by-patch or one patch that fixes
> all fields at once (the latter is harder to review though).

Hi Roman,

I agree with what you wrote. I have a (huge) patch pending that fixes 
all endianness issues in the qla2xxx driver. I plan to post that patch 
after agreement has been achieved about this patch series.

Thanks,

Bart.