diff mbox series

[2/2] qla2xxx: allow 32 bytes CDB

Message ID 20230804071944.27214-3-njavali@marvell.com (mailing list archive)
State Superseded
Headers show
Series qla2xxx: allow 32 bytes CDB | expand

Commit Message

Nilesh Javali Aug. 4, 2023, 7:19 a.m. UTC
From: Quinn Tran <qutran@marvell.com>

System crash when 32 bytes CDB was sent to a non T10-PI disk,

[  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0 [qla2xxx]
[  177.149165]  ? internal_add_timer+0x42/0x70
[  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
[  177.158730]  scsi_queue_rq+0x2b7/0xc00
[  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0

Current code attempt to use CRC IOCB to send the command but failed.
Instead, type 6 IOCB should be used to send the IO.

Clone existing type 6 IOCB code with addition of MQ support
to allow 32 bytes CDB to go through.

Signed-off-by: Quinn Tran <qutran@marvell.com>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_iocb.c | 270 ++++++++++++++++++++++++++++++++
 drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
 2 files changed, 273 insertions(+), 1 deletion(-)

Comments

Laurence Oberman Aug. 4, 2023, 4:58 p.m. UTC | #1
On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> System crash when 32 bytes CDB was sent to a non T10-PI disk,
> 
> [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0 [qla2xxx]
> [  177.149165]  ? internal_add_timer+0x42/0x70
> [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> 
> Current code attempt to use CRC IOCB to send the command but failed.
> Instead, type 6 IOCB should be used to send the IO.
> 
> Clone existing type 6 IOCB code with addition of MQ support
> to allow 32 bytes CDB to go through.
> 
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Cc: Laurence Oberman <loberman@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_iocb.c | 270
> ++++++++++++++++++++++++++++++++
>  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
>  2 files changed, 273 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> b/drivers/scsi/qla2xxx/qla_iocb.c
> index 0caa64a7df26..e99ebf7e1c7a 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -11,6 +11,7 @@
>  
>  #include <scsi/scsi_tcq.h>
>  
> +static int qla_start_scsi_type6(srb_t *sp);
>  /**
>   * qla2x00_get_cmd_direction() - Determine control_flag data
> direction.
>   * @sp: SCSI command
> @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
>         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
>                 if (cmd->cmd_len <= 16)
>                         return qla24xx_start_scsi(sp);
> +               else
> +                       return qla_start_scsi_type6(sp);
>         }
>  
>         /* Setup device pointers. */
> @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
>         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
>                 if (cmd->cmd_len <= 16)
>                         return qla2xxx_start_scsi_mq(sp);
> +               else
> +                       return qla_start_scsi_type6(sp);
>         }
>  
>         spin_lock_irqsave(&qpair->qp_lock, flags);
> @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp, struct
> scsi_qla_host *vha, uint32_t tot_dsds)
>  
>         return rval;
>  }
> +
> +/**
> + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> + * @sp: command to send to the ISP
> + *
> + * Returns non-zero if a failure occurred, else zero.
> + */
> +static int
> +qla_start_scsi_type6(srb_t *sp)
> +{
> +       int             nseg;
> +       unsigned long   flags;
> +       uint32_t        *clr_ptr;
> +       uint32_t        handle;
> +       struct cmd_type_6 *cmd_pkt;
> +       uint16_t        cnt;
> +       uint16_t        req_cnt;
> +       uint16_t        tot_dsds;
> +       struct req_que *req = NULL;
> +       struct rsp_que *rsp;
> +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> +       struct scsi_qla_host *vha = sp->fcport->vha;
> +       struct qla_hw_data *ha = vha->hw;
> +       struct qla_qpair *qpair = sp->qpair;
> +       uint16_t more_dsd_lists = 0;
> +       struct dsd_dma *dsd_ptr;
> +       uint16_t i;
> +       __be32 *fcp_dl;
> +       uint8_t additional_cdb_len;
> +       struct ct6_dsd *ctx;
> +
> +
> +       /* Acquire qpair specific lock */
> +       spin_lock_irqsave(&qpair->qp_lock, flags);
> +
> +       /* Setup qpair pointers */
> +       req = qpair->req;
> +       rsp = qpair->rsp;
> +
> +       /* So we know we haven't pci_map'ed anything yet */
> +       tot_dsds = 0;
> +
> +       /* Send marker if required */
> +       if (vha->marker_needed != 0) {
> +               if (__qla2x00_marker(vha, qpair, 0, 0, MK_SYNC_ALL)
> != QLA_SUCCESS) {
> +                       spin_unlock_irqrestore(&qpair->qp_lock,
> flags);
> +                       return QLA_FUNCTION_FAILED;
> +               }
> +               vha->marker_needed = 0;
> +       }
> +
> +       handle = qla2xxx_get_next_handle(req);
> +       if (handle == 0)
> +               goto queuing_error;
> +
> +       /* Map the sg table so we have an accurate count of sg
> entries needed */
> +       if (scsi_sg_count(cmd)) {
> +               nseg = dma_map_sg(&ha->pdev->dev, scsi_sglist(cmd),
> +                                 scsi_sg_count(cmd), cmd-
> >sc_data_direction);
> +               if (unlikely(!nseg))
> +                       goto queuing_error;
> +       } else {
> +               nseg = 0;
> +       }
> +
> +       tot_dsds = nseg;
> +
> +       /* eventhough driver only need 1 T6 IOCB, FW still convert
> DSD to Continueation IOCB */
> +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> +
> +       sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> +       sp->iores.exch_cnt = 1;
> +       sp->iores.iocb_cnt = req_cnt;
> +
> +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> +               goto queuing_error;
> +
> +       more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> +       if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> +               ql_dbg(ql_dbg_io, vha, 0x3028,
> +                      "Num of DSD list %d is than %d for cmd=%p.\n",
> +                      more_dsd_lists + qpair->dsd_inuse,
> NUM_DSD_CHAIN, cmd);
> +               goto queuing_error;
> +       }
> +
> +       if (more_dsd_lists <= qpair->dsd_avail)
> +               goto sufficient_dsds;
> +       else
> +               more_dsd_lists -= qpair->dsd_avail;
> +
> +       for (i = 0; i < more_dsd_lists; i++) {
> +               dsd_ptr = kzalloc(sizeof(*dsd_ptr), GFP_ATOMIC);
> +               if (!dsd_ptr) {
> +                       ql_log(ql_log_fatal, vha, 0x3029,
> +                           "Failed to allocate memory for dsd_dma
> for cmd=%p.\n", cmd);
> +                       goto queuing_error;
> +               }
> +               INIT_LIST_HEAD(&dsd_ptr->list);
> +
> +               dsd_ptr->dsd_addr = dma_pool_alloc(ha->dl_dma_pool,
> +                       GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
> +               if (!dsd_ptr->dsd_addr) {
> +                       kfree(dsd_ptr);
> +                       ql_log(ql_log_fatal, vha, 0x302a,
> +                           "Failed to allocate memory for dsd_addr
> for cmd=%p.\n", cmd);
> +                       goto queuing_error;
> +               }
> +               list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> +               qpair->dsd_avail++;
> +       }
> +
> +sufficient_dsds:
> +       req_cnt = 1;
> +
> +       if (req->cnt < (req_cnt + 2)) {
> +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> +                       cnt = *req->out_ptr;
> +               } else {
> +                       cnt = (uint16_t)rd_reg_dword_relaxed(req-
> >req_q_out);
> +                       if (qla2x00_check_reg16_for_disconnect(vha,
> cnt))
> +                               goto queuing_error;
> +               }
> +
> +               if (req->ring_index < cnt)
> +                       req->cnt = cnt - req->ring_index;
> +               else
> +                       req->cnt = req->length - (req->ring_index -
> cnt);
> +               if (req->cnt < (req_cnt + 2))
> +                       goto queuing_error;
> +       }
> +
> +       ctx = &sp->u.scmd.ct6_ctx;
> +
> +       memset(ctx, 0, sizeof(struct ct6_dsd));
> +       ctx->fcp_cmnd = dma_pool_zalloc(ha->fcp_cmnd_dma_pool,
> +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> +       if (!ctx->fcp_cmnd) {
> +               ql_log(ql_log_fatal, vha, 0x3031,
> +                   "Failed to allocate fcp_cmnd for cmd=%p.\n",
> cmd);
> +               goto queuing_error;
> +       }
> +
> +       /* Initialize the DSD list and dma handle */
> +       INIT_LIST_HEAD(&ctx->dsd_list);
> +       ctx->dsd_use_cnt = 0;
> +
> +       if (cmd->cmd_len > 16) {
> +               additional_cdb_len = cmd->cmd_len - 16;
> +               if (cmd->cmd_len % 4 ||
> +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> +                       /* SCSI command bigger than 16 bytes must be
> +                        * multiple of 4 or too big.
> +                        */
> +                       ql_log(ql_log_warn, vha, 0x3033,
> +                           "scsi cmd len %d not multiple of 4 for
> cmd=%p.\n",
> +                           cmd->cmd_len, cmd);
> +                       goto queuing_error_fcp_cmnd;
> +               }
> +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> +       } else {
> +               additional_cdb_len = 0;
> +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> +       }
> +
> +       /* Build command packet. */
> +       req->current_outstanding_cmd = handle;
> +       req->outstanding_cmds[handle] = sp;
> +       sp->handle = handle;
> +       cmd->host_scribble = (unsigned char *)(unsigned long)handle;
> +       req->cnt -= req_cnt;
> +
> +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> +       cmd_pkt->handle = make_handle(req->id, handle);
> +
> +       /* Zero out remaining portion of packet. */
> +       /*    tagged queuing modifier -- default is TSK_SIMPLE (0).
> */
> +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> +
> +       /* Set NPORT-ID and LUN number*/
> +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport->loop_id);
> +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> +       cmd_pkt->vp_index = sp->vha->vp_idx;
> +
> +       /* Build IOCB segments */
> +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt, tot_dsds);
> +
> +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun, sizeof(cmd_pkt-
> >lun));
> +
> +       /* build FCP_CMND IU */
> +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd->lun);
> +       ctx->fcp_cmnd->additional_cdb_len = additional_cdb_len;
> +
> +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> +       else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> +
> +       /* Populate the FCP_PRIO. */
> +       if (ha->flags.fcp_prio_enabled)
> +               ctx->fcp_cmnd->task_attribute |=
> +                   sp->fcport->fcp_prio << 3;
> +
> +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
> +
> +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> +           additional_cdb_len);
> +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> +
> +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx->fcp_cmnd_len);
> +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> +                          &cmd_pkt->fcp_cmnd_dseg_address);
> +
> +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> +       cmd_pkt->byte_count =
> cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> +       /* Set total data segment count. */
> +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> +
> +       wmb();
> +       /* Adjust ring index. */
> +       req->ring_index++;
> +       if (req->ring_index == req->length) {
> +               req->ring_index = 0;
> +               req->ring_ptr = req->ring;
> +       } else {
> +               req->ring_ptr++;
> +       }
> +
> +       sp->qpair->cmd_cnt++;
> +       sp->flags |= SRB_DMA_VALID;
> +
> +       /* Set chip new ring index. */
> +       wrt_reg_dword(req->req_q_in, req->ring_index);
> +
> +       /* Manage unprocessed RIO/ZIO commands in response queue. */
> +       if (vha->flags.process_response_queue &&
> +           rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> +               qla24xx_process_response_queue(vha, rsp);
> +
> +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> +
> +       return QLA_SUCCESS;
> +
> +queuing_error_fcp_cmnd:
> +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd, ctx-
> >fcp_cmnd_dma);
> +
> +queuing_error:
> +       if (tot_dsds)
> +               scsi_dma_unmap(cmd);
> +
> +       qla_put_fw_resources(sp->qpair, &sp->iores);
> +
> +       if (sp->u.scmd.crc_ctx) {
> +               mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);
> +               sp->u.scmd.crc_ctx = NULL;
> +       }
> +
> +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> +
> +       return QLA_FUNCTION_FAILED;
> +}
> diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> b/drivers/scsi/qla2xxx/qla_nx.h
> index 6dc80c8ddf79..5d1bdc15b75c 100644
> --- a/drivers/scsi/qla2xxx/qla_nx.h
> +++ b/drivers/scsi/qla2xxx/qla_nx.h
> @@ -857,7 +857,9 @@ struct fcp_cmnd {
>         uint8_t task_attribute;
>         uint8_t task_management;
>         uint8_t additional_cdb_len;
> -       uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL */
> +#define QLA_CDB_BUF_SIZE  256
> +#define QLA_FCP_DL_SIZE   4
> +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /* 256 for
> CDB len and 4 for FCP_DL */
>  };
>  
>  struct dsd_dma {

QT and Nilesh, Thank you.
I need more time to look at this fully but I will get it tested in the
meantime. 
I will reply with a review or questions later.

Thanks for sending this
Laurence
Laurence Oberman Aug. 4, 2023, 5:51 p.m. UTC | #2
On Fri, 2023-08-04 at 12:58 -0400, Laurence Oberman wrote:
> On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> > From: Quinn Tran <qutran@marvell.com>
> > 
> > System crash when 32 bytes CDB was sent to a non T10-PI disk,
> > 
> > [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0 [qla2xxx]
> > [  177.149165]  ? internal_add_timer+0x42/0x70
> > [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> > [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> > [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> > 
> > Current code attempt to use CRC IOCB to send the command but
> > failed.
> > Instead, type 6 IOCB should be used to send the IO.
> > 
> > Clone existing type 6 IOCB code with addition of MQ support
> > to allow 32 bytes CDB to go through.
> > 
> > Signed-off-by: Quinn Tran <qutran@marvell.com>
> > Cc: Laurence Oberman <loberman@redhat.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > ---
> >  drivers/scsi/qla2xxx/qla_iocb.c | 270
> > ++++++++++++++++++++++++++++++++
> >  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
> >  2 files changed, 273 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> > b/drivers/scsi/qla2xxx/qla_iocb.c
> > index 0caa64a7df26..e99ebf7e1c7a 100644
> > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include <scsi/scsi_tcq.h>
> >  
> > +static int qla_start_scsi_type6(srb_t *sp);
> >  /**
> >   * qla2x00_get_cmd_direction() - Determine control_flag data
> > direction.
> >   * @sp: SCSI command
> > @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
> >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> >                 if (cmd->cmd_len <= 16)
> >                         return qla24xx_start_scsi(sp);
> > +               else
> > +                       return qla_start_scsi_type6(sp);
> >         }
> >  
> >         /* Setup device pointers. */
> > @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
> >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> >                 if (cmd->cmd_len <= 16)
> >                         return qla2xxx_start_scsi_mq(sp);
> > +               else
> > +                       return qla_start_scsi_type6(sp);
> >         }
> >  
> >         spin_lock_irqsave(&qpair->qp_lock, flags);
> > @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp, struct
> > scsi_qla_host *vha, uint32_t tot_dsds)
> >  
> >         return rval;
> >  }
> > +
> > +/**
> > + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> > + * @sp: command to send to the ISP
> > + *
> > + * Returns non-zero if a failure occurred, else zero.
> > + */
> > +static int
> > +qla_start_scsi_type6(srb_t *sp)
> > +{
> > +       int             nseg;
> > +       unsigned long   flags;
> > +       uint32_t        *clr_ptr;
> > +       uint32_t        handle;
> > +       struct cmd_type_6 *cmd_pkt;
> > +       uint16_t        cnt;
> > +       uint16_t        req_cnt;
> > +       uint16_t        tot_dsds;
> > +       struct req_que *req = NULL;
> > +       struct rsp_que *rsp;
> > +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> > +       struct scsi_qla_host *vha = sp->fcport->vha;
> > +       struct qla_hw_data *ha = vha->hw;
> > +       struct qla_qpair *qpair = sp->qpair;
> > +       uint16_t more_dsd_lists = 0;
> > +       struct dsd_dma *dsd_ptr;
> > +       uint16_t i;
> > +       __be32 *fcp_dl;
> > +       uint8_t additional_cdb_len;
> > +       struct ct6_dsd *ctx;
> > +
> > +
> > +       /* Acquire qpair specific lock */
> > +       spin_lock_irqsave(&qpair->qp_lock, flags);
> > +
> > +       /* Setup qpair pointers */
> > +       req = qpair->req;
> > +       rsp = qpair->rsp;
> > +
> > +       /* So we know we haven't pci_map'ed anything yet */
> > +       tot_dsds = 0;
> > +
> > +       /* Send marker if required */
> > +       if (vha->marker_needed != 0) {
> > +               if (__qla2x00_marker(vha, qpair, 0, 0, MK_SYNC_ALL)
> > != QLA_SUCCESS) {
> > +                       spin_unlock_irqrestore(&qpair->qp_lock,
> > flags);
> > +                       return QLA_FUNCTION_FAILED;
> > +               }
> > +               vha->marker_needed = 0;
> > +       }
> > +
> > +       handle = qla2xxx_get_next_handle(req);
> > +       if (handle == 0)
> > +               goto queuing_error;
> > +
> > +       /* Map the sg table so we have an accurate count of sg
> > entries needed */
> > +       if (scsi_sg_count(cmd)) {
> > +               nseg = dma_map_sg(&ha->pdev->dev, scsi_sglist(cmd),
> > +                                 scsi_sg_count(cmd), cmd-
> > > sc_data_direction);
> > +               if (unlikely(!nseg))
> > +                       goto queuing_error;
> > +       } else {
> > +               nseg = 0;
> > +       }
> > +
> > +       tot_dsds = nseg;
> > +
> > +       /* eventhough driver only need 1 T6 IOCB, FW still convert
> > DSD to Continueation IOCB */
> > +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> > +
> > +       sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> > +       sp->iores.exch_cnt = 1;
> > +       sp->iores.iocb_cnt = req_cnt;
> > +
> > +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> > +               goto queuing_error;
> > +
> > +       more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> > +       if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> > +               ql_dbg(ql_dbg_io, vha, 0x3028,
> > +                      "Num of DSD list %d is than %d for
> > cmd=%p.\n",
> > +                      more_dsd_lists + qpair->dsd_inuse,
> > NUM_DSD_CHAIN, cmd);
> > +               goto queuing_error;
> > +       }
> > +
> > +       if (more_dsd_lists <= qpair->dsd_avail)
> > +               goto sufficient_dsds;
> > +       else
> > +               more_dsd_lists -= qpair->dsd_avail;
> > +
> > +       for (i = 0; i < more_dsd_lists; i++) {
> > +               dsd_ptr = kzalloc(sizeof(*dsd_ptr), GFP_ATOMIC);
> > +               if (!dsd_ptr) {
> > +                       ql_log(ql_log_fatal, vha, 0x3029,
> > +                           "Failed to allocate memory for dsd_dma
> > for cmd=%p.\n", cmd);
> > +                       goto queuing_error;
> > +               }
> > +               INIT_LIST_HEAD(&dsd_ptr->list);
> > +
> > +               dsd_ptr->dsd_addr = dma_pool_alloc(ha->dl_dma_pool,
> > +                       GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
> > +               if (!dsd_ptr->dsd_addr) {
> > +                       kfree(dsd_ptr);
> > +                       ql_log(ql_log_fatal, vha, 0x302a,
> > +                           "Failed to allocate memory for dsd_addr
> > for cmd=%p.\n", cmd);
> > +                       goto queuing_error;
> > +               }
> > +               list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> > +               qpair->dsd_avail++;
> > +       }
> > +
> > +sufficient_dsds:
> > +       req_cnt = 1;
> > +
> > +       if (req->cnt < (req_cnt + 2)) {
> > +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> > +                       cnt = *req->out_ptr;
> > +               } else {
> > +                       cnt = (uint16_t)rd_reg_dword_relaxed(req-
> > > req_q_out);
> > +                       if (qla2x00_check_reg16_for_disconnect(vha,
> > cnt))
> > +                               goto queuing_error;
> > +               }
> > +
> > +               if (req->ring_index < cnt)
> > +                       req->cnt = cnt - req->ring_index;
> > +               else
> > +                       req->cnt = req->length - (req->ring_index -
> > cnt);
> > +               if (req->cnt < (req_cnt + 2))
> > +                       goto queuing_error;
> > +       }
> > +
> > +       ctx = &sp->u.scmd.ct6_ctx;
> > +
> > +       memset(ctx, 0, sizeof(struct ct6_dsd));
> > +       ctx->fcp_cmnd = dma_pool_zalloc(ha->fcp_cmnd_dma_pool,
> > +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> > +       if (!ctx->fcp_cmnd) {
> > +               ql_log(ql_log_fatal, vha, 0x3031,
> > +                   "Failed to allocate fcp_cmnd for cmd=%p.\n",
> > cmd);
> > +               goto queuing_error;
> > +       }
> > +
> > +       /* Initialize the DSD list and dma handle */
> > +       INIT_LIST_HEAD(&ctx->dsd_list);
> > +       ctx->dsd_use_cnt = 0;
> > +
> > +       if (cmd->cmd_len > 16) {
> > +               additional_cdb_len = cmd->cmd_len - 16;
> > +               if (cmd->cmd_len % 4 ||
> > +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> > +                       /* SCSI command bigger than 16 bytes must
> > be
> > +                        * multiple of 4 or too big.
> > +                        */
> > +                       ql_log(ql_log_warn, vha, 0x3033,
> > +                           "scsi cmd len %d not multiple of 4 for
> > cmd=%p.\n",
> > +                           cmd->cmd_len, cmd);
> > +                       goto queuing_error_fcp_cmnd;
> > +               }
> > +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> > +       } else {
> > +               additional_cdb_len = 0;
> > +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> > +       }
> > +
> > +       /* Build command packet. */
> > +       req->current_outstanding_cmd = handle;
> > +       req->outstanding_cmds[handle] = sp;
> > +       sp->handle = handle;
> > +       cmd->host_scribble = (unsigned char *)(unsigned
> > long)handle;
> > +       req->cnt -= req_cnt;
> > +
> > +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> > +       cmd_pkt->handle = make_handle(req->id, handle);
> > +
> > +       /* Zero out remaining portion of packet. */
> > +       /*    tagged queuing modifier -- default is TSK_SIMPLE (0).
> > */
> > +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> > +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> > +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> > +
> > +       /* Set NPORT-ID and LUN number*/
> > +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport->loop_id);
> > +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> > +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> > +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> > +       cmd_pkt->vp_index = sp->vha->vp_idx;
> > +
> > +       /* Build IOCB segments */
> > +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt, tot_dsds);
> > +
> > +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> > +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun, sizeof(cmd_pkt-
> > > lun));
> > +
> > +       /* build FCP_CMND IU */
> > +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd->lun);
> > +       ctx->fcp_cmnd->additional_cdb_len = additional_cdb_len;
> > +
> > +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> > +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> > +       else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> > +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> > +
> > +       /* Populate the FCP_PRIO. */
> > +       if (ha->flags.fcp_prio_enabled)
> > +               ctx->fcp_cmnd->task_attribute |=
> > +                   sp->fcport->fcp_prio << 3;
> > +
> > +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
> > +
> > +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> > +           additional_cdb_len);
> > +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> > +
> > +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx-
> > >fcp_cmnd_len);
> > +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> > +                          &cmd_pkt->fcp_cmnd_dseg_address);
> > +
> > +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> > +       cmd_pkt->byte_count =
> > cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> > +       /* Set total data segment count. */
> > +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> > +
> > +       wmb();
> > +       /* Adjust ring index. */
> > +       req->ring_index++;
> > +       if (req->ring_index == req->length) {
> > +               req->ring_index = 0;
> > +               req->ring_ptr = req->ring;
> > +       } else {
> > +               req->ring_ptr++;
> > +       }
> > +
> > +       sp->qpair->cmd_cnt++;
> > +       sp->flags |= SRB_DMA_VALID;
> > +
> > +       /* Set chip new ring index. */
> > +       wrt_reg_dword(req->req_q_in, req->ring_index);
> > +
> > +       /* Manage unprocessed RIO/ZIO commands in response queue.
> > */
> > +       if (vha->flags.process_response_queue &&
> > +           rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> > +               qla24xx_process_response_queue(vha, rsp);
> > +
> > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > +
> > +       return QLA_SUCCESS;
> > +
> > +queuing_error_fcp_cmnd:
> > +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd, ctx-
> > > fcp_cmnd_dma);
> > +
> > +queuing_error:
> > +       if (tot_dsds)
> > +               scsi_dma_unmap(cmd);
> > +
> > +       qla_put_fw_resources(sp->qpair, &sp->iores);
> > +
> > +       if (sp->u.scmd.crc_ctx) {
> > +               mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);
> > +               sp->u.scmd.crc_ctx = NULL;
> > +       }
> > +
> > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > +
> > +       return QLA_FUNCTION_FAILED;
> > +}
> > diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> > b/drivers/scsi/qla2xxx/qla_nx.h
> > index 6dc80c8ddf79..5d1bdc15b75c 100644
> > --- a/drivers/scsi/qla2xxx/qla_nx.h
> > +++ b/drivers/scsi/qla2xxx/qla_nx.h
> > @@ -857,7 +857,9 @@ struct fcp_cmnd {
> >         uint8_t task_attribute;
> >         uint8_t task_management;
> >         uint8_t additional_cdb_len;
> > -       uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL */
> > +#define QLA_CDB_BUF_SIZE  256
> > +#define QLA_FCP_DL_SIZE   4
> > +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /* 256 for
> > CDB len and 4 for FCP_DL */
> >  };
> >  
> >  struct dsd_dma {
> 
> QT and Nilesh, Thank you.
> I need more time to look at this fully but I will get it tested in
> the
> meantime. 
> I will reply with a review or questions later.
> 
> Thanks for sending this
> Laurence

@QT and Nilesh
Which kernel was this against.
Did you guys test this only agains your driver or also upstream
I cannot build it clean because There is no structure member dsd_inuse

Can you send one for upstream please

Patch applied fine to 6.5-rc4

[loberman@segstorage3 linux]$ patch -p1 < qt.patch 
patching file drivers/scsi/qla2xxx/qla_iocb.c
Hunk #2 succeeded at 1718 (offset -4 lines).
Hunk #3 succeeded at 2099 (offset -4 lines).
Hunk #4 succeeded at 4179 (offset -31 lines).
patching file drivers/scsi/qla2xxx/qla_nx.h


Build fails

drivers/scsi/qla2xxx/qla_iocb.c: In function ‘qla_start_scsi_type6’:
drivers/scsi/qla2xxx/qla_iocb.c:4260:29: error: ‘struct qla_qpair’ has
no member named ‘dsd_inuse’
  if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
                             ^~
drivers/scsi/qla2xxx/qla_iocb.c:4263:32: error: ‘struct qla_qpair’ has
no member named ‘dsd_inuse’
          more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN, cmd);
                                ^~
