Re: [PATCH 2/6] qla2xxx: Add FC-NVMe command handling

[Madhani, Himanshu]

> On Jun 19, 2017, at 2:01 PM, James Smart  wrote:
> 
> On 6/16/2017 3:47 PM, Himanshu Madhani wrote:
>> @@ -615,8 +620,25 @@ struct sts_entry_24xx {
>>  uint32_t rsp_residual_count;/* FCP RSP residual count. */
>>  uint32_t sense_len; /* FCP SENSE length. */
>> -uint32_t rsp_data_len;  /* FCP response data length. */
>> -uint8_t data[28];   /* FCP response/sense information. */
>> +
>> +union {
>> +struct {
>> +uint32_t rsp_data_len;  /* FCP response data length  */
>> +uint8_t data[28];   /* FCP rsp/sense information */
>> +};
>> +struct {
>> +/* nvme ersp hdr */
>> +__u8status_code;
>> +__u8rsvd0;
>> +__be16  iu_len;
>> +__be32  rsn;
>> +__be32  xfrd_len;
>> +__be32  rsvd12;
>> +uint8_t cqe[16];
>> +};
>> +uint8_t nvme_ersp_data[32];
>> +};
>> +
> 
> rather than defining a new structure for ersp - can't you use the struct 
> nvme_fc_ersp_iu definition from include/linux/nvme-fc.h ?
> 

Sure. will remove this

> 
> 
>>  /*
>>   * If DIF Error is set in comp_status, these additional fields are
>>   * defined:
>> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>> index 40385bc1d1fa..0d60fd0da604 100644
>> --- a/drivers/scsi/qla2xxx/qla_isr.c
>> +++ b/drivers/scsi/qla2xxx/qla_isr.c
>> @@ -1799,6 +1799,86 @@ qla24xx_tm_iocb_entry(scsi_qla_host_t *vha, struct 
>> req_que *req, void *tsk)
>>  sp->done(sp, 0);
>>  }
>>  +static void
>> +qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, void 
>> *tsk)
>> +{
>> +const char func[] = "NVME-IOCB";
>> +fc_port_t *fcport;
>> +srb_t *sp;
>> +struct srb_iocb *iocb;
>> +struct sts_entry_24xx *sts = (struct sts_entry_24xx *)tsk;
>> +uint16_tstate_flags;
>> +struct nvmefc_fcp_req *fd;
>> +struct srb_iocb *nvme;
>> +
>> +sp = qla2x00_get_sp_from_handle(vha, func, req, tsk);
>> +if (!sp)
>> +return;
>> +
>> +iocb = >u.iocb_cmd;
>> +fcport = sp->fcport;
>> +iocb->u.nvme.comp_status = le16_to_cpu(sts->comp_status);
>> +state_flags  = le16_to_cpu(sts->state_flags);
>> +fd = iocb->u.nvme.desc;
>> +nvme = >u.iocb_cmd;
>> +
>> +if (unlikely(nvme->u.nvme.aen_op)) {
>> +atomic_dec(>vha->nvme_active_aen_cnt);
>> +/*
>> + * Needed right now since the transport doesn't cleanup
>> + * up AEN's IO's on remote port delete.
>> + * they could have gone away while we still have the *fd.
>> + */
>> +if (!atomic_read(>fcport->nvme_ref_count)) {
>> +sp->done(sp, QLA_FUNCTION_FAILED);
>> +return;
> 
> The missing aen abort has been corrected for several months.. Should be no 
> need for it.
> 
Ack. Will remove it.

> 
>> @@ -5996,6 +5998,18 @@ qla2x00_timer(scsi_qla_host_t *vha)
>>  if (!list_empty(>work_list))
>>  start_dpc++;
>>  +   /*
>> + * FC-NVME
>> + * see if the active AEN count has changed from what was last reported.
>> + */
>> +if (atomic_read(>nvme_active_aen_cnt) != vha->nvme_last_rptd_aen) {
>> +vha->nvme_last_rptd_aen =
>> +atomic_read(>nvme_active_aen_cnt);
>> +ql_log(ql_log_info, vha, 0x3002,
>> +"reporting new aen count of %d to the fw TBD\n",
>> +vha->nvme_last_rptd_aen);
>> +}
>> +
>>  /* Schedule the DPC routine if needed */
>>  if ((test_bit(ISP_ABORT_NEEDED, >dpc_flags) ||
>>  test_bit(LOOP_RESYNC_NEEDED, >dpc_flags) ||
> 
> Q: why does lldd need to track aen vs any other type of fcp operation ?

These are long lived exchanges and affect the adapter’s interrupt generation 
logic.

Thanks,
- Himanshu

Re: [PATCH 2/6] qla2xxx: Add FC-NVMe command handling

[James Smart]

On 6/16/2017 3:47 PM, Himanshu Madhani wrote:

@@ -615,8 +620,25 @@ struct sts_entry_24xx {
uint32_t rsp_residual_count;/* FCP RSP residual count. */
  
  	uint32_t sense_len;		/* FCP SENSE length. */

-   uint32_t rsp_data_len;  /* FCP response data length. */
-   uint8_t data[28];   /* FCP response/sense information. */
+
+   union {
+   struct {
+   uint32_t rsp_data_len;  /* FCP response data length  */
+   uint8_t data[28];   /* FCP rsp/sense information */
+   };
+   struct {
+   /* nvme ersp hdr */
+   __u8status_code;
+   __u8rsvd0;
+   __be16  iu_len;
+   __be32  rsn;
+   __be32  xfrd_len;
+   __be32  rsvd12;
+   uint8_t cqe[16];
+   };
+   uint8_t nvme_ersp_data[32];
+   };
+


rather than defining a new structure for ersp - can't you use the struct 
nvme_fc_ersp_iu definition from include/linux/nvme-fc.h ?





/*
 * If DIF Error is set in comp_status, these additional fields are
 * defined:
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 40385bc1d1fa..0d60fd0da604 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1799,6 +1799,86 @@ qla24xx_tm_iocb_entry(scsi_qla_host_t *vha, struct 
req_que *req, void *tsk)
sp->done(sp, 0);
  }
  
+static void

+qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, void *tsk)
+{
+   const char func[] = "NVME-IOCB";
+   fc_port_t *fcport;
+   srb_t *sp;
+   struct srb_iocb *iocb;
+   struct sts_entry_24xx *sts = (struct sts_entry_24xx *)tsk;
+   uint16_tstate_flags;
+   struct nvmefc_fcp_req *fd;
+   struct srb_iocb *nvme;
+
+   sp = qla2x00_get_sp_from_handle(vha, func, req, tsk);
+   if (!sp)
+   return;
+
+   iocb = >u.iocb_cmd;
+   fcport = sp->fcport;
+   iocb->u.nvme.comp_status = le16_to_cpu(sts->comp_status);
+   state_flags  = le16_to_cpu(sts->state_flags);
+   fd = iocb->u.nvme.desc;
+   nvme = >u.iocb_cmd;
+
+   if (unlikely(nvme->u.nvme.aen_op)) {
+   atomic_dec(>vha->nvme_active_aen_cnt);
+   /*
+* Needed right now since the transport doesn't cleanup
+* up AEN's IO's on remote port delete.
+* they could have gone away while we still have the *fd.
+*/
+   if (!atomic_read(>fcport->nvme_ref_count)) {
+   sp->done(sp, QLA_FUNCTION_FAILED);
+   return;


The missing aen abort has been corrected for several months.. Should be 
no need for it.




@@ -5996,6 +5998,18 @@ qla2x00_timer(scsi_qla_host_t *vha)
if (!list_empty(>work_list))
start_dpc++;
  
+	/*

+* FC-NVME
+* see if the active AEN count has changed from what was last reported.
+*/
+   if (atomic_read(>nvme_active_aen_cnt) != vha->nvme_last_rptd_aen) {
+   vha->nvme_last_rptd_aen =
+   atomic_read(>nvme_active_aen_cnt);
+   ql_log(ql_log_info, vha, 0x3002,
+   "reporting new aen count of %d to the fw TBD\n",
+   vha->nvme_last_rptd_aen);
+   }
+
/* Schedule the DPC routine if needed */
if ((test_bit(ISP_ABORT_NEEDED, >dpc_flags) ||
test_bit(LOOP_RESYNC_NEEDED, >dpc_flags) ||


Q: why does lldd need to track aen vs any other type of fcp operation ?

Re: [PATCH 2/6] qla2xxx: Add FC-NVMe command handling

[Madhani, Himanshu]

> On Jun 19, 2017, at 1:20 AM, Johannes Thumshirn  wrote:
> 
> On Fri, Jun 16, 2017 at 03:47:40PM -0700, Himanshu Madhani wrote:
>> From: Duane Grigsby 
>> 
>> Signed-off-by: Darren Trapp 
>> Signed-off-by: Duane Grigsby 
>> Signed-off-by: Anil Gurumurthy 
>> Signed-off-by: Giridhar Malavali 
>> Signed-off-by: Himanshu Madhani 
>> ---
> 
> [...]
> 
>> +static void
>> +qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, void 
>> *tsk)
>> +{
>> +const char func[] = "NVME-IOCB";
>> +fc_port_t *fcport;
>> +srb_t *sp;
>> +struct srb_iocb *iocb;
>> +struct sts_entry_24xx *sts = (struct sts_entry_24xx *)tsk;
> 
> No need to cast from void*
> 
> 

Thanks for Review. will fix this in v2.

> Reviewed-by: Johannes Thumshirn 
> 
> 
> -- 
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Thanks,
- Himanshu

[PATCH 2/6] qla2xxx: Add FC-NVMe command handling

[Himanshu Madhani]

From: Duane Grigsby 

Signed-off-by: Darren Trapp 
Signed-off-by: Duane Grigsby 
Signed-off-by: Anil Gurumurthy 
Signed-off-by: Giridhar Malavali 
Signed-off-by: Himanshu Madhani 
---
 drivers/scsi/qla2xxx/qla_def.h | 17 +
 drivers/scsi/qla2xxx/qla_fw.h  | 28 --
 drivers/scsi/qla2xxx/qla_isr.c | 86 ++
 drivers/scsi/qla2xxx/qla_os.c  | 18 -
 4 files changed, 144 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index bd1b3fef95a4..693c42392886 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -412,6 +412,20 @@ struct srb_iocb {
struct {
struct imm_ntfy_from_isp *ntfy;
} nack;
+   struct {
+   __le16 comp_status;
+   uint16_t rsp_pyld_len;
+   uint8_t aen_op;
+   void *desc;
+
+   /* These are only used with ls4 requests */
+   int cmd_len;
+   int rsp_len;
+   dma_addr_t cmd_dma;
+   dma_addr_t rsp_dma;
+   uint32_t dl;
+   uint32_t timeout_sec;
+   } nvme;
} u;
 
struct timer_list timer;
@@ -437,6 +451,7 @@ struct srb_iocb {
 #define SRB_NACK_PLOGI 16
 #define SRB_NACK_PRLI  17
 #define SRB_NACK_LOGO  18
+#define SRB_NVME_CMD   19
 #define SRB_PRLI_CMD   21
 
 enum {
@@ -4111,6 +4126,8 @@ typedef struct scsi_qla_host {
struct  nvme_fc_local_port *nvme_local_port;
atomic_tnvme_ref_count;
struct list_head nvme_rport_list;
+   atomic_tnvme_active_aen_cnt;
+   uint16_tnvme_last_rptd_aen;
 
uint16_tfcoe_vlan_id;
uint16_tfcoe_fcf_idx;
diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h
index dcae62d4cbeb..7d9a076c4667 100644
--- a/drivers/scsi/qla2xxx/qla_fw.h
+++ b/drivers/scsi/qla2xxx/qla_fw.h
@@ -603,9 +603,14 @@ struct sts_entry_24xx {
 
uint32_t residual_len;  /* FW calc residual transfer length. */
 
-   uint16_t reserved_1;
+   union {
+   uint16_t reserved_1;
+   uint16_t nvme_rsp_pyld_len;
+   };
+
uint16_t state_flags;   /* State flags. */
 #define SF_TRANSFERRED_DATABIT_11
+#define SF_NVME_ERSPBIT_6
 #define SF_FCP_RSP_DMA BIT_0
 
uint16_t retry_delay;
@@ -615,8 +620,25 @@ struct sts_entry_24xx {
uint32_t rsp_residual_count;/* FCP RSP residual count. */
 
uint32_t sense_len; /* FCP SENSE length. */
-   uint32_t rsp_data_len;  /* FCP response data length. */
-   uint8_t data[28];   /* FCP response/sense information. */
+
+   union {
+   struct {
+   uint32_t rsp_data_len;  /* FCP response data length  */
+   uint8_t data[28];   /* FCP rsp/sense information */
+   };
+   struct {
+   /* nvme ersp hdr */
+   __u8status_code;
+   __u8rsvd0;
+   __be16  iu_len;
+   __be32  rsn;
+   __be32  xfrd_len;
+   __be32  rsvd12;
+   uint8_t cqe[16];
+   };
+   uint8_t nvme_ersp_data[32];
+   };
+
/*
 * If DIF Error is set in comp_status, these additional fields are
 * defined:
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 40385bc1d1fa..0d60fd0da604 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1799,6 +1799,86 @@ qla24xx_tm_iocb_entry(scsi_qla_host_t *vha, struct 
req_que *req, void *tsk)
sp->done(sp, 0);
 }
 
+static void
+qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req, void *tsk)
+{
+   const char func[] = "NVME-IOCB";
+   fc_port_t *fcport;
+   srb_t *sp;
+   struct srb_iocb *iocb;
+   struct sts_entry_24xx *sts = (struct sts_entry_24xx *)tsk;
+   uint16_tstate_flags;
+   struct nvmefc_fcp_req *fd;
+   struct srb_iocb *nvme;
+
+   sp = qla2x00_get_sp_from_handle(vha, func, req, tsk);
+   if (!sp)
+   return;
+
+   iocb = >u.iocb_cmd;
+   fcport = sp->fcport;
+   iocb->u.nvme.comp_status = le16_to_cpu(sts->comp_status);
+   state_flags  = le16_to_cpu(sts->state_flags);
+   fd = iocb->u.nvme.desc;
+   nvme = >u.iocb_cmd;
+
+   if (unlikely(nvme->u.nvme.aen_op)) {
+   atomic_dec(>vha->nvme_active_aen_cnt);
+