Re: [PATCH 01/15] qla2xxx: Combine Active command arrays.

[Madhani, Himanshu]

Hi Bart, 

> On Jun 7, 2017, at 3:45 PM, Bart Van Assche  
> wrote:
> 
> On Wed, 2017-06-07 at 14:43 -0700, Himanshu Madhani wrote:
>> +enum {
>> +TYPE_SRB,
>> +TYPE_TGT_CMD,
>> +};
>> +
>> typedef struct srb {
>> +/*
>> + * Do not move cmd_type field, it needs to
>> + * line up with qla_tgt_cmd->cmd_type
>> + */
>> +uint8_t cmd_type;
>> +uint8_t pad[3];
>>  atomic_t ref_count;
>>  struct fc_port *fcport;
>>  struct scsi_qla_host *vha;
> 
> [ ... ]
> 
>> struct qla_tgt_cmd {
>> +/*
>> + * Do not move cmd_type field. it needs to line up with srb->cmd_type
>> + */
>> +uint8_t cmd_type;
>> +uint8_t pad[7];
>>  struct se_cmd se_cmd;
>>  struct fc_port *sess;
>>  int state;
> 
> Hello Quinn and Himanshu,
> 
> Sorry but this is really inelegant. Have you considered the following?
> - Keep the existing srb and qla_tgt_cmd data structures.
> - Introduce a new data structure with enum cmd_type as the first member and
>  a union of struct srb and struct qla_tgt_cmd as the second member.
> - Use __attribute__((aligned(...))) to express alignment requirements instead
>  of explicitly inserting padding bytes.
> 
> With that approach no casts are needed to convert a pointer to the new data
> structure into a struct srb or struct qla_tgt_cmd pointer - all that will be
> needed is to take the address of the appropriate member of the union.
> 
> Thanks,
> 
> Bart.

We are working on addressing this review comment. We’ll send it as a separate
patch once we run through our regression test cycle.

Thanks,
- Himanshu

Re: [PATCH 01/15] qla2xxx: Combine Active command arrays.

[Bart Van Assche]

On Wed, 2017-06-07 at 14:43 -0700, Himanshu Madhani wrote:
> +enum {
> + TYPE_SRB,
> + TYPE_TGT_CMD,
> +};
> +
>  typedef struct srb {
> + /*
> +  * Do not move cmd_type field, it needs to
> +  * line up with qla_tgt_cmd->cmd_type
> +  */
> + uint8_t cmd_type;
> + uint8_t pad[3];
>   atomic_t ref_count;
>   struct fc_port *fcport;
>   struct scsi_qla_host *vha;

[ ... ]
 
>  struct qla_tgt_cmd {
> + /*
> +  * Do not move cmd_type field. it needs to line up with srb->cmd_type
> +  */
> + uint8_t cmd_type;
> + uint8_t pad[7];
>   struct se_cmd se_cmd;
>   struct fc_port *sess;
>   int state;

Hello Quinn and Himanshu,

Sorry but this is really inelegant. Have you considered the following?
- Keep the existing srb and qla_tgt_cmd data structures.
- Introduce a new data structure with enum cmd_type as the first member and
  a union of struct srb and struct qla_tgt_cmd as the second member.
- Use __attribute__((aligned(...))) to express alignment requirements instead
  of explicitly inserting padding bytes.

With that approach no casts are needed to convert a pointer to the new data
structure into a struct srb or struct qla_tgt_cmd pointer - all that will be
needed is to take the address of the appropriate member of the union.

Thanks,

Bart.

[PATCH 01/15] qla2xxx: Combine Active command arrays.

[Himanshu Madhani]

From: Quinn Tran 

Merge active/outstanding cmd arrays from target side
and initiator side together in prepration for Target
Multi Queue support.

Signed-off-by: Quinn Tran 
Signed-off-by: Himanshu Madhani 
---
 drivers/scsi/qla2xxx/qla_def.h|  15 +++-
 drivers/scsi/qla2xxx/qla_gbl.h|   2 -
 drivers/scsi/qla2xxx/qla_inline.h |   1 +
 drivers/scsi/qla2xxx/qla_isr.c|  48 +
 drivers/scsi/qla2xxx/qla_os.c |  75 ++--
 drivers/scsi/qla2xxx/qla_target.c | 144 --
 drivers/scsi/qla2xxx/qla_target.h |  23 --
 7 files changed, 164 insertions(+), 144 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index ddf93efe3986..1b5049b1ef4a 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -437,7 +437,18 @@ struct srb_iocb {
 #define SRB_NACK_PRLI  17
 #define SRB_NACK_LOGO  18
 
+enum {
+   TYPE_SRB,
+   TYPE_TGT_CMD,
+};
+
 typedef struct srb {
+   /*
+* Do not move cmd_type field, it needs to
+* line up with qla_tgt_cmd->cmd_type
+*/
+   uint8_t cmd_type;
+   uint8_t pad[3];
atomic_t ref_count;
struct fc_port *fcport;
struct scsi_qla_host *vha;
@@ -3287,9 +3298,6 @@ struct qlt_hw_data {
uint32_t __iomem *atio_q_out;
 
struct qla_tgt_func_tmpl *tgt_ops;
-   struct qla_tgt_cmd *cmds[DEFAULT_OUTSTANDING_COMMANDS];
-   uint16_t current_handle;
-
struct qla_tgt_vp_map *tgt_vp_map;
 
int saved_set;
@@ -4258,6 +4266,7 @@ enum nexus_wait_type {
WAIT_LUN,
 };
 
+#include "qla_target.h"
 #include "qla_gbl.h"
 #include "qla_dbg.h"
 #include "qla_inline.h"
diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
index f8540f5c9e5d..63355f40ff2f 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -834,8 +834,6 @@ extern irqreturn_t qla8044_intr_handler(int, void *);
 extern void qla82xx_mbx_completion(scsi_qla_host_t *, uint16_t);
 extern int qla8044_abort_isp(scsi_qla_host_t *);
 extern int qla8044_check_fw_alive(struct scsi_qla_host *);
-
-extern void qlt_host_reset_handler(struct qla_hw_data *ha);
 extern int qla_get_exlogin_status(scsi_qla_host_t *, uint16_t *,
uint16_t *);
 extern int qla_set_exlogin_mem_cfg(scsi_qla_host_t *vha, dma_addr_t phys_addr);
diff --git a/drivers/scsi/qla2xxx/qla_inline.h 
b/drivers/scsi/qla2xxx/qla_inline.h
index 9996ec0daab1..99028d48c664 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -250,6 +250,7 @@ qla2x00_get_sp(scsi_qla_host_t *vha, fc_port_t *fcport, 
gfp_t flag)
 
memset(sp, 0, sizeof(*sp));
sp->fcport = fcport;
+   sp->cmd_type = TYPE_SRB;
sp->iocbs = 1;
sp->vha = vha;
 done:
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 2984abcc29e7..8aaddb75f964 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -17,7 +17,7 @@
 static void qla2x00_mbx_completion(scsi_qla_host_t *, uint16_t);
 static void qla2x00_status_entry(scsi_qla_host_t *, struct rsp_que *, void *);
 static void qla2x00_status_cont_entry(struct rsp_que *, sts_cont_entry_t *);
-static void qla2x00_error_entry(scsi_qla_host_t *, struct rsp_que *,
+static int qla2x00_error_entry(scsi_qla_host_t *, struct rsp_que *,
sts_entry_t *);
 
 /**
@@ -2280,6 +2280,14 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct 
rsp_que *rsp, void *pkt)
return;
}
 
+   if (sp->cmd_type != TYPE_SRB) {
+   req->outstanding_cmds[handle] = NULL;
+   ql_dbg(ql_dbg_io, vha, 0x3015,
+   "Unknown sp->cmd_type %x %p).\n",
+   sp->cmd_type, sp);
+   return;
+   }
+
if (unlikely((state_flags & BIT_1) && (sp->type == SRB_BIDI_CMD))) {
qla25xx_process_bidir_status_iocb(vha, pkt, req, handle);
return;
@@ -2632,8 +2640,9 @@ qla2x00_status_cont_entry(struct rsp_que *rsp, 
sts_cont_entry_t *pkt)
  * qla2x00_error_entry() - Process an error entry.
  * @ha: SCSI driver HA context
  * @pkt: Entry pointer
+ * return : 1=allow further error analysis. 0=no additional error analysis.
  */
-static void
+static int
 qla2x00_error_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, sts_entry_t 
*pkt)
 {
srb_t *sp;
@@ -2654,18 +2663,35 @@ qla2x00_error_entry(scsi_qla_host_t *vha, struct 
rsp_que *rsp, sts_entry_t *pkt)
if (pkt->entry_status & RF_BUSY)
res = DID_BUS_BUSY << 16;
 
-   if (pkt->entry_type == NOTIFY_ACK_TYPE &&
-   pkt->handle == QLA_TGT_SKIP_HANDLE)
-   return;
+   if ((pkt->handle & ~QLA_TGT_HANDLE_MASK) == QLA_TGT_SKIP_HANDLE)
+   return 0;
 
-   sp = qla2x00_get_sp_from_handle(vha, func, req, pkt);
-