drivers/scsi/qla2xxx/qla_iocb.c:4267:29: error: ‘struct qla_qpair’ has
no member named ‘dsd_avail’
  if (more_dsd_lists <= qpair->dsd_avail)
                             ^~
drivers/scsi/qla2xxx/qla_iocb.c:4270:26: error: ‘struct qla_qpair’ has
no member named ‘dsd_avail’
   more_dsd_lists -= qpair->dsd_avail;
                          ^~
drivers/scsi/qla2xxx/qla_iocb.c:4289:41: error: ‘struct qla_qpair’ has
no member named ‘dsd_list’; did you mean ‘hints_list’?
   list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
                                         ^~~~~~~~
                                         hints_list
drivers/scsi/qla2xxx/qla_iocb.c:4290:8: error: ‘struct qla_qpair’ has
no member named ‘dsd_avail’
   qpair->dsd_avail++;
Laurence Oberman Aug. 4, 2023, 6:35 p.m. UTC | #3
On Fri, 2023-08-04 at 13:51 -0400, Laurence Oberman wrote:
> On Fri, 2023-08-04 at 12:58 -0400, Laurence Oberman wrote:
> > On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> > > From: Quinn Tran <qutran@marvell.com>
> > > 
> > > System crash when 32 bytes CDB was sent to a non T10-PI disk,
> > > 
> > > [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0 [qla2xxx]
> > > [  177.149165]  ? internal_add_timer+0x42/0x70
> > > [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> > > [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> > > [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> > > 
> > > Current code attempt to use CRC IOCB to send the command but
> > > failed.
> > > Instead, type 6 IOCB should be used to send the IO.
> > > 
> > > Clone existing type 6 IOCB code with addition of MQ support
> > > to allow 32 bytes CDB to go through.
> > > 
> > > Signed-off-by: Quinn Tran <qutran@marvell.com>
> > > Cc: Laurence Oberman <loberman@redhat.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > > ---
> > >  drivers/scsi/qla2xxx/qla_iocb.c | 270
> > > ++++++++++++++++++++++++++++++++
> > >  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
> > >  2 files changed, 273 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> > > b/drivers/scsi/qla2xxx/qla_iocb.c
> > > index 0caa64a7df26..e99ebf7e1c7a 100644
> > > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > > @@ -11,6 +11,7 @@
> > >  
> > >  #include <scsi/scsi_tcq.h>
> > >  
> > > +static int qla_start_scsi_type6(srb_t *sp);
> > >  /**
> > >   * qla2x00_get_cmd_direction() - Determine control_flag data
> > > direction.
> > >   * @sp: SCSI command
> > > @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
> > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > >                 if (cmd->cmd_len <= 16)
> > >                         return qla24xx_start_scsi(sp);
> > > +               else
> > > +                       return qla_start_scsi_type6(sp);
> > >         }
> > >  
> > >         /* Setup device pointers. */
> > > @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
> > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > >                 if (cmd->cmd_len <= 16)
> > >                         return qla2xxx_start_scsi_mq(sp);
> > > +               else
> > > +                       return qla_start_scsi_type6(sp);
> > >         }
> > >  
> > >         spin_lock_irqsave(&qpair->qp_lock, flags);
> > > @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp, struct
> > > scsi_qla_host *vha, uint32_t tot_dsds)
> > >  
> > >         return rval;
> > >  }
> > > +
> > > +/**
> > > + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> > > + * @sp: command to send to the ISP
> > > + *
> > > + * Returns non-zero if a failure occurred, else zero.
> > > + */
> > > +static int
> > > +qla_start_scsi_type6(srb_t *sp)
> > > +{
> > > +       int             nseg;
> > > +       unsigned long   flags;
> > > +       uint32_t        *clr_ptr;
> > > +       uint32_t        handle;
> > > +       struct cmd_type_6 *cmd_pkt;
> > > +       uint16_t        cnt;
> > > +       uint16_t        req_cnt;
> > > +       uint16_t        tot_dsds;
> > > +       struct req_que *req = NULL;
> > > +       struct rsp_que *rsp;
> > > +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> > > +       struct scsi_qla_host *vha = sp->fcport->vha;
> > > +       struct qla_hw_data *ha = vha->hw;
> > > +       struct qla_qpair *qpair = sp->qpair;
> > > +       uint16_t more_dsd_lists = 0;
> > > +       struct dsd_dma *dsd_ptr;
> > > +       uint16_t i;
> > > +       __be32 *fcp_dl;
> > > +       uint8_t additional_cdb_len;
> > > +       struct ct6_dsd *ctx;
> > > +
> > > +
> > > +       /* Acquire qpair specific lock */
> > > +       spin_lock_irqsave(&qpair->qp_lock, flags);
> > > +
> > > +       /* Setup qpair pointers */
> > > +       req = qpair->req;
> > > +       rsp = qpair->rsp;
> > > +
> > > +       /* So we know we haven't pci_map'ed anything yet */
> > > +       tot_dsds = 0;
> > > +
> > > +       /* Send marker if required */
> > > +       if (vha->marker_needed != 0) {
> > > +               if (__qla2x00_marker(vha, qpair, 0, 0,
> > > MK_SYNC_ALL)
> > > != QLA_SUCCESS) {
> > > +                       spin_unlock_irqrestore(&qpair->qp_lock,
> > > flags);
> > > +                       return QLA_FUNCTION_FAILED;
> > > +               }
> > > +               vha->marker_needed = 0;
> > > +       }
> > > +
> > > +       handle = qla2xxx_get_next_handle(req);
> > > +       if (handle == 0)
> > > +               goto queuing_error;
> > > +
> > > +       /* Map the sg table so we have an accurate count of sg
> > > entries needed */
> > > +       if (scsi_sg_count(cmd)) {
> > > +               nseg = dma_map_sg(&ha->pdev->dev,
> > > scsi_sglist(cmd),
> > > +                                 scsi_sg_count(cmd), cmd-
> > > > sc_data_direction);
> > > +               if (unlikely(!nseg))
> > > +                       goto queuing_error;
> > > +       } else {
> > > +               nseg = 0;
> > > +       }
> > > +
> > > +       tot_dsds = nseg;
> > > +
> > > +       /* eventhough driver only need 1 T6 IOCB, FW still
> > > convert
> > > DSD to Continueation IOCB */
> > > +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> > > +
> > > +       sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> > > +       sp->iores.exch_cnt = 1;
> > > +       sp->iores.iocb_cnt = req_cnt;
> > > +
> > > +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> > > +               goto queuing_error;
> > > +
> > > +       more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> > > +       if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN)
> > > {
> > > +               ql_dbg(ql_dbg_io, vha, 0x3028,
> > > +                      "Num of DSD list %d is than %d for
> > > cmd=%p.\n",
> > > +                      more_dsd_lists + qpair->dsd_inuse,
> > > NUM_DSD_CHAIN, cmd);
> > > +               goto queuing_error;
> > > +       }
> > > +
> > > +       if (more_dsd_lists <= qpair->dsd_avail)
> > > +               goto sufficient_dsds;
> > > +       else
> > > +               more_dsd_lists -= qpair->dsd_avail;
> > > +
> > > +       for (i = 0; i < more_dsd_lists; i++) {
> > > +               dsd_ptr = kzalloc(sizeof(*dsd_ptr), GFP_ATOMIC);
> > > +               if (!dsd_ptr) {
> > > +                       ql_log(ql_log_fatal, vha, 0x3029,
> > > +                           "Failed to allocate memory for
> > > dsd_dma
> > > for cmd=%p.\n", cmd);
> > > +                       goto queuing_error;
> > > +               }
> > > +               INIT_LIST_HEAD(&dsd_ptr->list);
> > > +
> > > +               dsd_ptr->dsd_addr = dma_pool_alloc(ha-
> > > >dl_dma_pool,
> > > +                       GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
> > > +               if (!dsd_ptr->dsd_addr) {
> > > +                       kfree(dsd_ptr);
> > > +                       ql_log(ql_log_fatal, vha, 0x302a,
> > > +                           "Failed to allocate memory for
> > > dsd_addr
> > > for cmd=%p.\n", cmd);
> > > +                       goto queuing_error;
> > > +               }
> > > +               list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> > > +               qpair->dsd_avail++;
> > > +       }
> > > +
> > > +sufficient_dsds:
> > > +       req_cnt = 1;
> > > +
> > > +       if (req->cnt < (req_cnt + 2)) {
> > > +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> > > +                       cnt = *req->out_ptr;
> > > +               } else {
> > > +                       cnt = (uint16_t)rd_reg_dword_relaxed(req-
> > > > req_q_out);
> > > +                       if
> > > (qla2x00_check_reg16_for_disconnect(vha,
> > > cnt))
> > > +                               goto queuing_error;
> > > +               }
> > > +
> > > +               if (req->ring_index < cnt)
> > > +                       req->cnt = cnt - req->ring_index;
> > > +               else
> > > +                       req->cnt = req->length - (req->ring_index
> > > -
> > > cnt);
> > > +               if (req->cnt < (req_cnt + 2))
> > > +                       goto queuing_error;
> > > +       }
> > > +
> > > +       ctx = &sp->u.scmd.ct6_ctx;
> > > +
> > > +       memset(ctx, 0, sizeof(struct ct6_dsd));
> > > +       ctx->fcp_cmnd = dma_pool_zalloc(ha->fcp_cmnd_dma_pool,
> > > +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> > > +       if (!ctx->fcp_cmnd) {
> > > +               ql_log(ql_log_fatal, vha, 0x3031,
> > > +                   "Failed to allocate fcp_cmnd for cmd=%p.\n",
> > > cmd);
> > > +               goto queuing_error;
> > > +       }
> > > +
> > > +       /* Initialize the DSD list and dma handle */
> > > +       INIT_LIST_HEAD(&ctx->dsd_list);
> > > +       ctx->dsd_use_cnt = 0;
> > > +
> > > +       if (cmd->cmd_len > 16) {
> > > +               additional_cdb_len = cmd->cmd_len - 16;
> > > +               if (cmd->cmd_len % 4 ||
> > > +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> > > +                       /* SCSI command bigger than 16 bytes must
> > > be
> > > +                        * multiple of 4 or too big.
> > > +                        */
> > > +                       ql_log(ql_log_warn, vha, 0x3033,
> > > +                           "scsi cmd len %d not multiple of 4
> > > for
> > > cmd=%p.\n",
> > > +                           cmd->cmd_len, cmd);
> > > +                       goto queuing_error_fcp_cmnd;
> > > +               }
> > > +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> > > +       } else {
> > > +               additional_cdb_len = 0;
> > > +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> > > +       }
> > > +
> > > +       /* Build command packet. */
> > > +       req->current_outstanding_cmd = handle;
> > > +       req->outstanding_cmds[handle] = sp;
> > > +       sp->handle = handle;
> > > +       cmd->host_scribble = (unsigned char *)(unsigned
> > > long)handle;
> > > +       req->cnt -= req_cnt;
> > > +
> > > +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> > > +       cmd_pkt->handle = make_handle(req->id, handle);
> > > +
> > > +       /* Zero out remaining portion of packet. */
> > > +       /*    tagged queuing modifier -- default is TSK_SIMPLE
> > > (0).
> > > */
> > > +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> > > +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> > > +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> > > +
> > > +       /* Set NPORT-ID and LUN number*/
> > > +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport->loop_id);
> > > +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> > > +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> > > +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> > > +       cmd_pkt->vp_index = sp->vha->vp_idx;
> > > +
> > > +       /* Build IOCB segments */
> > > +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt, tot_dsds);
> > > +
> > > +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> > > +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun,
> > > sizeof(cmd_pkt-
> > > > lun));
> > > +
> > > +       /* build FCP_CMND IU */
> > > +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd->lun);
> > > +       ctx->fcp_cmnd->additional_cdb_len = additional_cdb_len;
> > > +
> > > +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> > > +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> > > +       else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> > > +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> > > +
> > > +       /* Populate the FCP_PRIO. */
> > > +       if (ha->flags.fcp_prio_enabled)
> > > +               ctx->fcp_cmnd->task_attribute |=
> > > +                   sp->fcport->fcp_prio << 3;
> > > +
> > > +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
> > > +
> > > +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> > > +           additional_cdb_len);
> > > +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> > > +
> > > +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx-
> > > > fcp_cmnd_len);
> > > +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> > > +                          &cmd_pkt->fcp_cmnd_dseg_address);
> > > +
> > > +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> > > +       cmd_pkt->byte_count =
> > > cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> > > +       /* Set total data segment count. */
> > > +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> > > +
> > > +       wmb();
> > > +       /* Adjust ring index. */
> > > +       req->ring_index++;
> > > +       if (req->ring_index == req->length) {
> > > +               req->ring_index = 0;
> > > +               req->ring_ptr = req->ring;
> > > +       } else {
> > > +               req->ring_ptr++;
> > > +       }
> > > +
> > > +       sp->qpair->cmd_cnt++;
> > > +       sp->flags |= SRB_DMA_VALID;
> > > +
> > > +       /* Set chip new ring index. */
> > > +       wrt_reg_dword(req->req_q_in, req->ring_index);
> > > +
> > > +       /* Manage unprocessed RIO/ZIO commands in response queue.
> > > */
> > > +       if (vha->flags.process_response_queue &&
> > > +           rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> > > +               qla24xx_process_response_queue(vha, rsp);
> > > +
> > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > +
> > > +       return QLA_SUCCESS;
> > > +
> > > +queuing_error_fcp_cmnd:
> > > +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd, ctx-
> > > > fcp_cmnd_dma);
> > > +
> > > +queuing_error:
> > > +       if (tot_dsds)
> > > +               scsi_dma_unmap(cmd);
> > > +
> > > +       qla_put_fw_resources(sp->qpair, &sp->iores);
> > > +
> > > +       if (sp->u.scmd.crc_ctx) {
> > > +               mempool_free(sp->u.scmd.crc_ctx, ha-
> > > >ctx_mempool);
> > > +               sp->u.scmd.crc_ctx = NULL;
> > > +       }
> > > +
> > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > +
> > > +       return QLA_FUNCTION_FAILED;
> > > +}
> > > diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> > > b/drivers/scsi/qla2xxx/qla_nx.h
> > > index 6dc80c8ddf79..5d1bdc15b75c 100644
> > > --- a/drivers/scsi/qla2xxx/qla_nx.h
> > > +++ b/drivers/scsi/qla2xxx/qla_nx.h
> > > @@ -857,7 +857,9 @@ struct fcp_cmnd {
> > >         uint8_t task_attribute;
> > >         uint8_t task_management;
> > >         uint8_t additional_cdb_len;
> > > -       uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL */
> > > +#define QLA_CDB_BUF_SIZE  256
> > > +#define QLA_FCP_DL_SIZE   4
> > > +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /* 256
> > > for
> > > CDB len and 4 for FCP_DL */
> > >  };
> > >  
> > >  struct dsd_dma {
> > 
> > QT and Nilesh, Thank you.
> > I need more time to look at this fully but I will get it tested in
> > the
> > meantime. 
> > I will reply with a review or questions later.
> > 
> > Thanks for sending this
> > Laurence
> 
> @QT and Nilesh
> Which kernel was this against.
> Did you guys test this only agains your driver or also upstream
> I cannot build it clean because There is no structure member
> dsd_inuse
> 
> Can you send one for upstream please
> 
> Patch applied fine to 6.5-rc4
> 
> [loberman@segstorage3 linux]$ patch -p1 < qt.patch 
> patching file drivers/scsi/qla2xxx/qla_iocb.c
> Hunk #2 succeeded at 1718 (offset -4 lines).
> Hunk #3 succeeded at 2099 (offset -4 lines).
> Hunk #4 succeeded at 4179 (offset -31 lines).
> patching file drivers/scsi/qla2xxx/qla_nx.h
> 
> 
> Build fails
> 
> drivers/scsi/qla2xxx/qla_iocb.c: In function ‘qla_start_scsi_type6’:
> drivers/scsi/qla2xxx/qla_iocb.c:4260:29: error: ‘struct qla_qpair’
> has
> no member named ‘dsd_inuse’
>   if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
>                              ^~
> drivers/scsi/qla2xxx/qla_iocb.c:4263:32: error: ‘struct qla_qpair’
> has
> no member named ‘dsd_inuse’
>           more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN, cmd);
>                                 ^~
> drivers/scsi/qla2xxx/qla_iocb.c:4267:29: error: ‘struct qla_qpair’
> has
> no member named ‘dsd_avail’
>   if (more_dsd_lists <= qpair->dsd_avail)
>                              ^~
> drivers/scsi/qla2xxx/qla_iocb.c:4270:26: error: ‘struct qla_qpair’
> has
> no member named ‘dsd_avail’
>    more_dsd_lists -= qpair->dsd_avail;
>                           ^~
> drivers/scsi/qla2xxx/qla_iocb.c:4289:41: error: ‘struct qla_qpair’
> has
> no member named ‘dsd_list’; did you mean ‘hints_list’?
>    list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
>                                          ^~~~~~~~
>                                          hints_list
> drivers/scsi/qla2xxx/qla_iocb.c:4290:8: error: ‘struct qla_qpair’ has
> no member named ‘dsd_avail’
>    qpair->dsd_avail++;

I see where this was done, there are two more patches
So I will add those and test 
[PATCH 1/2] qla2xxx: Move resource to allow code reuse
>
Laurence Oberman Aug. 4, 2023, 9:35 p.m. UTC | #4
On Fri, 2023-08-04 at 14:35 -0400, Laurence Oberman wrote:
> On Fri, 2023-08-04 at 13:51 -0400, Laurence Oberman wrote:
> > On Fri, 2023-08-04 at 12:58 -0400, Laurence Oberman wrote:
> > > On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> > > > From: Quinn Tran <qutran@marvell.com>
> > > > 
> > > > System crash when 32 bytes CDB was sent to a non T10-PI disk,
> > > > 
> > > > [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0
> > > > [qla2xxx]
> > > > [  177.149165]  ? internal_add_timer+0x42/0x70
> > > > [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> > > > [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> > > > [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> > > > 
> > > > Current code attempt to use CRC IOCB to send the command but
> > > > failed.
> > > > Instead, type 6 IOCB should be used to send the IO.
> > > > 
> > > > Clone existing type 6 IOCB code with addition of MQ support
> > > > to allow 32 bytes CDB to go through.
> > > > 
> > > > Signed-off-by: Quinn Tran <qutran@marvell.com>
> > > > Cc: Laurence Oberman <loberman@redhat.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > > > ---
> > > >  drivers/scsi/qla2xxx/qla_iocb.c | 270
> > > > ++++++++++++++++++++++++++++++++
> > > >  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
> > > >  2 files changed, 273 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > index 0caa64a7df26..e99ebf7e1c7a 100644
> > > > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > @@ -11,6 +11,7 @@
> > > >  
> > > >  #include <scsi/scsi_tcq.h>
> > > >  
> > > > +static int qla_start_scsi_type6(srb_t *sp);
> > > >  /**
> > > >   * qla2x00_get_cmd_direction() - Determine control_flag data
> > > > direction.
> > > >   * @sp: SCSI command
> > > > @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
> > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > >                 if (cmd->cmd_len <= 16)
> > > >                         return qla24xx_start_scsi(sp);
> > > > +               else
> > > > +                       return qla_start_scsi_type6(sp);
> > > >         }
> > > >  
> > > >         /* Setup device pointers. */
> > > > @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
> > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > >                 if (cmd->cmd_len <= 16)
> > > >                         return qla2xxx_start_scsi_mq(sp);
> > > > +               else
> > > > +                       return qla_start_scsi_type6(sp);
> > > >         }
> > > >  
> > > >         spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp, struct
> > > > scsi_qla_host *vha, uint32_t tot_dsds)
> > > >  
> > > >         return rval;
> > > >  }
> > > > +
> > > > +/**
> > > > + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> > > > + * @sp: command to send to the ISP
> > > > + *
> > > > + * Returns non-zero if a failure occurred, else zero.
> > > > + */
> > > > +static int
> > > > +qla_start_scsi_type6(srb_t *sp)
> > > > +{
> > > > +       int             nseg;
> > > > +       unsigned long   flags;
> > > > +       uint32_t        *clr_ptr;
> > > > +       uint32_t        handle;
> > > > +       struct cmd_type_6 *cmd_pkt;
> > > > +       uint16_t        cnt;
> > > > +       uint16_t        req_cnt;
> > > > +       uint16_t        tot_dsds;
> > > > +       struct req_que *req = NULL;
> > > > +       struct rsp_que *rsp;
> > > > +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> > > > +       struct scsi_qla_host *vha = sp->fcport->vha;
> > > > +       struct qla_hw_data *ha = vha->hw;
> > > > +       struct qla_qpair *qpair = sp->qpair;
> > > > +       uint16_t more_dsd_lists = 0;
> > > > +       struct dsd_dma *dsd_ptr;
> > > > +       uint16_t i;
> > > > +       __be32 *fcp_dl;
> > > > +       uint8_t additional_cdb_len;
> > > > +       struct ct6_dsd *ctx;
> > > > +
> > > > +
> > > > +       /* Acquire qpair specific lock */
> > > > +       spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > +
> > > > +       /* Setup qpair pointers */
> > > > +       req = qpair->req;
> > > > +       rsp = qpair->rsp;
> > > > +
> > > > +       /* So we know we haven't pci_map'ed anything yet */
> > > > +       tot_dsds = 0;
> > > > +
> > > > +       /* Send marker if required */
> > > > +       if (vha->marker_needed != 0) {
> > > > +               if (__qla2x00_marker(vha, qpair, 0, 0,
> > > > MK_SYNC_ALL)
> > > > != QLA_SUCCESS) {
> > > > +                       spin_unlock_irqrestore(&qpair->qp_lock,
> > > > flags);
> > > > +                       return QLA_FUNCTION_FAILED;
> > > > +               }
> > > > +               vha->marker_needed = 0;
> > > > +       }
> > > > +
> > > > +       handle = qla2xxx_get_next_handle(req);
> > > > +       if (handle == 0)
> > > > +               goto queuing_error;
> > > > +
> > > > +       /* Map the sg table so we have an accurate count of sg
> > > > entries needed */
> > > > +       if (scsi_sg_count(cmd)) {
> > > > +               nseg = dma_map_sg(&ha->pdev->dev,
> > > > scsi_sglist(cmd),
> > > > +                                 scsi_sg_count(cmd), cmd-
> > > > > sc_data_direction);
> > > > +               if (unlikely(!nseg))
> > > > +                       goto queuing_error;
> > > > +       } else {
> > > > +               nseg = 0;
> > > > +       }
> > > > +
> > > > +       tot_dsds = nseg;
> > > > +
> > > > +       /* eventhough driver only need 1 T6 IOCB, FW still
> > > > convert
> > > > DSD to Continueation IOCB */
> > > > +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> > > > +
> > > > +       sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> > > > +       sp->iores.exch_cnt = 1;
> > > > +       sp->iores.iocb_cnt = req_cnt;
> > > > +
> > > > +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> > > > +               goto queuing_error;
> > > > +
> > > > +       more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> > > > +       if ((more_dsd_lists + qpair->dsd_inuse) >=
> > > > NUM_DSD_CHAIN)
> > > > {
> > > > +               ql_dbg(ql_dbg_io, vha, 0x3028,
> > > > +                      "Num of DSD list %d is than %d for
> > > > cmd=%p.\n",
> > > > +                      more_dsd_lists + qpair->dsd_inuse,
> > > > NUM_DSD_CHAIN, cmd);
> > > > +               goto queuing_error;
> > > > +       }
> > > > +
> > > > +       if (more_dsd_lists <= qpair->dsd_avail)
> > > > +               goto sufficient_dsds;
> > > > +       else
> > > > +               more_dsd_lists -= qpair->dsd_avail;
> > > > +
> > > > +       for (i = 0; i < more_dsd_lists; i++) {
> > > > +               dsd_ptr = kzalloc(sizeof(*dsd_ptr),
> > > > GFP_ATOMIC);
> > > > +               if (!dsd_ptr) {
> > > > +                       ql_log(ql_log_fatal, vha, 0x3029,
> > > > +                           "Failed to allocate memory for
> > > > dsd_dma
> > > > for cmd=%p.\n", cmd);
> > > > +                       goto queuing_error;
> > > > +               }
> > > > +               INIT_LIST_HEAD(&dsd_ptr->list);
> > > > +
> > > > +               dsd_ptr->dsd_addr = dma_pool_alloc(ha-
> > > > > dl_dma_pool,
> > > > +                       GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
> > > > +               if (!dsd_ptr->dsd_addr) {
> > > > +                       kfree(dsd_ptr);
> > > > +                       ql_log(ql_log_fatal, vha, 0x302a,
> > > > +                           "Failed to allocate memory for
> > > > dsd_addr
> > > > for cmd=%p.\n", cmd);
> > > > +                       goto queuing_error;
> > > > +               }
> > > > +               list_add_tail(&dsd_ptr->list, &qpair-
> > > > >dsd_list);
> > > > +               qpair->dsd_avail++;
> > > > +       }
> > > > +
> > > > +sufficient_dsds:
> > > > +       req_cnt = 1;
> > > > +
> > > > +       if (req->cnt < (req_cnt + 2)) {
> > > > +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> > > > +                       cnt = *req->out_ptr;
> > > > +               } else {
> > > > +                       cnt =
> > > > (uint16_t)rd_reg_dword_relaxed(req-
> > > > > req_q_out);
> > > > +                       if
> > > > (qla2x00_check_reg16_for_disconnect(vha,
> > > > cnt))
> > > > +                               goto queuing_error;
> > > > +               }
> > > > +
> > > > +               if (req->ring_index < cnt)
> > > > +                       req->cnt = cnt - req->ring_index;
> > > > +               else
> > > > +                       req->cnt = req->length - (req-
> > > > >ring_index
> > > > -
> > > > cnt);
> > > > +               if (req->cnt < (req_cnt + 2))
> > > > +                       goto queuing_error;
> > > > +       }
> > > > +
> > > > +       ctx = &sp->u.scmd.ct6_ctx;
> > > > +
> > > > +       memset(ctx, 0, sizeof(struct ct6_dsd));
> > > > +       ctx->fcp_cmnd = dma_pool_zalloc(ha->fcp_cmnd_dma_pool,
> > > > +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> > > > +       if (!ctx->fcp_cmnd) {
> > > > +               ql_log(ql_log_fatal, vha, 0x3031,
> > > > +                   "Failed to allocate fcp_cmnd for
> > > > cmd=%p.\n",
> > > > cmd);
> > > > +               goto queuing_error;
> > > > +       }
> > > > +
> > > > +       /* Initialize the DSD list and dma handle */
> > > > +       INIT_LIST_HEAD(&ctx->dsd_list);
> > > > +       ctx->dsd_use_cnt = 0;
> > > > +
> > > > +       if (cmd->cmd_len > 16) {
> > > > +               additional_cdb_len = cmd->cmd_len - 16;
> > > > +               if (cmd->cmd_len % 4 ||
> > > > +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> > > > +                       /* SCSI command bigger than 16 bytes
> > > > must
> > > > be
> > > > +                        * multiple of 4 or too big.
> > > > +                        */
> > > > +                       ql_log(ql_log_warn, vha, 0x3033,
> > > > +                           "scsi cmd len %d not multiple of 4
> > > > for
> > > > cmd=%p.\n",
> > > > +                           cmd->cmd_len, cmd);
> > > > +                       goto queuing_error_fcp_cmnd;
> > > > +               }
> > > > +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> > > > +       } else {
> > > > +               additional_cdb_len = 0;
> > > > +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> > > > +       }
> > > > +
> > > > +       /* Build command packet. */
> > > > +       req->current_outstanding_cmd = handle;
> > > > +       req->outstanding_cmds[handle] = sp;
> > > > +       sp->handle = handle;
> > > > +       cmd->host_scribble = (unsigned char *)(unsigned
> > > > long)handle;
> > > > +       req->cnt -= req_cnt;
> > > > +
> > > > +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> > > > +       cmd_pkt->handle = make_handle(req->id, handle);
> > > > +
> > > > +       /* Zero out remaining portion of packet. */
> > > > +       /*    tagged queuing modifier -- default is TSK_SIMPLE
> > > > (0).
> > > > */
> > > > +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> > > > +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> > > > +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> > > > +
> > > > +       /* Set NPORT-ID and LUN number*/
> > > > +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport-
> > > > >loop_id);
> > > > +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> > > > +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> > > > +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> > > > +       cmd_pkt->vp_index = sp->vha->vp_idx;
> > > > +
> > > > +       /* Build IOCB segments */
> > > > +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt, tot_dsds);
> > > > +
> > > > +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> > > > +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun,
> > > > sizeof(cmd_pkt-
> > > > > lun));
> > > > +
> > > > +       /* build FCP_CMND IU */
> > > > +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd->lun);
> > > > +       ctx->fcp_cmnd->additional_cdb_len = additional_cdb_len;
> > > > +
> > > > +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> > > > +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> > > > +       else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> > > > +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> > > > +
> > > > +       /* Populate the FCP_PRIO. */
> > > > +       if (ha->flags.fcp_prio_enabled)
> > > > +               ctx->fcp_cmnd->task_attribute |=
> > > > +                   sp->fcport->fcp_prio << 3;
> > > > +
> > > > +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
> > > > +
> > > > +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> > > > +           additional_cdb_len);
> > > > +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> > > > +
> > > > +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx-
> > > > > fcp_cmnd_len);
> > > > +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> > > > +                          &cmd_pkt->fcp_cmnd_dseg_address);
> > > > +
> > > > +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> > > > +       cmd_pkt->byte_count =
> > > > cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> > > > +       /* Set total data segment count. */
> > > > +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> > > > +
> > > > +       wmb();
> > > > +       /* Adjust ring index. */
> > > > +       req->ring_index++;
> > > > +       if (req->ring_index == req->length) {
> > > > +               req->ring_index = 0;
> > > > +               req->ring_ptr = req->ring;
> > > > +       } else {
> > > > +               req->ring_ptr++;
> > > > +       }
> > > > +
> > > > +       sp->qpair->cmd_cnt++;
> > > > +       sp->flags |= SRB_DMA_VALID;
> > > > +
> > > > +       /* Set chip new ring index. */
> > > > +       wrt_reg_dword(req->req_q_in, req->ring_index);
> > > > +
> > > > +       /* Manage unprocessed RIO/ZIO commands in response
> > > > queue.
> > > > */
> > > > +       if (vha->flags.process_response_queue &&
> > > > +           rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> > > > +               qla24xx_process_response_queue(vha, rsp);
> > > > +
> > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > +
> > > > +       return QLA_SUCCESS;
> > > > +
> > > > +queuing_error_fcp_cmnd:
> > > > +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd,
> > > > ctx-
> > > > > fcp_cmnd_dma);
> > > > +
> > > > +queuing_error:
> > > > +       if (tot_dsds)
> > > > +               scsi_dma_unmap(cmd);
> > > > +
> > > > +       qla_put_fw_resources(sp->qpair, &sp->iores);
> > > > +
> > > > +       if (sp->u.scmd.crc_ctx) {
> > > > +               mempool_free(sp->u.scmd.crc_ctx, ha-
> > > > > ctx_mempool);
> > > > +               sp->u.scmd.crc_ctx = NULL;
> > > > +       }
> > > > +
> > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > +
> > > > +       return QLA_FUNCTION_FAILED;
> > > > +}
> > > > diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> > > > b/drivers/scsi/qla2xxx/qla_nx.h
> > > > index 6dc80c8ddf79..5d1bdc15b75c 100644
> > > > --- a/drivers/scsi/qla2xxx/qla_nx.h
> > > > +++ b/drivers/scsi/qla2xxx/qla_nx.h
> > > > @@ -857,7 +857,9 @@ struct fcp_cmnd {
> > > >         uint8_t task_attribute;
> > > >         uint8_t task_management;
> > > >         uint8_t additional_cdb_len;
> > > > -       uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL
> > > > */
> > > > +#define QLA_CDB_BUF_SIZE  256
> > > > +#define QLA_FCP_DL_SIZE   4
> > > > +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /* 256
> > > > for
> > > > CDB len and 4 for FCP_DL */
> > > >  };
> > > >  
> > > >  struct dsd_dma {
> > > 
> > > QT and Nilesh, Thank you.
> > > I need more time to look at this fully but I will get it tested
> > > in
> > > the
> > > meantime. 
> > > I will reply with a review or questions later.
> > > 
> > > Thanks for sending this
> > > Laurence
> > 
> > @QT and Nilesh
> > Which kernel was this against.
> > Did you guys test this only agains your driver or also upstream
> > I cannot build it clean because There is no structure member
> > dsd_inuse
> > 
> > Can you send one for upstream please
> > 
> > Patch applied fine to 6.5-rc4
> > 
> > [loberman@segstorage3 linux]$ patch -p1 < qt.patch 
> > patching file drivers/scsi/qla2xxx/qla_iocb.c
> > Hunk #2 succeeded at 1718 (offset -4 lines).
> > Hunk #3 succeeded at 2099 (offset -4 lines).
> > Hunk #4 succeeded at 4179 (offset -31 lines).
> > patching file drivers/scsi/qla2xxx/qla_nx.h
> > 
> > 
> > Build fails
> > 
> > drivers/scsi/qla2xxx/qla_iocb.c: In function
> > ‘qla_start_scsi_type6’:
> > drivers/scsi/qla2xxx/qla_iocb.c:4260:29: error: ‘struct qla_qpair’
> > has
> > no member named ‘dsd_inuse’
> >   if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> >                              ^~
> > drivers/scsi/qla2xxx/qla_iocb.c:4263:32: error: ‘struct qla_qpair’
> > has
> > no member named ‘dsd_inuse’
> >           more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN, cmd);
> >                                 ^~
> > drivers/scsi/qla2xxx/qla_iocb.c:4267:29: error: ‘struct qla_qpair’
> > has
> > no member named ‘dsd_avail’
> >   if (more_dsd_lists <= qpair->dsd_avail)
> >                              ^~
> > drivers/scsi/qla2xxx/qla_iocb.c:4270:26: error: ‘struct qla_qpair’
> > has
> > no member named ‘dsd_avail’
> >    more_dsd_lists -= qpair->dsd_avail;
> >                           ^~
> > drivers/scsi/qla2xxx/qla_iocb.c:4289:41: error: ‘struct qla_qpair’
> > has
> > no member named ‘dsd_list’; did you mean ‘hints_list’?
> >    list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> >                                          ^~~~~~~~
> >                                          hints_list
> > drivers/scsi/qla2xxx/qla_iocb.c:4290:8: error: ‘struct qla_qpair’
> > has
> > no member named ‘dsd_avail’
> >    qpair->dsd_avail++;
> 
> I see where this was done, there are two more patches
> So I will add those and test 
> [PATCH 1/2] qla2xxx: Move resource to allow code reuse
> > 
> 

OK, I was able to now build and and test this.
To be honest there are a lot of changes.
I just tested the original issue for which I had sent my first patch.

Unfortunately the test fails in a different way to the orginal BUG but
this may be firmware behavior on the MSA2050 that takes the device
paths away.
This is because the test passes on an LIO target

Against 6.5-rc4

Test notes
segstorage3 6.5.0-rc4+
Note that on a MSA2050 it will hang and timeout no more panics as
expected. However as noted we lose devices.

Starts out fine

mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
|-+- policy='service-time 0' prio=50 status=active
| |- 1:0:1:4   sdh       8:112   active ready     running
| |- 1:0:4:4   sdaf      65:240  active ready     running
| |- 3:0:2:4   sdfk      130:96  active ready     running
| `- 3:0:3:4   sdft      130:240 active ready     running
`-+- policy='service-time 0' prio=10 status=enabled
  |- 1:0:2:4   sdp       8:240   active ready     running
  |- 1:0:3:4   sdx       65:112  active ready     running
  |- 3:0:1:4   sdfb      129:208 active ready     running
  `- 3:0:4:4   sdge      131:160 active ready     running

Do the test

[root@segstorage3 ~]# sg_write_same -T --lba=1 /dev/mapper/mpathbp
Write same: transport: Host_status=0x03 [DID_TIME_OUT]
Driver_status=0x00 [DRIVER_OK]

It then caused problems with two of my multipath device paths but
perhaps this is array firmware behavior.

[  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
[  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready 00 00
00 00 00 00

[  825.926075] sd 3:0:2:0: rejecting I/O to offline device
[  825.926136] sd 3:0:2:1: rejecting I/O to offline device
[  825.926165] sd 3:0:2:3: rejecting I/O to offline device

And two paths get lost 

mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
|-+- policy='service-time 0' prio=50 status=active
| |- 1:0:4:4   sdaf      65:240  active ready     running
| `- 3:0:3:4   sdft      130:240 active ready     running
`-+- policy='service-time 0' prio=10 status=enabled
  |- 1:0:2:4   sdp       8:240   active ready     running
  |- 1:0:3:4   sdx       65:112  active ready     running
  |- 3:0:1:4   sdfb      129:208 active ready     running
  `- 3:0:4:4   sdge      131:160 active ready     running

I re-scanned but sdh and sdfk are gone from the device tree and do not
come back with a rescan.

[  846.043511] sd 1:0:1:4: [sdh] Synchronizing SCSI cache
[  846.043536] sd 1:0:1:4: [sdh] Synchronize Cache(10) failed: Result:
hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK

[  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
[  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready 00 00
00 00 00 00
[  851.166451] sd 3:0:2:4: [sdfk] Synchronizing SCSI cache
[  851.166477] sd 3:0:2:4: [sdfk] Synchronize Cache(10) failed: Result:
hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK

Now using an LIO target via tcm_qla2xxx

mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
size=1.0G features='0' hwhandler='1 alua' wp=rw
`-+- policy='service-time 0' prio=50 status=active
  |- 1:0:5:105 sdbv      68:144  active ready     running
  |- 1:0:6:105 sdei      128:160 active ready     running
  |- 3:0:5:105 sdhz      134:144 active ready     running
  `- 3:0:6:105 sdkm      66:416  active ready     running

[root@segstorage3 block]# sg_write_same -T --lba=1 /dev/mapper/mpathy
Write same: transport: Host_status=0x07 [DID_ERROR]
Driver_status=0x08 [DRIVER_SENSE]

No issues with the LIO device paths 

[root@segstorage3 block]# multipath -ll mpathy
mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
size=1.0G features='0' hwhandler='1 alua' wp=rw
`-+- policy='service-time 0' prio=50 status=active
  |- 1:0:5:105 sdbv      68:144  active ready     running
  |- 1:0:6:105 sdei      128:160 active ready     running
  |- 3:0:5:105 sdhz      134:144 active ready     running
  `- 3:0:6:105 sdkm      66:416  active ready     running

So I would like to give a positive test result but will need you folks
at Marvell to maybe first work with HPE on the MSA2050 behavior.

Thanks
Laurence
Laurence Oberman Aug. 5, 2023, 4:54 p.m. UTC | #5
On Fri, 2023-08-04 at 17:35 -0400, Laurence Oberman wrote:
> On Fri, 2023-08-04 at 14:35 -0400, Laurence Oberman wrote:
> > On Fri, 2023-08-04 at 13:51 -0400, Laurence Oberman wrote:
> > > On Fri, 2023-08-04 at 12:58 -0400, Laurence Oberman wrote:
> > > > On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> > > > > From: Quinn Tran <qutran@marvell.com>
> > > > > 
> > > > > System crash when 32 bytes CDB was sent to a non T10-PI disk,
> > > > > 
> > > > > [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0
> > > > > [qla2xxx]
> > > > > [  177.149165]  ? internal_add_timer+0x42/0x70
> > > > > [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> > > > > [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> > > > > [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> > > > > 
> > > > > Current code attempt to use CRC IOCB to send the command but
> > > > > failed.
> > > > > Instead, type 6 IOCB should be used to send the IO.
> > > > > 
> > > > > Clone existing type 6 IOCB code with addition of MQ support
> > > > > to allow 32 bytes CDB to go through.
> > > > > 
> > > > > Signed-off-by: Quinn Tran <qutran@marvell.com>
> > > > > Cc: Laurence Oberman <loberman@redhat.com>
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > > > > ---
> > > > >  drivers/scsi/qla2xxx/qla_iocb.c | 270
> > > > > ++++++++++++++++++++++++++++++++
> > > > >  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
> > > > >  2 files changed, 273 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > index 0caa64a7df26..e99ebf7e1c7a 100644
> > > > > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > @@ -11,6 +11,7 @@
> > > > >  
> > > > >  #include <scsi/scsi_tcq.h>
> > > > >  
> > > > > +static int qla_start_scsi_type6(srb_t *sp);
> > > > >  /**
> > > > >   * qla2x00_get_cmd_direction() - Determine control_flag data
> > > > > direction.
> > > > >   * @sp: SCSI command
> > > > > @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
> > > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > > >                 if (cmd->cmd_len <= 16)
> > > > >                         return qla24xx_start_scsi(sp);
> > > > > +               else
> > > > > +                       return qla_start_scsi_type6(sp);
> > > > >         }
> > > > >  
> > > > >         /* Setup device pointers. */
> > > > > @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
> > > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > > >                 if (cmd->cmd_len <= 16)
> > > > >                         return qla2xxx_start_scsi_mq(sp);
> > > > > +               else
> > > > > +                       return qla_start_scsi_type6(sp);
> > > > >         }
> > > > >  
> > > > >         spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > > @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp, struct
> > > > > scsi_qla_host *vha, uint32_t tot_dsds)
> > > > >  
> > > > >         return rval;
> > > > >  }
> > > > > +
> > > > > +/**
> > > > > + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> > > > > + * @sp: command to send to the ISP
> > > > > + *
> > > > > + * Returns non-zero if a failure occurred, else zero.
> > > > > + */
> > > > > +static int
> > > > > +qla_start_scsi_type6(srb_t *sp)
> > > > > +{
> > > > > +       int             nseg;
> > > > > +       unsigned long   flags;
> > > > > +       uint32_t        *clr_ptr;
> > > > > +       uint32_t        handle;
> > > > > +       struct cmd_type_6 *cmd_pkt;
> > > > > +       uint16_t        cnt;
> > > > > +       uint16_t        req_cnt;
> > > > > +       uint16_t        tot_dsds;
> > > > > +       struct req_que *req = NULL;
> > > > > +       struct rsp_que *rsp;
> > > > > +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> > > > > +       struct scsi_qla_host *vha = sp->fcport->vha;
> > > > > +       struct qla_hw_data *ha = vha->hw;
> > > > > +       struct qla_qpair *qpair = sp->qpair;
> > > > > +       uint16_t more_dsd_lists = 0;
> > > > > +       struct dsd_dma *dsd_ptr;
> > > > > +       uint16_t i;
> > > > > +       __be32 *fcp_dl;
> > > > > +       uint8_t additional_cdb_len;
> > > > > +       struct ct6_dsd *ctx;
> > > > > +
> > > > > +
> > > > > +       /* Acquire qpair specific lock */
> > > > > +       spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > > +
> > > > > +       /* Setup qpair pointers */
> > > > > +       req = qpair->req;
> > > > > +       rsp = qpair->rsp;
> > > > > +
> > > > > +       /* So we know we haven't pci_map'ed anything yet */
> > > > > +       tot_dsds = 0;
> > > > > +
> > > > > +       /* Send marker if required */
> > > > > +       if (vha->marker_needed != 0) {
> > > > > +               if (__qla2x00_marker(vha, qpair, 0, 0,
> > > > > MK_SYNC_ALL)
> > > > > != QLA_SUCCESS) {
> > > > > +                       spin_unlock_irqrestore(&qpair-
> > > > > >qp_lock,
> > > > > flags);
> > > > > +                       return QLA_FUNCTION_FAILED;
> > > > > +               }
> > > > > +               vha->marker_needed = 0;
> > > > > +       }
> > > > > +
> > > > > +       handle = qla2xxx_get_next_handle(req);
> > > > > +       if (handle == 0)
> > > > > +               goto queuing_error;
> > > > > +
> > > > > +       /* Map the sg table so we have an accurate count of
> > > > > sg
> > > > > entries needed */
> > > > > +       if (scsi_sg_count(cmd)) {
> > > > > +               nseg = dma_map_sg(&ha->pdev->dev,
> > > > > scsi_sglist(cmd),
> > > > > +                                 scsi_sg_count(cmd), cmd-
> > > > > > sc_data_direction);
> > > > > +               if (unlikely(!nseg))
> > > > > +                       goto queuing_error;
> > > > > +       } else {
> > > > > +               nseg = 0;
> > > > > +       }
> > > > > +
> > > > > +       tot_dsds = nseg;
> > > > > +
> > > > > +       /* eventhough driver only need 1 T6 IOCB, FW still
> > > > > convert
> > > > > DSD to Continueation IOCB */
> > > > > +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> > > > > +
> > > > > +       sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> > > > > +       sp->iores.exch_cnt = 1;
> > > > > +       sp->iores.iocb_cnt = req_cnt;
> > > > > +
> > > > > +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> > > > > +               goto queuing_error;
> > > > > +
> > > > > +       more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> > > > > +       if ((more_dsd_lists + qpair->dsd_inuse) >=
> > > > > NUM_DSD_CHAIN)
> > > > > {
> > > > > +               ql_dbg(ql_dbg_io, vha, 0x3028,
> > > > > +                      "Num of DSD list %d is than %d for
> > > > > cmd=%p.\n",
> > > > > +                      more_dsd_lists + qpair->dsd_inuse,
> > > > > NUM_DSD_CHAIN, cmd);
> > > > > +               goto queuing_error;
> > > > > +       }
> > > > > +
> > > > > +       if (more_dsd_lists <= qpair->dsd_avail)
> > > > > +               goto sufficient_dsds;
> > > > > +       else
> > > > > +               more_dsd_lists -= qpair->dsd_avail;
> > > > > +
> > > > > +       for (i = 0; i < more_dsd_lists; i++) {
> > > > > +               dsd_ptr = kzalloc(sizeof(*dsd_ptr),
> > > > > GFP_ATOMIC);
> > > > > +               if (!dsd_ptr) {
> > > > > +                       ql_log(ql_log_fatal, vha, 0x3029,
> > > > > +                           "Failed to allocate memory for
> > > > > dsd_dma
> > > > > for cmd=%p.\n", cmd);
> > > > > +                       goto queuing_error;
> > > > > +               }
> > > > > +               INIT_LIST_HEAD(&dsd_ptr->list);
> > > > > +
> > > > > +               dsd_ptr->dsd_addr = dma_pool_alloc(ha-
> > > > > > dl_dma_pool,
> > > > > +                       GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
> > > > > +               if (!dsd_ptr->dsd_addr) {
> > > > > +                       kfree(dsd_ptr);
> > > > > +                       ql_log(ql_log_fatal, vha, 0x302a,
> > > > > +                           "Failed to allocate memory for
> > > > > dsd_addr
> > > > > for cmd=%p.\n", cmd);
> > > > > +                       goto queuing_error;
> > > > > +               }
> > > > > +               list_add_tail(&dsd_ptr->list, &qpair-
> > > > > > dsd_list);
> > > > > +               qpair->dsd_avail++;
> > > > > +       }
> > > > > +
> > > > > +sufficient_dsds:
> > > > > +       req_cnt = 1;
> > > > > +
> > > > > +       if (req->cnt < (req_cnt + 2)) {
> > > > > +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> > > > > +                       cnt = *req->out_ptr;
> > > > > +               } else {
> > > > > +                       cnt =
> > > > > (uint16_t)rd_reg_dword_relaxed(req-
> > > > > > req_q_out);
> > > > > +                       if
> > > > > (qla2x00_check_reg16_for_disconnect(vha,
> > > > > cnt))
> > > > > +                               goto queuing_error;
> > > > > +               }
> > > > > +
> > > > > +               if (req->ring_index < cnt)
> > > > > +                       req->cnt = cnt - req->ring_index;
> > > > > +               else
> > > > > +                       req->cnt = req->length - (req-
> > > > > > ring_index
> > > > > -
> > > > > cnt);
> > > > > +               if (req->cnt < (req_cnt + 2))
> > > > > +                       goto queuing_error;
> > > > > +       }
> > > > > +
> > > > > +       ctx = &sp->u.scmd.ct6_ctx;
> > > > > +
> > > > > +       memset(ctx, 0, sizeof(struct ct6_dsd));
> > > > > +       ctx->fcp_cmnd = dma_pool_zalloc(ha-
> > > > > >fcp_cmnd_dma_pool,
> > > > > +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> > > > > +       if (!ctx->fcp_cmnd) {
> > > > > +               ql_log(ql_log_fatal, vha, 0x3031,
> > > > > +                   "Failed to allocate fcp_cmnd for
> > > > > cmd=%p.\n",
> > > > > cmd);
> > > > > +               goto queuing_error;
> > > > > +       }
> > > > > +
> > > > > +       /* Initialize the DSD list and dma handle */
> > > > > +       INIT_LIST_HEAD(&ctx->dsd_list);
> > > > > +       ctx->dsd_use_cnt = 0;
> > > > > +
> > > > > +       if (cmd->cmd_len > 16) {
> > > > > +               additional_cdb_len = cmd->cmd_len - 16;
> > > > > +               if (cmd->cmd_len % 4 ||
> > > > > +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> > > > > +                       /* SCSI command bigger than 16 bytes
> > > > > must
> > > > > be
> > > > > +                        * multiple of 4 or too big.
> > > > > +                        */
> > > > > +                       ql_log(ql_log_warn, vha, 0x3033,
> > > > > +                           "scsi cmd len %d not multiple of
> > > > > 4
> > > > > for
> > > > > cmd=%p.\n",
> > > > > +                           cmd->cmd_len, cmd);
> > > > > +                       goto queuing_error_fcp_cmnd;
> > > > > +               }
> > > > > +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> > > > > +       } else {
> > > > > +               additional_cdb_len = 0;
> > > > > +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> > > > > +       }
> > > > > +
> > > > > +       /* Build command packet. */
> > > > > +       req->current_outstanding_cmd = handle;
> > > > > +       req->outstanding_cmds[handle] = sp;
> > > > > +       sp->handle = handle;
> > > > > +       cmd->host_scribble = (unsigned char *)(unsigned
> > > > > long)handle;
> > > > > +       req->cnt -= req_cnt;
> > > > > +
> > > > > +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> > > > > +       cmd_pkt->handle = make_handle(req->id, handle);
> > > > > +
> > > > > +       /* Zero out remaining portion of packet. */
> > > > > +       /*    tagged queuing modifier -- default is
> > > > > TSK_SIMPLE
> > > > > (0).
> > > > > */
> > > > > +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> > > > > +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> > > > > +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> > > > > +
> > > > > +       /* Set NPORT-ID and LUN number*/
> > > > > +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport-
> > > > > > loop_id);
> > > > > +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> > > > > +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> > > > > +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> > > > > +       cmd_pkt->vp_index = sp->vha->vp_idx;
> > > > > +
> > > > > +       /* Build IOCB segments */
> > > > > +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt,
> > > > > tot_dsds);
> > > > > +
> > > > > +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> > > > > +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun,
> > > > > sizeof(cmd_pkt-
> > > > > > lun));
> > > > > +
> > > > > +       /* build FCP_CMND IU */
> > > > > +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd-
> > > > > >lun);
> > > > > +       ctx->fcp_cmnd->additional_cdb_len =
> > > > > additional_cdb_len;
> > > > > +
> > > > > +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> > > > > +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> > > > > +       else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> > > > > +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> > > > > +
> > > > > +       /* Populate the FCP_PRIO. */
> > > > > +       if (ha->flags.fcp_prio_enabled)
> > > > > +               ctx->fcp_cmnd->task_attribute |=
> > > > > +                   sp->fcport->fcp_prio << 3;
> > > > > +
> > > > > +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
> > > > > +
> > > > > +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> > > > > +           additional_cdb_len);
> > > > > +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> > > > > +
> > > > > +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx-
> > > > > > fcp_cmnd_len);
> > > > > +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> > > > > +                          &cmd_pkt->fcp_cmnd_dseg_address);
> > > > > +
> > > > > +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> > > > > +       cmd_pkt->byte_count =
> > > > > cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> > > > > +       /* Set total data segment count. */
> > > > > +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> > > > > +
> > > > > +       wmb();
> > > > > +       /* Adjust ring index. */
> > > > > +       req->ring_index++;
> > > > > +       if (req->ring_index == req->length) {
> > > > > +               req->ring_index = 0;
> > > > > +               req->ring_ptr = req->ring;
> > > > > +       } else {
> > > > > +               req->ring_ptr++;
> > > > > +       }
> > > > > +
> > > > > +       sp->qpair->cmd_cnt++;
> > > > > +       sp->flags |= SRB_DMA_VALID;
> > > > > +
> > > > > +       /* Set chip new ring index. */
> > > > > +       wrt_reg_dword(req->req_q_in, req->ring_index);
> > > > > +
> > > > > +       /* Manage unprocessed RIO/ZIO commands in response
> > > > > queue.
> > > > > */
> > > > > +       if (vha->flags.process_response_queue &&
> > > > > +           rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> > > > > +               qla24xx_process_response_queue(vha, rsp);
> > > > > +
> > > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > > +
> > > > > +       return QLA_SUCCESS;
> > > > > +
> > > > > +queuing_error_fcp_cmnd:
> > > > > +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd,
> > > > > ctx-
> > > > > > fcp_cmnd_dma);
> > > > > +
> > > > > +queuing_error:
> > > > > +       if (tot_dsds)
> > > > > +               scsi_dma_unmap(cmd);
> > > > > +
> > > > > +       qla_put_fw_resources(sp->qpair, &sp->iores);
> > > > > +
> > > > > +       if (sp->u.scmd.crc_ctx) {
> > > > > +               mempool_free(sp->u.scmd.crc_ctx, ha-
> > > > > > ctx_mempool);
> > > > > +               sp->u.scmd.crc_ctx = NULL;
> > > > > +       }
> > > > > +
> > > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > > +
> > > > > +       return QLA_FUNCTION_FAILED;
> > > > > +}
> > > > > diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> > > > > b/drivers/scsi/qla2xxx/qla_nx.h
> > > > > index 6dc80c8ddf79..5d1bdc15b75c 100644
> > > > > --- a/drivers/scsi/qla2xxx/qla_nx.h
> > > > > +++ b/drivers/scsi/qla2xxx/qla_nx.h
> > > > > @@ -857,7 +857,9 @@ struct fcp_cmnd {
> > > > >         uint8_t task_attribute;
> > > > >         uint8_t task_management;
> > > > >         uint8_t additional_cdb_len;
> > > > > -       uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL
> > > > > */
> > > > > +#define QLA_CDB_BUF_SIZE  256
> > > > > +#define QLA_FCP_DL_SIZE   4
> > > > > +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /*
> > > > > 256
> > > > > for
> > > > > CDB len and 4 for FCP_DL */
> > > > >  };
> > > > >  
> > > > >  struct dsd_dma {
> > > > 
> > > > QT and Nilesh, Thank you.
> > > > I need more time to look at this fully but I will get it tested
> > > > in
> > > > the
> > > > meantime. 
> > > > I will reply with a review or questions later.
> > > > 
> > > > Thanks for sending this
> > > > Laurence
> > > 
> > > @QT and Nilesh
> > > Which kernel was this against.
> > > Did you guys test this only agains your driver or also upstream
> > > I cannot build it clean because There is no structure member
> > > dsd_inuse
> > > 
> > > Can you send one for upstream please
> > > 
> > > Patch applied fine to 6.5-rc4
> > > 
> > > [loberman@segstorage3 linux]$ patch -p1 < qt.patch 
> > > patching file drivers/scsi/qla2xxx/qla_iocb.c
> > > Hunk #2 succeeded at 1718 (offset -4 lines).
> > > Hunk #3 succeeded at 2099 (offset -4 lines).
> > > Hunk #4 succeeded at 4179 (offset -31 lines).
> > > patching file drivers/scsi/qla2xxx/qla_nx.h
> > > 
> > > 
> > > Build fails
> > > 
> > > drivers/scsi/qla2xxx/qla_iocb.c: In function
> > > ‘qla_start_scsi_type6’:
> > > drivers/scsi/qla2xxx/qla_iocb.c:4260:29: error: ‘struct
> > > qla_qpair’
> > > has
> > > no member named ‘dsd_inuse’
> > >   if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> > >                              ^~
> > > drivers/scsi/qla2xxx/qla_iocb.c:4263:32: error: ‘struct
> > > qla_qpair’
> > > has
> > > no member named ‘dsd_inuse’
> > >           more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN, cmd);
> > >                                 ^~
> > > drivers/scsi/qla2xxx/qla_iocb.c:4267:29: error: ‘struct
> > > qla_qpair’
> > > has
> > > no member named ‘dsd_avail’
> > >   if (more_dsd_lists <= qpair->dsd_avail)
> > >                              ^~
> > > drivers/scsi/qla2xxx/qla_iocb.c:4270:26: error: ‘struct
> > > qla_qpair’
> > > has
> > > no member named ‘dsd_avail’
> > >    more_dsd_lists -= qpair->dsd_avail;
> > >                           ^~
> > > drivers/scsi/qla2xxx/qla_iocb.c:4289:41: error: ‘struct
> > > qla_qpair’
> > > has
> > > no member named ‘dsd_list’; did you mean ‘hints_list’?
> > >    list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> > >                                          ^~~~~~~~
> > >                                          hints_list
> > > drivers/scsi/qla2xxx/qla_iocb.c:4290:8: error: ‘struct qla_qpair’
> > > has
> > > no member named ‘dsd_avail’
> > >    qpair->dsd_avail++;
> > 
> > I see where this was done, there are two more patches
> > So I will add those and test 
> > [PATCH 1/2] qla2xxx: Move resource to allow code reuse
> > > 
> > 
> 
> OK, I was able to now build and and test this.
> To be honest there are a lot of changes.
> I just tested the original issue for which I had sent my first patch.
> 
> Unfortunately the test fails in a different way to the orginal BUG
> but
> this may be firmware behavior on the MSA2050 that takes the device
> paths away.
> This is because the test passes on an LIO target
> 
> Against 6.5-rc4
> 
> Test notes
> segstorage3 6.5.0-rc4+
> Note that on a MSA2050 it will hang and timeout no more panics as
> expected. However as noted we lose devices.
> 
> Starts out fine
> 
> mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
> size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
> > -+- policy='service-time 0' prio=50 status=active
> > > - 1:0:1:4   sdh       8:112   active ready     running
> > > - 1:0:4:4   sdaf      65:240  active ready     running
> > > - 3:0:2:4   sdfk      130:96  active ready     running
> > `- 3:0:3:4   sdft      130:240 active ready     running
> `-+- policy='service-time 0' prio=10 status=enabled
>   |- 1:0:2:4   sdp       8:240   active ready     running
>   |- 1:0:3:4   sdx       65:112  active ready     running
>   |- 3:0:1:4   sdfb      129:208 active ready     running
>   `- 3:0:4:4   sdge      131:160 active ready     running
> 
> Do the test
> 
> [root@segstorage3 ~]# sg_write_same -T --lba=1 /dev/mapper/mpathbp
> Write same: transport: Host_status=0x03 [DID_TIME_OUT]
> Driver_status=0x00 [DRIVER_OK]
> 
> It then caused problems with two of my multipath device paths but
> perhaps this is array firmware behavior.
> 
> [  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
> hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
> [  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready 00 00
> 00 00 00 00
> 
> [  825.926075] sd 3:0:2:0: rejecting I/O to offline device
> [  825.926136] sd 3:0:2:1: rejecting I/O to offline device
> [  825.926165] sd 3:0:2:3: rejecting I/O to offline device
> 
> And two paths get lost 
> 
> mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
> size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
> > -+- policy='service-time 0' prio=50 status=active
> > > - 1:0:4:4   sdaf      65:240  active ready     running
> > `- 3:0:3:4   sdft      130:240 active ready     running
> `-+- policy='service-time 0' prio=10 status=enabled
>   |- 1:0:2:4   sdp       8:240   active ready     running
>   |- 1:0:3:4   sdx       65:112  active ready     running
>   |- 3:0:1:4   sdfb      129:208 active ready     running
>   `- 3:0:4:4   sdge      131:160 active ready     running
> 
> I re-scanned but sdh and sdfk are gone from the device tree and do
> not
> come back with a rescan.
> 
> [  846.043511] sd 1:0:1:4: [sdh] Synchronizing SCSI cache
> [  846.043536] sd 1:0:1:4: [sdh] Synchronize Cache(10) failed:
> Result:
> hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> 
> [  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
> hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
> [  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready 00 00
> 00 00 00 00
> [  851.166451] sd 3:0:2:4: [sdfk] Synchronizing SCSI cache
> [  851.166477] sd 3:0:2:4: [sdfk] Synchronize Cache(10) failed:
> Result:
> hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> 
> Now using an LIO target via tcm_qla2xxx
> 
> mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
> size=1.0G features='0' hwhandler='1 alua' wp=rw
> `-+- policy='service-time 0' prio=50 status=active
>   |- 1:0:5:105 sdbv      68:144  active ready     running
>   |- 1:0:6:105 sdei      128:160 active ready     running
>   |- 3:0:5:105 sdhz      134:144 active ready     running
>   `- 3:0:6:105 sdkm      66:416  active ready     running
> 
> [root@segstorage3 block]# sg_write_same -T --lba=1 /dev/mapper/mpathy
> Write same: transport: Host_status=0x07 [DID_ERROR]
> Driver_status=0x08 [DRIVER_SENSE]
> 
> No issues with the LIO device paths 
> 
> [root@segstorage3 block]# multipath -ll mpathy
> mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
> size=1.0G features='0' hwhandler='1 alua' wp=rw
> `-+- policy='service-time 0' prio=50 status=active
>   |- 1:0:5:105 sdbv      68:144  active ready     running
>   |- 1:0:6:105 sdei      128:160 active ready     running
>   |- 3:0:5:105 sdhz      134:144 active ready     running
>   `- 3:0:6:105 sdkm      66:416  active ready     running
> 
> So I would like to give a positive test result but will need you
> folks
> at Marvell to maybe first work with HPE on the MSA2050 behavior.
> 
> Thanks
> Laurence
Hello Nilesh and Marvell

I ran a few more tests, this 32 bytes CDB patch set totally breaks the
MSA2050 such that a full storage side reboot of the controller pair is
needed to get the paths back. 

For that reason I am going to NACK this until you chat with HPE.
Those MSA2050's are installed everywhere.

Not pushing my original fix, but my patch would land up with the
MSA2050 timing out but then all would function as it should after the
timeout. Something about your change is not making the MSA2050 happy at
all.

Thanks
Laurence
Laurence Oberman Aug. 5, 2023, 6:16 p.m. UTC | #6
On Sat, 2023-08-05 at 12:54 -0400, Laurence Oberman wrote:
> On Fri, 2023-08-04 at 17:35 -0400, Laurence Oberman wrote:
> > On Fri, 2023-08-04 at 14:35 -0400, Laurence Oberman wrote:
> > > On Fri, 2023-08-04 at 13:51 -0400, Laurence Oberman wrote:
> > > > On Fri, 2023-08-04 at 12:58 -0400, Laurence Oberman wrote:
> > > > > On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> > > > > > From: Quinn Tran <qutran@marvell.com>
> > > > > > 
> > > > > > System crash when 32 bytes CDB was sent to a non T10-PI
> > > > > > disk,
> > > > > > 
> > > > > > [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0
> > > > > > [qla2xxx]
> > > > > > [  177.149165]  ? internal_add_timer+0x42/0x70
> > > > > > [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> > > > > > [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> > > > > > [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> > > > > > 
> > > > > > Current code attempt to use CRC IOCB to send the command
> > > > > > but
> > > > > > failed.
> > > > > > Instead, type 6 IOCB should be used to send the IO.
> > > > > > 
> > > > > > Clone existing type 6 IOCB code with addition of MQ support
> > > > > > to allow 32 bytes CDB to go through.
> > > > > > 
> > > > > > Signed-off-by: Quinn Tran <qutran@marvell.com>
> > > > > > Cc: Laurence Oberman <loberman@redhat.com>
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > > > > > ---
> > > > > >  drivers/scsi/qla2xxx/qla_iocb.c | 270
> > > > > > ++++++++++++++++++++++++++++++++
> > > > > >  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
> > > > > >  2 files changed, 273 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > index 0caa64a7df26..e99ebf7e1c7a 100644
> > > > > > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > @@ -11,6 +11,7 @@
> > > > > >  
> > > > > >  #include <scsi/scsi_tcq.h>
> > > > > >  
> > > > > > +static int qla_start_scsi_type6(srb_t *sp);
> > > > > >  /**
> > > > > >   * qla2x00_get_cmd_direction() - Determine control_flag
> > > > > > data
> > > > > > direction.
> > > > > >   * @sp: SCSI command
> > > > > > @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
> > > > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > > > >                 if (cmd->cmd_len <= 16)
> > > > > >                         return qla24xx_start_scsi(sp);
> > > > > > +               else
> > > > > > +                       return qla_start_scsi_type6(sp);
> > > > > >         }
> > > > > >  
> > > > > >         /* Setup device pointers. */
> > > > > > @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
> > > > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > > > >                 if (cmd->cmd_len <= 16)
> > > > > >                         return qla2xxx_start_scsi_mq(sp);
> > > > > > +               else
> > > > > > +                       return qla_start_scsi_type6(sp);
> > > > > >         }
> > > > > >  
> > > > > >         spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > > > @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp,
> > > > > > struct
> > > > > > scsi_qla_host *vha, uint32_t tot_dsds)
> > > > > >  
> > > > > >         return rval;
> > > > > >  }
> > > > > > +
> > > > > > +/**
> > > > > > + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> > > > > > + * @sp: command to send to the ISP
> > > > > > + *
> > > > > > + * Returns non-zero if a failure occurred, else zero.
> > > > > > + */
> > > > > > +static int
> > > > > > +qla_start_scsi_type6(srb_t *sp)
> > > > > > +{
> > > > > > +       int             nseg;
> > > > > > +       unsigned long   flags;
> > > > > > +       uint32_t        *clr_ptr;
> > > > > > +       uint32_t        handle;
> > > > > > +       struct cmd_type_6 *cmd_pkt;
> > > > > > +       uint16_t        cnt;
> > > > > > +       uint16_t        req_cnt;
> > > > > > +       uint16_t        tot_dsds;
> > > > > > +       struct req_que *req = NULL;
> > > > > > +       struct rsp_que *rsp;
> > > > > > +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> > > > > > +       struct scsi_qla_host *vha = sp->fcport->vha;
> > > > > > +       struct qla_hw_data *ha = vha->hw;
> > > > > > +       struct qla_qpair *qpair = sp->qpair;
> > > > > > +       uint16_t more_dsd_lists = 0;
> > > > > > +       struct dsd_dma *dsd_ptr;
> > > > > > +       uint16_t i;
> > > > > > +       __be32 *fcp_dl;
> > > > > > +       uint8_t additional_cdb_len;
> > > > > > +       struct ct6_dsd *ctx;
> > > > > > +
> > > > > > +
> > > > > > +       /* Acquire qpair specific lock */
> > > > > > +       spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > > > +
> > > > > > +       /* Setup qpair pointers */
> > > > > > +       req = qpair->req;
> > > > > > +       rsp = qpair->rsp;
> > > > > > +
> > > > > > +       /* So we know we haven't pci_map'ed anything yet */
> > > > > > +       tot_dsds = 0;
> > > > > > +
> > > > > > +       /* Send marker if required */
> > > > > > +       if (vha->marker_needed != 0) {
> > > > > > +               if (__qla2x00_marker(vha, qpair, 0, 0,
> > > > > > MK_SYNC_ALL)
> > > > > > != QLA_SUCCESS) {
> > > > > > +                       spin_unlock_irqrestore(&qpair-
> > > > > > > qp_lock,
> > > > > > flags);
> > > > > > +                       return QLA_FUNCTION_FAILED;
> > > > > > +               }
> > > > > > +               vha->marker_needed = 0;
> > > > > > +       }
> > > > > > +
> > > > > > +       handle = qla2xxx_get_next_handle(req);
> > > > > > +       if (handle == 0)
> > > > > > +               goto queuing_error;
> > > > > > +
> > > > > > +       /* Map the sg table so we have an accurate count of
> > > > > > sg
> > > > > > entries needed */
> > > > > > +       if (scsi_sg_count(cmd)) {
> > > > > > +               nseg = dma_map_sg(&ha->pdev->dev,
> > > > > > scsi_sglist(cmd),
> > > > > > +                                 scsi_sg_count(cmd), cmd-
> > > > > > > sc_data_direction);
> > > > > > +               if (unlikely(!nseg))
> > > > > > +                       goto queuing_error;
> > > > > > +       } else {
> > > > > > +               nseg = 0;
> > > > > > +       }
> > > > > > +
> > > > > > +       tot_dsds = nseg;
> > > > > > +
> > > > > > +       /* eventhough driver only need 1 T6 IOCB, FW still
> > > > > > convert
> > > > > > DSD to Continueation IOCB */
> > > > > > +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> > > > > > +
> > > > > > +       sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> > > > > > +       sp->iores.exch_cnt = 1;
> > > > > > +       sp->iores.iocb_cnt = req_cnt;
> > > > > > +
> > > > > > +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> > > > > > +               goto queuing_error;
> > > > > > +
> > > > > > +       more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> > > > > > +       if ((more_dsd_lists + qpair->dsd_inuse) >=
> > > > > > NUM_DSD_CHAIN)
> > > > > > {
> > > > > > +               ql_dbg(ql_dbg_io, vha, 0x3028,
> > > > > > +                      "Num of DSD list %d is than %d for
> > > > > > cmd=%p.\n",
> > > > > > +                      more_dsd_lists + qpair->dsd_inuse,
> > > > > > NUM_DSD_CHAIN, cmd);
> > > > > > +               goto queuing_error;
> > > > > > +       }
> > > > > > +
> > > > > > +       if (more_dsd_lists <= qpair->dsd_avail)
> > > > > > +               goto sufficient_dsds;
> > > > > > +       else
> > > > > > +               more_dsd_lists -= qpair->dsd_avail;
> > > > > > +
> > > > > > +       for (i = 0; i < more_dsd_lists; i++) {
> > > > > > +               dsd_ptr = kzalloc(sizeof(*dsd_ptr),
> > > > > > GFP_ATOMIC);
> > > > > > +               if (!dsd_ptr) {
> > > > > > +                       ql_log(ql_log_fatal, vha, 0x3029,
> > > > > > +                           "Failed to allocate memory for
> > > > > > dsd_dma
> > > > > > for cmd=%p.\n", cmd);
> > > > > > +                       goto queuing_error;
> > > > > > +               }
> > > > > > +               INIT_LIST_HEAD(&dsd_ptr->list);
> > > > > > +
> > > > > > +               dsd_ptr->dsd_addr = dma_pool_alloc(ha-
> > > > > > > dl_dma_pool,
> > > > > > +                       GFP_ATOMIC, &dsd_ptr-
> > > > > > >dsd_list_dma);
> > > > > > +               if (!dsd_ptr->dsd_addr) {
> > > > > > +                       kfree(dsd_ptr);
> > > > > > +                       ql_log(ql_log_fatal, vha, 0x302a,
> > > > > > +                           "Failed to allocate memory for
> > > > > > dsd_addr
> > > > > > for cmd=%p.\n", cmd);
> > > > > > +                       goto queuing_error;
> > > > > > +               }
> > > > > > +               list_add_tail(&dsd_ptr->list, &qpair-
> > > > > > > dsd_list);
> > > > > > +               qpair->dsd_avail++;
> > > > > > +       }
> > > > > > +
> > > > > > +sufficient_dsds:
> > > > > > +       req_cnt = 1;
> > > > > > +
> > > > > > +       if (req->cnt < (req_cnt + 2)) {
> > > > > > +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> > > > > > +                       cnt = *req->out_ptr;
> > > > > > +               } else {
> > > > > > +                       cnt =
> > > > > > (uint16_t)rd_reg_dword_relaxed(req-
> > > > > > > req_q_out);
> > > > > > +                       if
> > > > > > (qla2x00_check_reg16_for_disconnect(vha,
> > > > > > cnt))
> > > > > > +                               goto queuing_error;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (req->ring_index < cnt)
> > > > > > +                       req->cnt = cnt - req->ring_index;
> > > > > > +               else
> > > > > > +                       req->cnt = req->length - (req-
> > > > > > > ring_index
> > > > > > -
> > > > > > cnt);
> > > > > > +               if (req->cnt < (req_cnt + 2))
> > > > > > +                       goto queuing_error;
> > > > > > +       }
> > > > > > +
> > > > > > +       ctx = &sp->u.scmd.ct6_ctx;
> > > > > > +
> > > > > > +       memset(ctx, 0, sizeof(struct ct6_dsd));
> > > > > > +       ctx->fcp_cmnd = dma_pool_zalloc(ha-
> > > > > > > fcp_cmnd_dma_pool,
> > > > > > +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> > > > > > +       if (!ctx->fcp_cmnd) {
> > > > > > +               ql_log(ql_log_fatal, vha, 0x3031,
> > > > > > +                   "Failed to allocate fcp_cmnd for
> > > > > > cmd=%p.\n",
> > > > > > cmd);
> > > > > > +               goto queuing_error;
> > > > > > +       }
> > > > > > +
> > > > > > +       /* Initialize the DSD list and dma handle */
> > > > > > +       INIT_LIST_HEAD(&ctx->dsd_list);
> > > > > > +       ctx->dsd_use_cnt = 0;
> > > > > > +
> > > > > > +       if (cmd->cmd_len > 16) {
> > > > > > +               additional_cdb_len = cmd->cmd_len - 16;
> > > > > > +               if (cmd->cmd_len % 4 ||
> > > > > > +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> > > > > > +                       /* SCSI command bigger than 16
> > > > > > bytes
> > > > > > must
> > > > > > be
> > > > > > +                        * multiple of 4 or too big.
> > > > > > +                        */
> > > > > > +                       ql_log(ql_log_warn, vha, 0x3033,
> > > > > > +                           "scsi cmd len %d not multiple
> > > > > > of
> > > > > > 4
> > > > > > for
> > > > > > cmd=%p.\n",
> > > > > > +                           cmd->cmd_len, cmd);
> > > > > > +                       goto queuing_error_fcp_cmnd;
> > > > > > +               }
> > > > > > +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> > > > > > +       } else {
> > > > > > +               additional_cdb_len = 0;
> > > > > > +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> > > > > > +       }
> > > > > > +
> > > > > > +       /* Build command packet. */
> > > > > > +       req->current_outstanding_cmd = handle;
> > > > > > +       req->outstanding_cmds[handle] = sp;
> > > > > > +       sp->handle = handle;
> > > > > > +       cmd->host_scribble = (unsigned char *)(unsigned
> > > > > > long)handle;
> > > > > > +       req->cnt -= req_cnt;
> > > > > > +
> > > > > > +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> > > > > > +       cmd_pkt->handle = make_handle(req->id, handle);
> > > > > > +
> > > > > > +       /* Zero out remaining portion of packet. */
> > > > > > +       /*    tagged queuing modifier -- default is
> > > > > > TSK_SIMPLE
> > > > > > (0).
> > > > > > */
> > > > > > +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> > > > > > +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> > > > > > +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> > > > > > +
> > > > > > +       /* Set NPORT-ID and LUN number*/
> > > > > > +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport-
> > > > > > > loop_id);
> > > > > > +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> > > > > > +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> > > > > > +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> > > > > > +       cmd_pkt->vp_index = sp->vha->vp_idx;
> > > > > > +
> > > > > > +       /* Build IOCB segments */
> > > > > > +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt,
> > > > > > tot_dsds);
> > > > > > +
> > > > > > +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> > > > > > +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun,
> > > > > > sizeof(cmd_pkt-
> > > > > > > lun));
> > > > > > +
> > > > > > +       /* build FCP_CMND IU */
> > > > > > +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd-
> > > > > > > lun);
> > > > > > +       ctx->fcp_cmnd->additional_cdb_len =
> > > > > > additional_cdb_len;
> > > > > > +
> > > > > > +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> > > > > > +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> > > > > > +       else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> > > > > > +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> > > > > > +
> > > > > > +       /* Populate the FCP_PRIO. */
> > > > > > +       if (ha->flags.fcp_prio_enabled)
> > > > > > +               ctx->fcp_cmnd->task_attribute |=
> > > > > > +                   sp->fcport->fcp_prio << 3;
> > > > > > +
> > > > > > +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd-
> > > > > > >cmd_len);
> > > > > > +
> > > > > > +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> > > > > > +           additional_cdb_len);
> > > > > > +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> > > > > > +
> > > > > > +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx-
> > > > > > > fcp_cmnd_len);
> > > > > > +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> > > > > > +                          &cmd_pkt-
> > > > > > >fcp_cmnd_dseg_address);
> > > > > > +
> > > > > > +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> > > > > > +       cmd_pkt->byte_count =
> > > > > > cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> > > > > > +       /* Set total data segment count. */
> > > > > > +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> > > > > > +
> > > > > > +       wmb();
> > > > > > +       /* Adjust ring index. */
> > > > > > +       req->ring_index++;
> > > > > > +       if (req->ring_index == req->length) {
> > > > > > +               req->ring_index = 0;
> > > > > > +               req->ring_ptr = req->ring;
> > > > > > +       } else {
> > > > > > +               req->ring_ptr++;
> > > > > > +       }
> > > > > > +
> > > > > > +       sp->qpair->cmd_cnt++;
> > > > > > +       sp->flags |= SRB_DMA_VALID;
> > > > > > +
> > > > > > +       /* Set chip new ring index. */
> > > > > > +       wrt_reg_dword(req->req_q_in, req->ring_index);
> > > > > > +
> > > > > > +       /* Manage unprocessed RIO/ZIO commands in response
> > > > > > queue.
> > > > > > */
> > > > > > +       if (vha->flags.process_response_queue &&
> > > > > > +           rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> > > > > > +               qla24xx_process_response_queue(vha, rsp);
> > > > > > +
> > > > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > > > +
> > > > > > +       return QLA_SUCCESS;
> > > > > > +
> > > > > > +queuing_error_fcp_cmnd:
> > > > > > +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd,
> > > > > > ctx-
> > > > > > > fcp_cmnd_dma);
> > > > > > +
> > > > > > +queuing_error:
> > > > > > +       if (tot_dsds)
> > > > > > +               scsi_dma_unmap(cmd);
> > > > > > +
> > > > > > +       qla_put_fw_resources(sp->qpair, &sp->iores);
> > > > > > +
> > > > > > +       if (sp->u.scmd.crc_ctx) {
> > > > > > +               mempool_free(sp->u.scmd.crc_ctx, ha-
> > > > > > > ctx_mempool);
> > > > > > +               sp->u.scmd.crc_ctx = NULL;
> > > > > > +       }
> > > > > > +
> > > > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > > > +
> > > > > > +       return QLA_FUNCTION_FAILED;
> > > > > > +}
> > > > > > diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > b/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > index 6dc80c8ddf79..5d1bdc15b75c 100644
> > > > > > --- a/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > +++ b/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > @@ -857,7 +857,9 @@ struct fcp_cmnd {
> > > > > >         uint8_t task_attribute;
> > > > > >         uint8_t task_management;
> > > > > >         uint8_t additional_cdb_len;
> > > > > > -       uint8_t cdb[260]; /* 256 for CDB len and 4 for
> > > > > > FCP_DL
> > > > > > */
> > > > > > +#define QLA_CDB_BUF_SIZE  256
> > > > > > +#define QLA_FCP_DL_SIZE   4
> > > > > > +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /*
> > > > > > 256
> > > > > > for
> > > > > > CDB len and 4 for FCP_DL */
> > > > > >  };
> > > > > >  
> > > > > >  struct dsd_dma {
> > > > > 
> > > > > QT and Nilesh, Thank you.
> > > > > I need more time to look at this fully but I will get it
> > > > > tested
> > > > > in
> > > > > the
> > > > > meantime. 
> > > > > I will reply with a review or questions later.
> > > > > 
> > > > > Thanks for sending this
> > > > > Laurence
> > > > 
> > > > @QT and Nilesh
> > > > Which kernel was this against.
> > > > Did you guys test this only agains your driver or also upstream
> > > > I cannot build it clean because There is no structure member
> > > > dsd_inuse
> > > > 
> > > > Can you send one for upstream please
> > > > 
> > > > Patch applied fine to 6.5-rc4
> > > > 
> > > > [loberman@segstorage3 linux]$ patch -p1 < qt.patch 
> > > > patching file drivers/scsi/qla2xxx/qla_iocb.c
> > > > Hunk #2 succeeded at 1718 (offset -4 lines).
> > > > Hunk #3 succeeded at 2099 (offset -4 lines).
> > > > Hunk #4 succeeded at 4179 (offset -31 lines).
> > > > patching file drivers/scsi/qla2xxx/qla_nx.h
> > > > 
> > > > 
> > > > Build fails
> > > > 
> > > > drivers/scsi/qla2xxx/qla_iocb.c: In function
> > > > ‘qla_start_scsi_type6’:
> > > > drivers/scsi/qla2xxx/qla_iocb.c:4260:29: error: ‘struct
> > > > qla_qpair’
> > > > has
> > > > no member named ‘dsd_inuse’
> > > >   if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> > > >                              ^~
> > > > drivers/scsi/qla2xxx/qla_iocb.c:4263:32: error: ‘struct
> > > > qla_qpair’
> > > > has
> > > > no member named ‘dsd_inuse’
> > > >           more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN,
> > > > cmd);
> > > >                                 ^~
> > > > drivers/scsi/qla2xxx/qla_iocb.c:4267:29: error: ‘struct
> > > > qla_qpair’
> > > > has
> > > > no member named ‘dsd_avail’
> > > >   if (more_dsd_lists <= qpair->dsd_avail)
> > > >                              ^~
> > > > drivers/scsi/qla2xxx/qla_iocb.c:4270:26: error: ‘struct
> > > > qla_qpair’
> > > > has
> > > > no member named ‘dsd_avail’
> > > >    more_dsd_lists -= qpair->dsd_avail;
> > > >                           ^~
> > > > drivers/scsi/qla2xxx/qla_iocb.c:4289:41: error: ‘struct
> > > > qla_qpair’
> > > > has
> > > > no member named ‘dsd_list’; did you mean ‘hints_list’?
> > > >    list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> > > >                                          ^~~~~~~~
> > > >                                          hints_list
> > > > drivers/scsi/qla2xxx/qla_iocb.c:4290:8: error: ‘struct
> > > > qla_qpair’
> > > > has
> > > > no member named ‘dsd_avail’
> > > >    qpair->dsd_avail++;
> > > 
> > > I see where this was done, there are two more patches
> > > So I will add those and test 
> > > [PATCH 1/2] qla2xxx: Move resource to allow code reuse
> > > > 
> > > 
> > 
> > OK, I was able to now build and and test this.
> > To be honest there are a lot of changes.
> > I just tested the original issue for which I had sent my first
> > patch.
> > 
> > Unfortunately the test fails in a different way to the orginal BUG
> > but
> > this may be firmware behavior on the MSA2050 that takes the device
> > paths away.
> > This is because the test passes on an LIO target
> > 
> > Against 6.5-rc4
> > 
> > Test notes
> > segstorage3 6.5.0-rc4+
> > Note that on a MSA2050 it will hang and timeout no more panics as
> > expected. However as noted we lose devices.
> > 
> > Starts out fine
> > 
> > mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
> > size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
> > > -+- policy='service-time 0' prio=50 status=active
> > > > - 1:0:1:4   sdh       8:112   active ready     running
> > > > - 1:0:4:4   sdaf      65:240  active ready     running
> > > > - 3:0:2:4   sdfk      130:96  active ready     running
> > > `- 3:0:3:4   sdft      130:240 active ready     running
> > `-+- policy='service-time 0' prio=10 status=enabled
> >   |- 1:0:2:4   sdp       8:240   active ready     running
> >   |- 1:0:3:4   sdx       65:112  active ready     running
> >   |- 3:0:1:4   sdfb      129:208 active ready     running
> >   `- 3:0:4:4   sdge      131:160 active ready     running
> > 
> > Do the test
> > 
> > [root@segstorage3 ~]# sg_write_same -T --lba=1 /dev/mapper/mpathbp
> > Write same: transport: Host_status=0x03 [DID_TIME_OUT]
> > Driver_status=0x00 [DRIVER_OK]
> > 
> > It then caused problems with two of my multipath device paths but
> > perhaps this is array firmware behavior.
> > 
> > [  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
> > hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
> > [  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready 00
> > 00
> > 00 00 00 00
> > 
> > [  825.926075] sd 3:0:2:0: rejecting I/O to offline device
> > [  825.926136] sd 3:0:2:1: rejecting I/O to offline device
> > [  825.926165] sd 3:0:2:3: rejecting I/O to offline device
> > 
> > And two paths get lost 
> > 
> > mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
> > size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
> > > -+- policy='service-time 0' prio=50 status=active
> > > > - 1:0:4:4   sdaf      65:240  active ready     running
> > > `- 3:0:3:4   sdft      130:240 active ready     running
> > `-+- policy='service-time 0' prio=10 status=enabled
> >   |- 1:0:2:4   sdp       8:240   active ready     running
> >   |- 1:0:3:4   sdx       65:112  active ready     running
> >   |- 3:0:1:4   sdfb      129:208 active ready     running
> >   `- 3:0:4:4   sdge      131:160 active ready     running
> > 
> > I re-scanned but sdh and sdfk are gone from the device tree and do
> > not
> > come back with a rescan.
> > 
> > [  846.043511] sd 1:0:1:4: [sdh] Synchronizing SCSI cache
> > [  846.043536] sd 1:0:1:4: [sdh] Synchronize Cache(10) failed:
> > Result:
> > hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> > 
> > [  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
> > hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
> > [  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready 00
> > 00
> > 00 00 00 00
> > [  851.166451] sd 3:0:2:4: [sdfk] Synchronizing SCSI cache
> > [  851.166477] sd 3:0:2:4: [sdfk] Synchronize Cache(10) failed:
> > Result:
> > hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> > 
> > Now using an LIO target via tcm_qla2xxx
> > 
> > mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
> > size=1.0G features='0' hwhandler='1 alua' wp=rw
> > `-+- policy='service-time 0' prio=50 status=active
> >   |- 1:0:5:105 sdbv      68:144  active ready     running
> >   |- 1:0:6:105 sdei      128:160 active ready     running
> >   |- 3:0:5:105 sdhz      134:144 active ready     running
> >   `- 3:0:6:105 sdkm      66:416  active ready     running
> > 
> > [root@segstorage3 block]# sg_write_same -T --lba=1
> > /dev/mapper/mpathy
> > Write same: transport: Host_status=0x07 [DID_ERROR]
> > Driver_status=0x08 [DRIVER_SENSE]
> > 
> > No issues with the LIO device paths 
> > 
> > [root@segstorage3 block]# multipath -ll mpathy
> > mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
> > size=1.0G features='0' hwhandler='1 alua' wp=rw
> > `-+- policy='service-time 0' prio=50 status=active
> >   |- 1:0:5:105 sdbv      68:144  active ready     running
> >   |- 1:0:6:105 sdei      128:160 active ready     running
> >   |- 3:0:5:105 sdhz      134:144 active ready     running
> >   `- 3:0:6:105 sdkm      66:416  active ready     running
> > 
> > So I would like to give a positive test result but will need you
> > folks
> > at Marvell to maybe first work with HPE on the MSA2050 behavior.
> > 
> > Thanks
> > Laurence
> Hello Nilesh and Marvell
> 
> I ran a few more tests, this 32 bytes CDB patch set totally breaks
> the
> MSA2050 such that a full storage side reboot of the controller pair
> is
> needed to get the paths back. 
> 
> For that reason I am going to NACK this until you chat with HPE.
> Those MSA2050's are installed everywhere.
> 
> Not pushing my original fix, but my patch would land up with the
> MSA2050 timing out but then all would function as it should after the
> timeout. Something about your change is not making the MSA2050 happy
> at
> all.
> 
> Thanks
> Laurence
> 
> 
> 

Turns out the patch I had sent earlier also makes the MSA2050 unhappy
so I will reach out to HPE about your submission and the behavior and
get back to you.

Again, the LIO seems to work fine with your patch set.

This patch below also breaks the MSA, I guess I did not not see it
during my first set of tests


diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
b/drivers/scsi/qla2xxx/qla_iocb.c
index 730d8609276c..32809e556ec6 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -1386,7 +1386,8 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct
cmd_type_crc_2 *cmd_pkt,
        if ((scsi_get_prot_op(cmd) == SCSI_PROT_READ_INSERT) ||
            (scsi_get_prot_op(cmd) == SCSI_PROT_WRITE_STRIP) ||
            (scsi_get_prot_op(cmd) == SCSI_PROT_READ_STRIP) ||
-           (scsi_get_prot_op(cmd) == SCSI_PROT_WRITE_INSERT))
+           (scsi_get_prot_op(cmd) == SCSI_PROT_WRITE_INSERT) ||
+           (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL))
                bundling = 0;
 
        /* Allocate CRC context from global pool */
@@ -1448,6 +1449,9 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct
cmd_type_crc_2 *cmd_pkt,
        dif_bytes = (data_bytes / blk_size) * 8;
 
        switch (scsi_get_prot_op(GET_CMD_SP(sp))) {
+       case SCSI_PROT_NORMAL:
+               total_bytes = data_bytes;
+               break;
        case SCSI_PROT_READ_INSERT:
        case SCSI_PROT_WRITE_STRIP:
                total_bytes = data_bytes;
Laurence Oberman Aug. 10, 2023, 2:38 p.m. UTC | #7
On Sat, 2023-08-05 at 14:16 -0400, Laurence Oberman wrote:
> On Sat, 2023-08-05 at 12:54 -0400, Laurence Oberman wrote:
> > On Fri, 2023-08-04 at 17:35 -0400, Laurence Oberman wrote:
> > > On Fri, 2023-08-04 at 14:35 -0400, Laurence Oberman wrote:
> > > > On Fri, 2023-08-04 at 13:51 -0400, Laurence Oberman wrote:
> > > > > On Fri, 2023-08-04 at 12:58 -0400, Laurence Oberman wrote:
> > > > > > On Fri, 2023-08-04 at 12:49 +0530, Nilesh Javali wrote:
> > > > > > > From: Quinn Tran <qutran@marvell.com>
> > > > > > > 
> > > > > > > System crash when 32 bytes CDB was sent to a non T10-PI
> > > > > > > disk,
> > > > > > > 
> > > > > > > [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0
> > > > > > > [qla2xxx]
> > > > > > > [  177.149165]  ? internal_add_timer+0x42/0x70
> > > > > > > [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0
> > > > > > > [qla2xxx]
> > > > > > > [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> > > > > > > [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> > > > > > > 
> > > > > > > Current code attempt to use CRC IOCB to send the command
> > > > > > > but
> > > > > > > failed.
> > > > > > > Instead, type 6 IOCB should be used to send the IO.
> > > > > > > 
> > > > > > > Clone existing type 6 IOCB code with addition of MQ
> > > > > > > support
> > > > > > > to allow 32 bytes CDB to go through.
> > > > > > > 
> > > > > > > Signed-off-by: Quinn Tran <qutran@marvell.com>
> > > > > > > Cc: Laurence Oberman <loberman@redhat.com>
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > > > > > > ---
> > > > > > >  drivers/scsi/qla2xxx/qla_iocb.c | 270
> > > > > > > ++++++++++++++++++++++++++++++++
> > > > > > >  drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
> > > > > > >  2 files changed, 273 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > > b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > > index 0caa64a7df26..e99ebf7e1c7a 100644
> > > > > > > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > > > > > > @@ -11,6 +11,7 @@
> > > > > > >  
> > > > > > >  #include <scsi/scsi_tcq.h>
> > > > > > >  
> > > > > > > +static int qla_start_scsi_type6(srb_t *sp);
> > > > > > >  /**
> > > > > > >   * qla2x00_get_cmd_direction() - Determine control_flag
> > > > > > > data
> > > > > > > direction.
> > > > > > >   * @sp: SCSI command
> > > > > > > @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
> > > > > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > > > > >                 if (cmd->cmd_len <= 16)
> > > > > > >                         return qla24xx_start_scsi(sp);
> > > > > > > +               else
> > > > > > > +                       return qla_start_scsi_type6(sp);
> > > > > > >         }
> > > > > > >  
> > > > > > >         /* Setup device pointers. */
> > > > > > > @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t
> > > > > > > *sp)
> > > > > > >         if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
> > > > > > >                 if (cmd->cmd_len <= 16)
> > > > > > >                         return qla2xxx_start_scsi_mq(sp);
> > > > > > > +               else
> > > > > > > +                       return qla_start_scsi_type6(sp);
> > > > > > >         }
> > > > > > >  
> > > > > > >         spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > > > > @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp,
> > > > > > > struct
> > > > > > > scsi_qla_host *vha, uint32_t tot_dsds)
> > > > > > >  
> > > > > > >         return rval;
> > > > > > >  }
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * qla_start_scsi_type6() - Send a SCSI command to the
> > > > > > > ISP
> > > > > > > + * @sp: command to send to the ISP
> > > > > > > + *
> > > > > > > + * Returns non-zero if a failure occurred, else zero.
> > > > > > > + */
> > > > > > > +static int
> > > > > > > +qla_start_scsi_type6(srb_t *sp)
> > > > > > > +{
> > > > > > > +       int             nseg;
> > > > > > > +       unsigned long   flags;
> > > > > > > +       uint32_t        *clr_ptr;
> > > > > > > +       uint32_t        handle;
> > > > > > > +       struct cmd_type_6 *cmd_pkt;
> > > > > > > +       uint16_t        cnt;
> > > > > > > +       uint16_t        req_cnt;
> > > > > > > +       uint16_t        tot_dsds;
> > > > > > > +       struct req_que *req = NULL;
> > > > > > > +       struct rsp_que *rsp;
> > > > > > > +       struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> > > > > > > +       struct scsi_qla_host *vha = sp->fcport->vha;
> > > > > > > +       struct qla_hw_data *ha = vha->hw;
> > > > > > > +       struct qla_qpair *qpair = sp->qpair;
> > > > > > > +       uint16_t more_dsd_lists = 0;
> > > > > > > +       struct dsd_dma *dsd_ptr;
> > > > > > > +       uint16_t i;
> > > > > > > +       __be32 *fcp_dl;
> > > > > > > +       uint8_t additional_cdb_len;
> > > > > > > +       struct ct6_dsd *ctx;
> > > > > > > +
> > > > > > > +
> > > > > > > +       /* Acquire qpair specific lock */
> > > > > > > +       spin_lock_irqsave(&qpair->qp_lock, flags);
> > > > > > > +
> > > > > > > +       /* Setup qpair pointers */
> > > > > > > +       req = qpair->req;
> > > > > > > +       rsp = qpair->rsp;
> > > > > > > +
> > > > > > > +       /* So we know we haven't pci_map'ed anything yet
> > > > > > > */
> > > > > > > +       tot_dsds = 0;
> > > > > > > +
> > > > > > > +       /* Send marker if required */
> > > > > > > +       if (vha->marker_needed != 0) {
> > > > > > > +               if (__qla2x00_marker(vha, qpair, 0, 0,
> > > > > > > MK_SYNC_ALL)
> > > > > > > != QLA_SUCCESS) {
> > > > > > > +                       spin_unlock_irqrestore(&qpair-
> > > > > > > > qp_lock,
> > > > > > > flags);
> > > > > > > +                       return QLA_FUNCTION_FAILED;
> > > > > > > +               }
> > > > > > > +               vha->marker_needed = 0;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       handle = qla2xxx_get_next_handle(req);
> > > > > > > +       if (handle == 0)
> > > > > > > +               goto queuing_error;
> > > > > > > +
> > > > > > > +       /* Map the sg table so we have an accurate count
> > > > > > > of
> > > > > > > sg
> > > > > > > entries needed */
> > > > > > > +       if (scsi_sg_count(cmd)) {
> > > > > > > +               nseg = dma_map_sg(&ha->pdev->dev,
> > > > > > > scsi_sglist(cmd),
> > > > > > > +                                 scsi_sg_count(cmd),
> > > > > > > cmd-
> > > > > > > > sc_data_direction);
> > > > > > > +               if (unlikely(!nseg))
> > > > > > > +                       goto queuing_error;
> > > > > > > +       } else {
> > > > > > > +               nseg = 0;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       tot_dsds = nseg;
> > > > > > > +
> > > > > > > +       /* eventhough driver only need 1 T6 IOCB, FW
> > > > > > > still
> > > > > > > convert
> > > > > > > DSD to Continueation IOCB */
> > > > > > > +       req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> > > > > > > +
> > > > > > > +       sp->iores.res_type = RESOURCE_IOCB |
> > > > > > > RESOURCE_EXCH;
> > > > > > > +       sp->iores.exch_cnt = 1;
> > > > > > > +       sp->iores.iocb_cnt = req_cnt;
> > > > > > > +
> > > > > > > +       if (qla_get_fw_resources(sp->qpair, &sp->iores))
> > > > > > > +               goto queuing_error;
> > > > > > > +
> > > > > > > +       more_dsd_lists =
> > > > > > > qla24xx_calc_dsd_lists(tot_dsds);
> > > > > > > +       if ((more_dsd_lists + qpair->dsd_inuse) >=
> > > > > > > NUM_DSD_CHAIN)
> > > > > > > {
> > > > > > > +               ql_dbg(ql_dbg_io, vha, 0x3028,
> > > > > > > +                      "Num of DSD list %d is than %d for
> > > > > > > cmd=%p.\n",
> > > > > > > +                      more_dsd_lists + qpair->dsd_inuse,
> > > > > > > NUM_DSD_CHAIN, cmd);
> > > > > > > +               goto queuing_error;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       if (more_dsd_lists <= qpair->dsd_avail)
> > > > > > > +               goto sufficient_dsds;
> > > > > > > +       else
> > > > > > > +               more_dsd_lists -= qpair->dsd_avail;
> > > > > > > +
> > > > > > > +       for (i = 0; i < more_dsd_lists; i++) {
> > > > > > > +               dsd_ptr = kzalloc(sizeof(*dsd_ptr),
> > > > > > > GFP_ATOMIC);
> > > > > > > +               if (!dsd_ptr) {
> > > > > > > +                       ql_log(ql_log_fatal, vha, 0x3029,
> > > > > > > +                           "Failed to allocate memory
> > > > > > > for
> > > > > > > dsd_dma
> > > > > > > for cmd=%p.\n", cmd);
> > > > > > > +                       goto queuing_error;
> > > > > > > +               }
> > > > > > > +               INIT_LIST_HEAD(&dsd_ptr->list);
> > > > > > > +
> > > > > > > +               dsd_ptr->dsd_addr = dma_pool_alloc(ha-
> > > > > > > > dl_dma_pool,
> > > > > > > +                       GFP_ATOMIC, &dsd_ptr-
> > > > > > > > dsd_list_dma);
> > > > > > > +               if (!dsd_ptr->dsd_addr) {
> > > > > > > +                       kfree(dsd_ptr);
> > > > > > > +                       ql_log(ql_log_fatal, vha, 0x302a,
> > > > > > > +                           "Failed to allocate memory
> > > > > > > for
> > > > > > > dsd_addr
> > > > > > > for cmd=%p.\n", cmd);
> > > > > > > +                       goto queuing_error;
> > > > > > > +               }
> > > > > > > +               list_add_tail(&dsd_ptr->list, &qpair-
> > > > > > > > dsd_list);
> > > > > > > +               qpair->dsd_avail++;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +sufficient_dsds:
> > > > > > > +       req_cnt = 1;
> > > > > > > +
> > > > > > > +       if (req->cnt < (req_cnt + 2)) {
> > > > > > > +               if (IS_SHADOW_REG_CAPABLE(ha)) {
> > > > > > > +                       cnt = *req->out_ptr;
> > > > > > > +               } else {
> > > > > > > +                       cnt =
> > > > > > > (uint16_t)rd_reg_dword_relaxed(req-
> > > > > > > > req_q_out);
> > > > > > > +                       if
> > > > > > > (qla2x00_check_reg16_for_disconnect(vha,
> > > > > > > cnt))
> > > > > > > +                               goto queuing_error;
> > > > > > > +               }
> > > > > > > +
> > > > > > > +               if (req->ring_index < cnt)
> > > > > > > +                       req->cnt = cnt - req->ring_index;
> > > > > > > +               else
> > > > > > > +                       req->cnt = req->length - (req-
> > > > > > > > ring_index
> > > > > > > -
> > > > > > > cnt);
> > > > > > > +               if (req->cnt < (req_cnt + 2))
> > > > > > > +                       goto queuing_error;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       ctx = &sp->u.scmd.ct6_ctx;
> > > > > > > +
> > > > > > > +       memset(ctx, 0, sizeof(struct ct6_dsd));
> > > > > > > +       ctx->fcp_cmnd = dma_pool_zalloc(ha-
> > > > > > > > fcp_cmnd_dma_pool,
> > > > > > > +               GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> > > > > > > +       if (!ctx->fcp_cmnd) {
> > > > > > > +               ql_log(ql_log_fatal, vha, 0x3031,
> > > > > > > +                   "Failed to allocate fcp_cmnd for
> > > > > > > cmd=%p.\n",
> > > > > > > cmd);
> > > > > > > +               goto queuing_error;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       /* Initialize the DSD list and dma handle */
> > > > > > > +       INIT_LIST_HEAD(&ctx->dsd_list);
> > > > > > > +       ctx->dsd_use_cnt = 0;
> > > > > > > +
> > > > > > > +       if (cmd->cmd_len > 16) {
> > > > > > > +               additional_cdb_len = cmd->cmd_len - 16;
> > > > > > > +               if (cmd->cmd_len % 4 ||
> > > > > > > +                   cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> > > > > > > +                       /* SCSI command bigger than 16
> > > > > > > bytes
> > > > > > > must
> > > > > > > be
> > > > > > > +                        * multiple of 4 or too big.
> > > > > > > +                        */
> > > > > > > +                       ql_log(ql_log_warn, vha, 0x3033,
> > > > > > > +                           "scsi cmd len %d not multiple
> > > > > > > of
> > > > > > > 4
> > > > > > > for
> > > > > > > cmd=%p.\n",
> > > > > > > +                           cmd->cmd_len, cmd);
> > > > > > > +                       goto queuing_error_fcp_cmnd;
> > > > > > > +               }
> > > > > > > +               ctx->fcp_cmnd_len = 12 + cmd->cmd_len +
> > > > > > > 4;
> > > > > > > +       } else {
> > > > > > > +               additional_cdb_len = 0;
> > > > > > > +               ctx->fcp_cmnd_len = 12 + 16 + 4;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       /* Build command packet. */
> > > > > > > +       req->current_outstanding_cmd = handle;
> > > > > > > +       req->outstanding_cmds[handle] = sp;
> > > > > > > +       sp->handle = handle;
> > > > > > > +       cmd->host_scribble = (unsigned char *)(unsigned
> > > > > > > long)handle;
> > > > > > > +       req->cnt -= req_cnt;
> > > > > > > +
> > > > > > > +       cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> > > > > > > +       cmd_pkt->handle = make_handle(req->id, handle);
> > > > > > > +
> > > > > > > +       /* Zero out remaining portion of packet. */
> > > > > > > +       /*    tagged queuing modifier -- default is
> > > > > > > TSK_SIMPLE
> > > > > > > (0).
> > > > > > > */
> > > > > > > +       clr_ptr = (uint32_t *)cmd_pkt + 2;
> > > > > > > +       memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> > > > > > > +       cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> > > > > > > +
> > > > > > > +       /* Set NPORT-ID and LUN number*/
> > > > > > > +       cmd_pkt->nport_handle = cpu_to_le16(sp->fcport-
> > > > > > > > loop_id);
> > > > > > > +       cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> > > > > > > +       cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> > > > > > > +       cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> > > > > > > +       cmd_pkt->vp_index = sp->vha->vp_idx;
> > > > > > > +
> > > > > > > +       /* Build IOCB segments */
> > > > > > > +       qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt,
> > > > > > > tot_dsds);
> > > > > > > +
> > > > > > > +       int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> > > > > > > +       host_to_fcp_swap((uint8_t *)&cmd_pkt->lun,
> > > > > > > sizeof(cmd_pkt-
> > > > > > > > lun));
> > > > > > > +
> > > > > > > +       /* build FCP_CMND IU */
> > > > > > > +       int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd-
> > > > > > > > lun);
> > > > > > > +       ctx->fcp_cmnd->additional_cdb_len =
> > > > > > > additional_cdb_len;
> > > > > > > +
> > > > > > > +       if (cmd->sc_data_direction == DMA_TO_DEVICE)
> > > > > > > +               ctx->fcp_cmnd->additional_cdb_len |= 1;
> > > > > > > +       else if (cmd->sc_data_direction ==
> > > > > > > DMA_FROM_DEVICE)
> > > > > > > +               ctx->fcp_cmnd->additional_cdb_len |= 2;
> > > > > > > +
> > > > > > > +       /* Populate the FCP_PRIO. */
> > > > > > > +       if (ha->flags.fcp_prio_enabled)
> > > > > > > +               ctx->fcp_cmnd->task_attribute |=
> > > > > > > +                   sp->fcport->fcp_prio << 3;
> > > > > > > +
> > > > > > > +       memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd-
> > > > > > > > cmd_len);
> > > > > > > +
> > > > > > > +       fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> > > > > > > +           additional_cdb_len);
> > > > > > > +       *fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> > > > > > > +
> > > > > > > +       cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx-
> > > > > > > > fcp_cmnd_len);
> > > > > > > +       put_unaligned_le64(ctx->fcp_cmnd_dma,
> > > > > > > +                          &cmd_pkt-
> > > > > > > > fcp_cmnd_dseg_address);
> > > > > > > +
> > > > > > > +       sp->flags |= SRB_FCP_CMND_DMA_VALID;
> > > > > > > +       cmd_pkt->byte_count =
> > > > > > > cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> > > > > > > +       /* Set total data segment count. */
> > > > > > > +       cmd_pkt->entry_count = (uint8_t)req_cnt;
> > > > > > > +
> > > > > > > +       wmb();
> > > > > > > +       /* Adjust ring index. */
> > > > > > > +       req->ring_index++;
> > > > > > > +       if (req->ring_index == req->length) {
> > > > > > > +               req->ring_index = 0;
> > > > > > > +               req->ring_ptr = req->ring;
> > > > > > > +       } else {
> > > > > > > +               req->ring_ptr++;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       sp->qpair->cmd_cnt++;
> > > > > > > +       sp->flags |= SRB_DMA_VALID;
> > > > > > > +
> > > > > > > +       /* Set chip new ring index. */
> > > > > > > +       wrt_reg_dword(req->req_q_in, req->ring_index);
> > > > > > > +
> > > > > > > +       /* Manage unprocessed RIO/ZIO commands in
> > > > > > > response
> > > > > > > queue.
> > > > > > > */
> > > > > > > +       if (vha->flags.process_response_queue &&
> > > > > > > +           rsp->ring_ptr->signature !=
> > > > > > > RESPONSE_PROCESSED)
> > > > > > > +               qla24xx_process_response_queue(vha, rsp);
> > > > > > > +
> > > > > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > > > > +
> > > > > > > +       return QLA_SUCCESS;
> > > > > > > +
> > > > > > > +queuing_error_fcp_cmnd:
> > > > > > > +       dma_pool_free(ha->fcp_cmnd_dma_pool, ctx-
> > > > > > > >fcp_cmnd,
> > > > > > > ctx-
> > > > > > > > fcp_cmnd_dma);
> > > > > > > +
> > > > > > > +queuing_error:
> > > > > > > +       if (tot_dsds)
> > > > > > > +               scsi_dma_unmap(cmd);
> > > > > > > +
> > > > > > > +       qla_put_fw_resources(sp->qpair, &sp->iores);
> > > > > > > +
> > > > > > > +       if (sp->u.scmd.crc_ctx) {
> > > > > > > +               mempool_free(sp->u.scmd.crc_ctx, ha-
> > > > > > > > ctx_mempool);
> > > > > > > +               sp->u.scmd.crc_ctx = NULL;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       spin_unlock_irqrestore(&qpair->qp_lock, flags);
> > > > > > > +
> > > > > > > +       return QLA_FUNCTION_FAILED;
> > > > > > > +}
> > > > > > > diff --git a/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > > b/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > > index 6dc80c8ddf79..5d1bdc15b75c 100644
> > > > > > > --- a/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > > +++ b/drivers/scsi/qla2xxx/qla_nx.h
> > > > > > > @@ -857,7 +857,9 @@ struct fcp_cmnd {
> > > > > > >         uint8_t task_attribute;
> > > > > > >         uint8_t task_management;
> > > > > > >         uint8_t additional_cdb_len;
> > > > > > > -       uint8_t cdb[260]; /* 256 for CDB len and 4 for
> > > > > > > FCP_DL
> > > > > > > */
> > > > > > > +#define QLA_CDB_BUF_SIZE  256
> > > > > > > +#define QLA_FCP_DL_SIZE   4
> > > > > > > +       uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE];
> > > > > > > /*
> > > > > > > 256
> > > > > > > for
> > > > > > > CDB len and 4 for FCP_DL */
> > > > > > >  };
> > > > > > >  
> > > > > > >  struct dsd_dma {
> > > > > > 
> > > > > > QT and Nilesh, Thank you.
> > > > > > I need more time to look at this fully but I will get it
> > > > > > tested
> > > > > > in
> > > > > > the
> > > > > > meantime. 
> > > > > > I will reply with a review or questions later.
> > > > > > 
> > > > > > Thanks for sending this
> > > > > > Laurence
> > > > > 
> > > > > @QT and Nilesh
> > > > > Which kernel was this against.
> > > > > Did you guys test this only agains your driver or also
> > > > > upstream
> > > > > I cannot build it clean because There is no structure member
> > > > > dsd_inuse
> > > > > 
> > > > > Can you send one for upstream please
> > > > > 
> > > > > Patch applied fine to 6.5-rc4
> > > > > 
> > > > > [loberman@segstorage3 linux]$ patch -p1 < qt.patch 
> > > > > patching file drivers/scsi/qla2xxx/qla_iocb.c
> > > > > Hunk #2 succeeded at 1718 (offset -4 lines).
> > > > > Hunk #3 succeeded at 2099 (offset -4 lines).
> > > > > Hunk #4 succeeded at 4179 (offset -31 lines).
> > > > > patching file drivers/scsi/qla2xxx/qla_nx.h
> > > > > 
> > > > > 
> > > > > Build fails
> > > > > 
> > > > > drivers/scsi/qla2xxx/qla_iocb.c: In function
> > > > > ‘qla_start_scsi_type6’:
> > > > > drivers/scsi/qla2xxx/qla_iocb.c:4260:29: error: ‘struct
> > > > > qla_qpair’
> > > > > has
> > > > > no member named ‘dsd_inuse’
> > > > >   if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> > > > >                              ^~
> > > > > drivers/scsi/qla2xxx/qla_iocb.c:4263:32: error: ‘struct
> > > > > qla_qpair’
> > > > > has
> > > > > no member named ‘dsd_inuse’
> > > > >           more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN,
> > > > > cmd);
> > > > >                                 ^~
> > > > > drivers/scsi/qla2xxx/qla_iocb.c:4267:29: error: ‘struct
> > > > > qla_qpair’
> > > > > has
> > > > > no member named ‘dsd_avail’
> > > > >   if (more_dsd_lists <= qpair->dsd_avail)
> > > > >                              ^~
> > > > > drivers/scsi/qla2xxx/qla_iocb.c:4270:26: error: ‘struct
> > > > > qla_qpair’
> > > > > has
> > > > > no member named ‘dsd_avail’
> > > > >    more_dsd_lists -= qpair->dsd_avail;
> > > > >                           ^~
> > > > > drivers/scsi/qla2xxx/qla_iocb.c:4289:41: error: ‘struct
> > > > > qla_qpair’
> > > > > has
> > > > > no member named ‘dsd_list’; did you mean ‘hints_list’?
> > > > >    list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> > > > >                                          ^~~~~~~~
> > > > >                                          hints_list
> > > > > drivers/scsi/qla2xxx/qla_iocb.c:4290:8: error: ‘struct
> > > > > qla_qpair’
> > > > > has
> > > > > no member named ‘dsd_avail’
> > > > >    qpair->dsd_avail++;
> > > > 
> > > > I see where this was done, there are two more patches
> > > > So I will add those and test 
> > > > [PATCH 1/2] qla2xxx: Move resource to allow code reuse
> > > > > 
> > > > 
> > > 
> > > OK, I was able to now build and and test this.
> > > To be honest there are a lot of changes.
> > > I just tested the original issue for which I had sent my first
> > > patch.
> > > 
> > > Unfortunately the test fails in a different way to the orginal
> > > BUG
> > > but
> > > this may be firmware behavior on the MSA2050 that takes the
> > > device
> > > paths away.
> > > This is because the test passes on an LIO target
> > > 
> > > Against 6.5-rc4
> > > 
> > > Test notes
> > > segstorage3 6.5.0-rc4+
> > > Note that on a MSA2050 it will hang and timeout no more panics as
> > > expected. However as noted we lose devices.
> > > 
> > > Starts out fine
> > > 
> > > mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
> > > size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
> > > > -+- policy='service-time 0' prio=50 status=active
> > > > > - 1:0:1:4   sdh       8:112   active ready     running
> > > > > - 1:0:4:4   sdaf      65:240  active ready     running
> > > > > - 3:0:2:4   sdfk      130:96  active ready     running
> > > > `- 3:0:3:4   sdft      130:240 active ready     running
> > > `-+- policy='service-time 0' prio=10 status=enabled
> > >   |- 1:0:2:4   sdp       8:240   active ready     running
> > >   |- 1:0:3:4   sdx       65:112  active ready     running
> > >   |- 3:0:1:4   sdfb      129:208 active ready     running
> > >   `- 3:0:4:4   sdge      131:160 active ready     running
> > > 
> > > Do the test
> > > 
> > > [root@segstorage3 ~]# sg_write_same -T --lba=1
> > > /dev/mapper/mpathbp
> > > Write same: transport: Host_status=0x03 [DID_TIME_OUT]
> > > Driver_status=0x00 [DRIVER_OK]
> > > 
> > > It then caused problems with two of my multipath device paths but
> > > perhaps this is array firmware behavior.
> > > 
> > > [  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
> > > hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
> > > [  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready
> > > 00
> > > 00
> > > 00 00 00 00
> > > 
> > > [  825.926075] sd 3:0:2:0: rejecting I/O to offline device
> > > [  825.926136] sd 3:0:2:1: rejecting I/O to offline device
> > > [  825.926165] sd 3:0:2:3: rejecting I/O to offline device
> > > 
> > > And two paths get lost 
> > > 
> > > mpathbp (3600c0ff00043feab07a7ff6307000000) dm-8 HPE,MSA 2050 SAN
> > > size=9.3G features='1 queue_if_no_path' hwhandler='1 alua' wp=rw
> > > > -+- policy='service-time 0' prio=50 status=active
> > > > > - 1:0:4:4   sdaf      65:240  active ready     running
> > > > `- 3:0:3:4   sdft      130:240 active ready     running
> > > `-+- policy='service-time 0' prio=10 status=enabled
> > >   |- 1:0:2:4   sdp       8:240   active ready     running
> > >   |- 1:0:3:4   sdx       65:112  active ready     running
> > >   |- 3:0:1:4   sdfb      129:208 active ready     running
> > >   `- 3:0:4:4   sdge      131:160 active ready     running
> > > 
> > > I re-scanned but sdh and sdfk are gone from the device tree and
> > > do
> > > not
> > > come back with a rescan.
> > > 
> > > [  846.043511] sd 1:0:1:4: [sdh] Synchronizing SCSI cache
> > > [  846.043536] sd 1:0:1:4: [sdh] Synchronize Cache(10) failed:
> > > Result:
> > > hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> > > 
> > > [  820.836495] sd 3:0:2:4: [sdfk] tag#1704 FAILED Result:
> > > hostbyte=DID_TRANSPORT_DISRUPTED driverbyte=DRIVER_OK cmd_age=23s
> > > [  820.836497] sd 3:0:2:4: [sdfk] tag#1704 CDB: Test Unit Ready
> > > 00
> > > 00
> > > 00 00 00 00
> > > [  851.166451] sd 3:0:2:4: [sdfk] Synchronizing SCSI cache
> > > [  851.166477] sd 3:0:2:4: [sdfk] Synchronize Cache(10) failed:
> > > Result:
> > > hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> > > 
> > > Now using an LIO target via tcm_qla2xxx
> > > 
> > > mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
> > > size=1.0G features='0' hwhandler='1 alua' wp=rw
> > > `-+- policy='service-time 0' prio=50 status=active
> > >   |- 1:0:5:105 sdbv      68:144  active ready     running
> > >   |- 1:0:6:105 sdei      128:160 active ready     running
> > >   |- 3:0:5:105 sdhz      134:144 active ready     running
> > >   `- 3:0:6:105 sdkm      66:416  active ready     running
> > > 
> > > [root@segstorage3 block]# sg_write_same -T --lba=1
> > > /dev/mapper/mpathy
> > > Write same: transport: Host_status=0x07 [DID_ERROR]
> > > Driver_status=0x08 [DRIVER_SENSE]
> > > 
> > > No issues with the LIO device paths 
> > > 
> > > [root@segstorage3 block]# multipath -ll mpathy
> > > mpathy (3600140549ed41778d414038bf1f1bb42) dm-74 LIO-ORG,test-70
> > > size=1.0G features='0' hwhandler='1 alua' wp=rw
> > > `-+- policy='service-time 0' prio=50 status=active
> > >   |- 1:0:5:105 sdbv      68:144  active ready     running
> > >   |- 1:0:6:105 sdei      128:160 active ready     running
> > >   |- 3:0:5:105 sdhz      134:144 active ready     running
> > >   `- 3:0:6:105 sdkm      66:416  active ready     running
> > > 
> > > So I would like to give a positive test result but will need you
> > > folks
> > > at Marvell to maybe first work with HPE on the MSA2050 behavior.
> > > 
> > > Thanks
> > > Laurence
> > Hello Nilesh and Marvell
> > 
> > I ran a few more tests, this 32 bytes CDB patch set totally breaks
> > the
> > MSA2050 such that a full storage side reboot of the controller pair
> > is
> > needed to get the paths back. 
> > 
> > For that reason I am going to NACK this until you chat with HPE.
> > Those MSA2050's are installed everywhere.
> > 
> > Not pushing my original fix, but my patch would land up with the
> > MSA2050 timing out but then all would function as it should after
> > the
> > timeout. Something about your change is not making the MSA2050
> > happy
> > at
> > all.
> > 
> > Thanks
> > Laurence
> > 
> > 
> > 
> 
> Turns out the patch I had sent earlier also makes the MSA2050 unhappy
> so I will reach out to HPE about your submission and the behavior and
> get back to you.
> 
> Again, the LIO seems to work fine with your patch set.
> 
> This patch below also breaks the MSA, I guess I did not not see it
> during my first set of tests
> 
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
> b/drivers/scsi/qla2xxx/qla_iocb.c
> index 730d8609276c..32809e556ec6 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -1386,7 +1386,8 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp,
> struct
> cmd_type_crc_2 *cmd_pkt,
>         if ((scsi_get_prot_op(cmd) == SCSI_PROT_READ_INSERT) ||
>             (scsi_get_prot_op(cmd) == SCSI_PROT_WRITE_STRIP) ||
>             (scsi_get_prot_op(cmd) == SCSI_PROT_READ_STRIP) ||
> -           (scsi_get_prot_op(cmd) == SCSI_PROT_WRITE_INSERT))
> +           (scsi_get_prot_op(cmd) == SCSI_PROT_WRITE_INSERT) ||
> +           (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL))
>                 bundling = 0;
>  
>         /* Allocate CRC context from global pool */
> @@ -1448,6 +1449,9 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp,
> struct
> cmd_type_crc_2 *cmd_pkt,
>         dif_bytes = (data_bytes / blk_size) * 8;
>  
>         switch (scsi_get_prot_op(GET_CMD_SP(sp))) {
> +       case SCSI_PROT_NORMAL:
> +               total_bytes = data_bytes;
> +               break;
>         case SCSI_PROT_READ_INSERT:
>         case SCSI_PROT_WRITE_STRIP:
>                 total_bytes = data_bytes;
> 
Hello 

I think we can ack this patch. The MSA20250 behavior seems to be a
firmware issue specific to that array because the LIO target passes all
the tests.
I did reach out to HPE and let them know but given that without this
patch we panic a server, the patch is needed. Hopefully HPE follows up
with me.

So:

Tested-by: Laurence Oberman <loberman@redhat.com> 
noting that the MSA array sees an issue not seen on other arrays tested
so far.

Thanks
Himanshu Madhani Aug. 14, 2023, 4:38 p.m. UTC | #8
On 8/4/23 00:19, Nilesh Javali wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> System crash when 32 bytes CDB was sent to a non T10-PI disk,
> 
> [  177.143279]  ? qla2xxx_dif_start_scsi_mq+0xcd8/0xce0 [qla2xxx]
> [  177.149165]  ? internal_add_timer+0x42/0x70
> [  177.153372]  qla2xxx_mqueuecommand+0x207/0x2b0 [qla2xxx]
> [  177.158730]  scsi_queue_rq+0x2b7/0xc00
> [  177.162501]  blk_mq_dispatch_rq_list+0x3ea/0x7e0
> 
> Current code attempt to use CRC IOCB to send the command but failed.
> Instead, type 6 IOCB should be used to send the IO.
> 
> Clone existing type 6 IOCB code with addition of MQ support
> to allow 32 bytes CDB to go through.
> 
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Cc: Laurence Oberman <loberman@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>   drivers/scsi/qla2xxx/qla_iocb.c | 270 ++++++++++++++++++++++++++++++++
>   drivers/scsi/qla2xxx/qla_nx.h   |   4 +-
>   2 files changed, 273 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 0caa64a7df26..e99ebf7e1c7a 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -11,6 +11,7 @@
>   
>   #include <scsi/scsi_tcq.h>
>   
> +static int qla_start_scsi_type6(srb_t *sp);
>   /**
>    * qla2x00_get_cmd_direction() - Determine control_flag data direction.
>    * @sp: SCSI command
> @@ -1721,6 +1722,8 @@ qla24xx_dif_start_scsi(srb_t *sp)
>   	if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
>   		if (cmd->cmd_len <= 16)
>   			return qla24xx_start_scsi(sp);
> +		else
> +			return qla_start_scsi_type6(sp);
>   	}
>   
>   	/* Setup device pointers. */
> @@ -2100,6 +2103,8 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp)
>   	if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
>   		if (cmd->cmd_len <= 16)
>   			return qla2xxx_start_scsi_mq(sp);
> +		else
> +			return qla_start_scsi_type6(sp);
>   	}
>   
>   	spin_lock_irqsave(&qpair->qp_lock, flags);
> @@ -4205,3 +4210,268 @@ qla2x00_start_bidir(srb_t *sp, struct scsi_qla_host *vha, uint32_t tot_dsds)
>   
>   	return rval;
>   }
> +
> +/**
> + * qla_start_scsi_type6() - Send a SCSI command to the ISP
> + * @sp: command to send to the ISP
> + *
> + * Returns non-zero if a failure occurred, else zero.
> + */
> +static int
> +qla_start_scsi_type6(srb_t *sp)
> +{
> +	int		nseg;
> +	unsigned long   flags;
> +	uint32_t	*clr_ptr;
> +	uint32_t	handle;
> +	struct cmd_type_6 *cmd_pkt;
> +	uint16_t	cnt;
> +	uint16_t	req_cnt;
> +	uint16_t	tot_dsds;
> +	struct req_que *req = NULL;
> +	struct rsp_que *rsp;
> +	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
> +	struct scsi_qla_host *vha = sp->fcport->vha;
> +	struct qla_hw_data *ha = vha->hw;
> +	struct qla_qpair *qpair = sp->qpair;
> +	uint16_t more_dsd_lists = 0;
> +	struct dsd_dma *dsd_ptr;
> +	uint16_t i;
> +	__be32 *fcp_dl;
> +	uint8_t additional_cdb_len;
> +	struct ct6_dsd *ctx;
> +
remove extra newline
> +
> +	/* Acquire qpair specific lock */
> +	spin_lock_irqsave(&qpair->qp_lock, flags);
> +
> +	/* Setup qpair pointers */
> +	req = qpair->req;
> +	rsp = qpair->rsp;
> +
> +	/* So we know we haven't pci_map'ed anything yet */
> +	tot_dsds = 0;
> +
> +	/* Send marker if required */
> +	if (vha->marker_needed != 0) {
> +		if (__qla2x00_marker(vha, qpair, 0, 0, MK_SYNC_ALL) != QLA_SUCCESS) {
> +			spin_unlock_irqrestore(&qpair->qp_lock, flags);
> +			return QLA_FUNCTION_FAILED;
> +		}
> +		vha->marker_needed = 0;
> +	}
> +
> +	handle = qla2xxx_get_next_handle(req);
> +	if (handle == 0)
> +		goto queuing_error;
> +
> +	/* Map the sg table so we have an accurate count of sg entries needed */
> +	if (scsi_sg_count(cmd)) {
> +		nseg = dma_map_sg(&ha->pdev->dev, scsi_sglist(cmd),
> +				  scsi_sg_count(cmd), cmd->sc_data_direction);
> +		if (unlikely(!nseg))
> +			goto queuing_error;
> +	} else {
> +		nseg = 0;
> +	}
> +
> +	tot_dsds = nseg;
> +
> +	/* eventhough driver only need 1 T6 IOCB, FW still convert DSD to Continueation IOCB */
> +	req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
> +
> +	sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
> +	sp->iores.exch_cnt = 1;
> +	sp->iores.iocb_cnt = req_cnt;
> +
> +	if (qla_get_fw_resources(sp->qpair, &sp->iores))
> +		goto queuing_error;
> +
> +	more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
> +	if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
> +		ql_dbg(ql_dbg_io, vha, 0x3028,
> +		       "Num of DSD list %d is than %d for cmd=%p.\n",
> +		       more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN, cmd);
> +		goto queuing_error;
> +	}
> +
> +	if (more_dsd_lists <= qpair->dsd_avail)
> +		goto sufficient_dsds;
> +	else
> +		more_dsd_lists -= qpair->dsd_avail;
> +
> +	for (i = 0; i < more_dsd_lists; i++) {
> +		dsd_ptr = kzalloc(sizeof(*dsd_ptr), GFP_ATOMIC);
> +		if (!dsd_ptr) {
> +			ql_log(ql_log_fatal, vha, 0x3029,
> +			    "Failed to allocate memory for dsd_dma for cmd=%p.\n", cmd);
> +			goto queuing_error;
> +		}
> +		INIT_LIST_HEAD(&dsd_ptr->list);
> +
> +		dsd_ptr->dsd_addr = dma_pool_alloc(ha->dl_dma_pool,
> +			GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
> +		if (!dsd_ptr->dsd_addr) {
> +			kfree(dsd_ptr);
> +			ql_log(ql_log_fatal, vha, 0x302a,
> +			    "Failed to allocate memory for dsd_addr for cmd=%p.\n", cmd);
> +			goto queuing_error;
> +		}
> +		list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
> +		qpair->dsd_avail++;
> +	}
> +
> +sufficient_dsds:
> +	req_cnt = 1;
> +
> +	if (req->cnt < (req_cnt + 2)) {
> +		if (IS_SHADOW_REG_CAPABLE(ha)) {
> +			cnt = *req->out_ptr;
> +		} else {
> +			cnt = (uint16_t)rd_reg_dword_relaxed(req->req_q_out);
> +			if (qla2x00_check_reg16_for_disconnect(vha, cnt))
> +				goto queuing_error;
> +		}
> +
> +		if (req->ring_index < cnt)
> +			req->cnt = cnt - req->ring_index;
> +		else
> +			req->cnt = req->length - (req->ring_index - cnt);
> +		if (req->cnt < (req_cnt + 2))
> +			goto queuing_error;
> +	}
> +
> +	ctx = &sp->u.scmd.ct6_ctx;
> +
> +	memset(ctx, 0, sizeof(struct ct6_dsd));
> +	ctx->fcp_cmnd = dma_pool_zalloc(ha->fcp_cmnd_dma_pool,
> +		GFP_ATOMIC, &ctx->fcp_cmnd_dma);
> +	if (!ctx->fcp_cmnd) {
> +		ql_log(ql_log_fatal, vha, 0x3031,
> +		    "Failed to allocate fcp_cmnd for cmd=%p.\n", cmd);
> +		goto queuing_error;
> +	}
> +
> +	/* Initialize the DSD list and dma handle */
> +	INIT_LIST_HEAD(&ctx->dsd_list);
> +	ctx->dsd_use_cnt = 0;
> +
> +	if (cmd->cmd_len > 16) {
> +		additional_cdb_len = cmd->cmd_len - 16;
> +		if (cmd->cmd_len % 4 ||
> +		    cmd->cmd_len > QLA_CDB_BUF_SIZE) {
> +			/* SCSI command bigger than 16 bytes must be
> +			 * multiple of 4 or too big.
> +			 */
> 
fix comment formatting

+			ql_log(ql_log_warn, vha, 0x3033,
> +			    "scsi cmd len %d not multiple of 4 for cmd=%p.\n",
> +			    cmd->cmd_len, cmd);
> +			goto queuing_error_fcp_cmnd;
> +		}
> +		ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
> +	} else {
> +		additional_cdb_len = 0;
> +		ctx->fcp_cmnd_len = 12 + 16 + 4;
> +	}
> +
> +	/* Build command packet. */
> +	req->current_outstanding_cmd = handle;
> +	req->outstanding_cmds[handle] = sp;
> +	sp->handle = handle;
> +	cmd->host_scribble = (unsigned char *)(unsigned long)handle;
> +	req->cnt -= req_cnt;
> +
> +	cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
> +	cmd_pkt->handle = make_handle(req->id, handle);
> +
> +	/* Zero out remaining portion of packet. */
^^ remove self-explanatory comment.

> +	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */

fix comment format

> +	clr_ptr = (uint32_t *)cmd_pkt + 2;
> +	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> +	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
> +
> +	/* Set NPORT-ID and LUN number*/
> +	cmd_pkt->nport_handle = cpu_to_le16(sp->fcport->loop_id);
> +	cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> +	cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> +	cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> +	cmd_pkt->vp_index = sp->vha->vp_idx;
> +
> +	/* Build IOCB segments */
> +	qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt, tot_dsds);
> +
> +	int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
> +	host_to_fcp_swap((uint8_t *)&cmd_pkt->lun, sizeof(cmd_pkt->lun));
> +
> +	/* build FCP_CMND IU */
> +	int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd->lun);
> +	ctx->fcp_cmnd->additional_cdb_len = additional_cdb_len;
> +
> +	if (cmd->sc_data_direction == DMA_TO_DEVICE)
> +		ctx->fcp_cmnd->additional_cdb_len |= 1;
> +	else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> +		ctx->fcp_cmnd->additional_cdb_len |= 2;
> +
> +	/* Populate the FCP_PRIO. */
> +	if (ha->flags.fcp_prio_enabled)
> +		ctx->fcp_cmnd->task_attribute |=
> +		    sp->fcport->fcp_prio << 3;
> +
> +	memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
> +
> +	fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
> +	    additional_cdb_len);
> +	*fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
> +
> +	cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx->fcp_cmnd_len);
> +	put_unaligned_le64(ctx->fcp_cmnd_dma,
> +			   &cmd_pkt->fcp_cmnd_dseg_address);
> +
> +	sp->flags |= SRB_FCP_CMND_DMA_VALID;
> +	cmd_pkt->byte_count = cpu_to_le32((uint32_t)scsi_bufflen(cmd));
> +	/* Set total data segment count. */
> +	cmd_pkt->entry_count = (uint8_t)req_cnt;
> +
> +	wmb();
> +	/* Adjust ring index. */
> +	req->ring_index++;
> +	if (req->ring_index == req->length) {
> +		req->ring_index = 0;
> +		req->ring_ptr = req->ring;
> +	} else {
> +		req->ring_ptr++;
> +	}
> +
> +	sp->qpair->cmd_cnt++;
> +	sp->flags |= SRB_DMA_VALID;
> +
> +	/* Set chip new ring index. */
> +	wrt_reg_dword(req->req_q_in, req->ring_index);
> +
> +	/* Manage unprocessed RIO/ZIO commands in response queue. */
> +	if (vha->flags.process_response_queue &&
> +	    rsp->ring_ptr->signature != RESPONSE_PROCESSED)
> +		qla24xx_process_response_queue(vha, rsp);
> +
> +	spin_unlock_irqrestore(&qpair->qp_lock, flags);
> +
> +	return QLA_SUCCESS;
> +
> +queuing_error_fcp_cmnd:
> +	dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd, ctx->fcp_cmnd_dma);
> +
> +queuing_error:
> +	if (tot_dsds)
> +		scsi_dma_unmap(cmd);
> +
> +	qla_put_fw_resources(sp->qpair, &sp->iores);
> +
> +	if (sp->u.scmd.crc_ctx) {
> +		mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);
> +		sp->u.scmd.crc_ctx = NULL;
> +	}
> +
> +	spin_unlock_irqrestore(&qpair->qp_lock, flags);
> +
> +	return QLA_FUNCTION_FAILED;
> +}
> diff --git a/drivers/scsi/qla2xxx/qla_nx.h b/drivers/scsi/qla2xxx/qla_nx.h
> index 6dc80c8ddf79..5d1bdc15b75c 100644
> --- a/drivers/scsi/qla2xxx/qla_nx.h
> +++ b/drivers/scsi/qla2xxx/qla_nx.h
> @@ -857,7 +857,9 @@ struct fcp_cmnd {
>   	uint8_t task_attribute;
>   	uint8_t task_management;
>   	uint8_t additional_cdb_len;
> -	uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL */
> +#define QLA_CDB_BUF_SIZE  256
> +#define QLA_FCP_DL_SIZE   4
> +	uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /* 256 for CDB len and 4 for FCP_DL */
>   };
>   
>   struct dsd_dma {

Once you fix comments in this patch... you can add

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 0caa64a7df26..e99ebf7e1c7a 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -11,6 +11,7 @@ 
 
 #include <scsi/scsi_tcq.h>
 
+static int qla_start_scsi_type6(srb_t *sp);
 /**
  * qla2x00_get_cmd_direction() - Determine control_flag data direction.
  * @sp: SCSI command
@@ -1721,6 +1722,8 @@  qla24xx_dif_start_scsi(srb_t *sp)
 	if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
 		if (cmd->cmd_len <= 16)
 			return qla24xx_start_scsi(sp);
+		else
+			return qla_start_scsi_type6(sp);
 	}
 
 	/* Setup device pointers. */
@@ -2100,6 +2103,8 @@  qla2xxx_dif_start_scsi_mq(srb_t *sp)
 	if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) {
 		if (cmd->cmd_len <= 16)
 			return qla2xxx_start_scsi_mq(sp);
+		else
+			return qla_start_scsi_type6(sp);
 	}
 
 	spin_lock_irqsave(&qpair->qp_lock, flags);
@@ -4205,3 +4210,268 @@  qla2x00_start_bidir(srb_t *sp, struct scsi_qla_host *vha, uint32_t tot_dsds)
 
 	return rval;
 }
+
+/**
+ * qla_start_scsi_type6() - Send a SCSI command to the ISP
+ * @sp: command to send to the ISP
+ *
+ * Returns non-zero if a failure occurred, else zero.
+ */
+static int
+qla_start_scsi_type6(srb_t *sp)
+{
+	int		nseg;
+	unsigned long   flags;
+	uint32_t	*clr_ptr;
+	uint32_t	handle;
+	struct cmd_type_6 *cmd_pkt;
+	uint16_t	cnt;
+	uint16_t	req_cnt;
+	uint16_t	tot_dsds;
+	struct req_que *req = NULL;
+	struct rsp_que *rsp;
+	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
+	struct scsi_qla_host *vha = sp->fcport->vha;
+	struct qla_hw_data *ha = vha->hw;
+	struct qla_qpair *qpair = sp->qpair;
+	uint16_t more_dsd_lists = 0;
+	struct dsd_dma *dsd_ptr;
+	uint16_t i;
+	__be32 *fcp_dl;
+	uint8_t additional_cdb_len;
+	struct ct6_dsd *ctx;
+
+
+	/* Acquire qpair specific lock */
+	spin_lock_irqsave(&qpair->qp_lock, flags);
+
+	/* Setup qpair pointers */
+	req = qpair->req;
+	rsp = qpair->rsp;
+
+	/* So we know we haven't pci_map'ed anything yet */
+	tot_dsds = 0;
+
+	/* Send marker if required */
+	if (vha->marker_needed != 0) {
+		if (__qla2x00_marker(vha, qpair, 0, 0, MK_SYNC_ALL) != QLA_SUCCESS) {
+			spin_unlock_irqrestore(&qpair->qp_lock, flags);
+			return QLA_FUNCTION_FAILED;
+		}
+		vha->marker_needed = 0;
+	}
+
+	handle = qla2xxx_get_next_handle(req);
+	if (handle == 0)
+		goto queuing_error;
+
+	/* Map the sg table so we have an accurate count of sg entries needed */
+	if (scsi_sg_count(cmd)) {
+		nseg = dma_map_sg(&ha->pdev->dev, scsi_sglist(cmd),
+				  scsi_sg_count(cmd), cmd->sc_data_direction);
+		if (unlikely(!nseg))
+			goto queuing_error;
+	} else {
+		nseg = 0;
+	}
+
+	tot_dsds = nseg;
+
+	/* eventhough driver only need 1 T6 IOCB, FW still convert DSD to Continueation IOCB */
+	req_cnt = qla24xx_calc_iocbs(vha, tot_dsds);
+
+	sp->iores.res_type = RESOURCE_IOCB | RESOURCE_EXCH;
+	sp->iores.exch_cnt = 1;
+	sp->iores.iocb_cnt = req_cnt;
+
+	if (qla_get_fw_resources(sp->qpair, &sp->iores))
+		goto queuing_error;
+
+	more_dsd_lists = qla24xx_calc_dsd_lists(tot_dsds);
+	if ((more_dsd_lists + qpair->dsd_inuse) >= NUM_DSD_CHAIN) {
+		ql_dbg(ql_dbg_io, vha, 0x3028,
+		       "Num of DSD list %d is than %d for cmd=%p.\n",
+		       more_dsd_lists + qpair->dsd_inuse, NUM_DSD_CHAIN, cmd);
+		goto queuing_error;
+	}
+
+	if (more_dsd_lists <= qpair->dsd_avail)
+		goto sufficient_dsds;
+	else
+		more_dsd_lists -= qpair->dsd_avail;
+
+	for (i = 0; i < more_dsd_lists; i++) {
+		dsd_ptr = kzalloc(sizeof(*dsd_ptr), GFP_ATOMIC);
+		if (!dsd_ptr) {
+			ql_log(ql_log_fatal, vha, 0x3029,
+			    "Failed to allocate memory for dsd_dma for cmd=%p.\n", cmd);
+			goto queuing_error;
+		}
+		INIT_LIST_HEAD(&dsd_ptr->list);
+
+		dsd_ptr->dsd_addr = dma_pool_alloc(ha->dl_dma_pool,
+			GFP_ATOMIC, &dsd_ptr->dsd_list_dma);
+		if (!dsd_ptr->dsd_addr) {
+			kfree(dsd_ptr);
+			ql_log(ql_log_fatal, vha, 0x302a,
+			    "Failed to allocate memory for dsd_addr for cmd=%p.\n", cmd);
+			goto queuing_error;
+		}
+		list_add_tail(&dsd_ptr->list, &qpair->dsd_list);
+		qpair->dsd_avail++;
+	}
+
+sufficient_dsds:
+	req_cnt = 1;
+
+	if (req->cnt < (req_cnt + 2)) {
+		if (IS_SHADOW_REG_CAPABLE(ha)) {
+			cnt = *req->out_ptr;
+		} else {
+			cnt = (uint16_t)rd_reg_dword_relaxed(req->req_q_out);
+			if (qla2x00_check_reg16_for_disconnect(vha, cnt))
+				goto queuing_error;
+		}
+
+		if (req->ring_index < cnt)
+			req->cnt = cnt - req->ring_index;
+		else
+			req->cnt = req->length - (req->ring_index - cnt);
+		if (req->cnt < (req_cnt + 2))
+			goto queuing_error;
+	}
+
+	ctx = &sp->u.scmd.ct6_ctx;
+
+	memset(ctx, 0, sizeof(struct ct6_dsd));
+	ctx->fcp_cmnd = dma_pool_zalloc(ha->fcp_cmnd_dma_pool,
+		GFP_ATOMIC, &ctx->fcp_cmnd_dma);
+	if (!ctx->fcp_cmnd) {
+		ql_log(ql_log_fatal, vha, 0x3031,
+		    "Failed to allocate fcp_cmnd for cmd=%p.\n", cmd);
+		goto queuing_error;
+	}
+
+	/* Initialize the DSD list and dma handle */
+	INIT_LIST_HEAD(&ctx->dsd_list);
+	ctx->dsd_use_cnt = 0;
+
+	if (cmd->cmd_len > 16) {
+		additional_cdb_len = cmd->cmd_len - 16;
+		if (cmd->cmd_len % 4 ||
+		    cmd->cmd_len > QLA_CDB_BUF_SIZE) {
+			/* SCSI command bigger than 16 bytes must be
+			 * multiple of 4 or too big.
+			 */
+			ql_log(ql_log_warn, vha, 0x3033,
+			    "scsi cmd len %d not multiple of 4 for cmd=%p.\n",
+			    cmd->cmd_len, cmd);
+			goto queuing_error_fcp_cmnd;
+		}
+		ctx->fcp_cmnd_len = 12 + cmd->cmd_len + 4;
+	} else {
+		additional_cdb_len = 0;
+		ctx->fcp_cmnd_len = 12 + 16 + 4;
+	}
+
+	/* Build command packet. */
+	req->current_outstanding_cmd = handle;
+	req->outstanding_cmds[handle] = sp;
+	sp->handle = handle;
+	cmd->host_scribble = (unsigned char *)(unsigned long)handle;
+	req->cnt -= req_cnt;
+
+	cmd_pkt = (struct cmd_type_6 *)req->ring_ptr;
+	cmd_pkt->handle = make_handle(req->id, handle);
+
+	/* Zero out remaining portion of packet. */
+	/*    tagged queuing modifier -- default is TSK_SIMPLE (0). */
+	clr_ptr = (uint32_t *)cmd_pkt + 2;
+	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
+	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);
+
+	/* Set NPORT-ID and LUN number*/
+	cmd_pkt->nport_handle = cpu_to_le16(sp->fcport->loop_id);
+	cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
+	cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
+	cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
+	cmd_pkt->vp_index = sp->vha->vp_idx;
+
+	/* Build IOCB segments */
+	qla24xx_build_scsi_type_6_iocbs(sp, cmd_pkt, tot_dsds);
+
+	int_to_scsilun(cmd->device->lun, &cmd_pkt->lun);
+	host_to_fcp_swap((uint8_t *)&cmd_pkt->lun, sizeof(cmd_pkt->lun));
+
+	/* build FCP_CMND IU */
+	int_to_scsilun(cmd->device->lun, &ctx->fcp_cmnd->lun);
+	ctx->fcp_cmnd->additional_cdb_len = additional_cdb_len;
+
+	if (cmd->sc_data_direction == DMA_TO_DEVICE)
+		ctx->fcp_cmnd->additional_cdb_len |= 1;
+	else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
+		ctx->fcp_cmnd->additional_cdb_len |= 2;
+
+	/* Populate the FCP_PRIO. */
+	if (ha->flags.fcp_prio_enabled)
+		ctx->fcp_cmnd->task_attribute |=
+		    sp->fcport->fcp_prio << 3;
+
+	memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
+
+	fcp_dl = (__be32 *)(ctx->fcp_cmnd->cdb + 16 +
+	    additional_cdb_len);
+	*fcp_dl = htonl((uint32_t)scsi_bufflen(cmd));
+
+	cmd_pkt->fcp_cmnd_dseg_len = cpu_to_le16(ctx->fcp_cmnd_len);
+	put_unaligned_le64(ctx->fcp_cmnd_dma,
+			   &cmd_pkt->fcp_cmnd_dseg_address);
+
+	sp->flags |= SRB_FCP_CMND_DMA_VALID;
+	cmd_pkt->byte_count = cpu_to_le32((uint32_t)scsi_bufflen(cmd));
+	/* Set total data segment count. */
+	cmd_pkt->entry_count = (uint8_t)req_cnt;
+
+	wmb();
+	/* Adjust ring index. */
+	req->ring_index++;
+	if (req->ring_index == req->length) {
+		req->ring_index = 0;
+		req->ring_ptr = req->ring;
+	} else {
+		req->ring_ptr++;
+	}
+
+	sp->qpair->cmd_cnt++;
+	sp->flags |= SRB_DMA_VALID;
+
+	/* Set chip new ring index. */
+	wrt_reg_dword(req->req_q_in, req->ring_index);
+
+	/* Manage unprocessed RIO/ZIO commands in response queue. */
+	if (vha->flags.process_response_queue &&
+	    rsp->ring_ptr->signature != RESPONSE_PROCESSED)
+		qla24xx_process_response_queue(vha, rsp);
+
+	spin_unlock_irqrestore(&qpair->qp_lock, flags);
+
+	return QLA_SUCCESS;
+
+queuing_error_fcp_cmnd:
+	dma_pool_free(ha->fcp_cmnd_dma_pool, ctx->fcp_cmnd, ctx->fcp_cmnd_dma);
+
+queuing_error:
+	if (tot_dsds)
+		scsi_dma_unmap(cmd);
+
+	qla_put_fw_resources(sp->qpair, &sp->iores);
+
+	if (sp->u.scmd.crc_ctx) {
+		mempool_free(sp->u.scmd.crc_ctx, ha->ctx_mempool);
+		sp->u.scmd.crc_ctx = NULL;
+	}
+
+	spin_unlock_irqrestore(&qpair->qp_lock, flags);
+
+	return QLA_FUNCTION_FAILED;
+}
diff --git a/drivers/scsi/qla2xxx/qla_nx.h b/drivers/scsi/qla2xxx/qla_nx.h
index 6dc80c8ddf79..5d1bdc15b75c 100644
--- a/drivers/scsi/qla2xxx/qla_nx.h
+++ b/drivers/scsi/qla2xxx/qla_nx.h
@@ -857,7 +857,9 @@  struct fcp_cmnd {
 	uint8_t task_attribute;
 	uint8_t task_management;
 	uint8_t additional_cdb_len;
-	uint8_t cdb[260]; /* 256 for CDB len and 4 for FCP_DL */
+#define QLA_CDB_BUF_SIZE  256
+#define QLA_FCP_DL_SIZE   4
+	uint8_t cdb[QLA_CDB_BUF_SIZE + QLA_FCP_DL_SIZE]; /* 256 for CDB len and 4 for FCP_DL */
 };
 
 struct dsd_dma {