[Open-FCoE] [v2 PATCH 4/5] bnx2fc: Broadcom FCoE Offload driver submission - part 2

Bhanu Gollapudi bprakash at broadcom.com
Tue Jan 18 00:37:46 UTC 2011


On Sat, 2011-01-15 at 01:17 -0800, Mike Christie wrote:
> On 12/24/2010 12:02 AM, Bhanu Gollapudi wrote:
> > +
> > +static void bnx2fc_scsi_done(struct bnx2fc_cmd *io_req, int err_code)
> 
> 
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "sc=%p, result=0x%x, retries=%d, allowed=%d\n",
> > +             sc_cmd, host_byte(sc_cmd->result), sc_cmd->retries,
> > +             sc_cmd->allowed);
> > +     scsi_set_resid(sc_cmd, scsi_bufflen(sc_cmd));
> > +     sc_cmd->SCp.ptr = NULL;
> > +     if (sc_cmd->scsi_done)
> > +             sc_cmd->scsi_done(sc_cmd);
> 
> 
> 
> It seems scsi_done should always be set. In newer kernels scsi-ml sets
> this too, so you should remove the checks.

OK. 

> 
> 
> 
> > +
> > +u16 bnx2fc_get_xid(struct bnx2fc_cmd *io_req, struct bnx2fc_hba *hba)
> > +{
> > +     u16 xid;
> > +     xid = io_req->xid;
> > +     return xid;
> > +}
> 
> Kind of a useless function.

Ya, I should remove this.

> 
> 
> > +
> > +static void bnx2fc_cmd_hold(struct bnx2fc_cmd *io_req)
> > +{
> > +     atomic_inc(&io_req->cmd_refcnt);
> > +}
> 
> 
> Use krefs.

OK.

> 
> 
> > +
> > +int bnx2fc_init_mp_req(struct bnx2fc_cmd *io_req)
> > +{
> > +     struct bnx2fc_mp_req *mp_req;
> > +     struct fcoe_bd_ctx *mp_req_bd;
> > +     struct fcoe_bd_ctx *mp_resp_bd;
> > +     struct bnx2fc_hba *hba = io_req->port->hba;
> > +     dma_addr_t addr;
> > +     size_t sz;
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "Entered bnx2fc_init_mp_req\n");
> > +
> 
> 
> For your logging and printks, I think you want to add the host or rport
> that is having the problem.

OK. I can chance the debug macro appropriately.

> 
> There is the scsi printks and maybe libfc or the fc class could export
> some fc specific ones.
> 
> 
> 
> > +     mp_req = (struct bnx2fc_mp_req *)&(io_req->mp_req);
> > +     memset(mp_req, 0, sizeof(struct bnx2fc_mp_req));
> > +
> > +     mp_req->req_len = sizeof(struct fcp_cmnd);
> > +     io_req->data_xfer_len = mp_req->req_len;
> > +     mp_req->req_buf = dma_alloc_coherent(&hba->pcidev->dev, 0x1000,
> > +                                     &mp_req->req_buf_dma,
> > +                                          GFP_ATOMIC);
> 
> 
> I think you can use GFP_NOIO in these allocations.

We cannot, as this function can be called in a locking context.

> 
> What is the 0x1000 value based off of? Is it hardware or is just due
> because it is the page size on many archs?

I'll  make it a PAGE_SIZE.

> 
> 
> 
> > +     if (!mp_req->req_buf) {
> > +             printk(KERN_ERR PFX "unable to alloc MP req buffer\n");
> > +             bnx2fc_free_mp_resc(io_req);
> > +             return FAILED;
> > +     }
> > +
> > +     mp_req->resp_buf = dma_alloc_coherent(&hba->pcidev->dev, 0x1000,
> > +                                     &mp_req->resp_buf_dma,
> > +                                           GFP_ATOMIC);
> > +     if (!mp_req->resp_buf) {
> > +             printk(KERN_ERR PFX "unable to alloc TM resp buffer\n");
> > +             bnx2fc_free_mp_resc(io_req);
> > +             return FAILED;
> > +     }
> > +     memset(mp_req->req_buf, 0, 0x1000);
> > +     memset(mp_req->resp_buf, 0, 0x1000);
> > +
> > +     /* Allocate and map mp_req_bd and mp_resp_bd */
> > +     sz = sizeof(struct fcoe_bd_ctx);
> > +     mp_req->mp_req_bd = dma_alloc_coherent(&hba->pcidev->dev, sz,
> > +                                             &mp_req->mp_req_bd_dma,
> > +                                              GFP_ATOMIC);
> > +     if (!mp_req->mp_req_bd) {
> > +             printk(KERN_ERR PFX "unable to alloc MP req bd\n");
> > +             bnx2fc_free_mp_resc(io_req);
> > +             return FAILED;
> > +     }
> > +     mp_req->mp_resp_bd = dma_alloc_coherent(&hba->pcidev->dev, sz,
> > +                                             &mp_req->mp_resp_bd_dma,
> > +                                              GFP_ATOMIC);
> > +     if (!mp_req->mp_req_bd) {
> > +             printk(KERN_ERR PFX "unable to alloc MP resp bd\n");
> > +             bnx2fc_free_mp_resc(io_req);
> > +             return FAILED;
> > +     }
> > +     /* Fill bd table */
> > +     addr = mp_req->req_buf_dma;
> > +     mp_req_bd = mp_req->mp_req_bd;
> > +     mp_req_bd->buf_addr_lo = (u32)addr&  0xffffffff;
> > +     mp_req_bd->buf_addr_hi = (u32)((u64)addr>>  32);
> > +     mp_req_bd->buf_len = 0x1000;
> > +     mp_req_bd->flags = 0;
> > +
> > +     /*
> > +      * MP buffer is either a task mgmt command or an ELS.
> > +      * So the assumption is that it consumes a single bd
> > +      * entry in the bd table
> > +      */
> > +     mp_resp_bd = mp_req->mp_resp_bd;
> > +     addr = mp_req->resp_buf_dma;
> > +     mp_resp_bd->buf_addr_lo = (u32)addr&  0xffffffff;
> > +     mp_resp_bd->buf_addr_hi = (u32)((u64)addr>>  32);
> > +     mp_resp_bd->buf_len = 0x1000;
> > +     mp_resp_bd->flags = 0;
> > +
> > +     return SUCCESS;
> > +}
> > +
> > +static int bnx2fc_initiate_tmf(struct scsi_cmnd *sc_cmd, u8 tm_flags)
> > +{
> > +     struct fc_lport *lport;
> > +     struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd->device));
> > +     struct fc_rport_libfc_priv *rp = rport->dd_data;
> > +     struct bnx2fc_port *port;
> > +     struct bnx2fc_hba *hba;
> > +     struct bnx2fc_rport *tgt;
> > +     struct bnx2fc_cmd *io_req;
> > +     struct bnx2fc_mp_req *tm_req;
> > +     struct fcoe_task_ctx_entry *task;
> > +     struct fcoe_task_ctx_entry *task_page;
> > +     struct Scsi_Host *host = sc_cmd->device->host;
> > +     struct fc_frame_header *fc_hdr;
> > +     struct fcp_cmnd *fcp_cmnd;
> > +     int task_idx, index;
> > +     int rc = SUCCESS;
> > +     u16 xid;
> > +     int rval;
> > +     u32 sid, did;
> > +     unsigned long start = jiffies;
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "initiate_tmf - tm_flags = %d\n", tm_flags);
> > +
> > +     lport = shost_priv(host);
> > +     port = lport_priv(lport);
> > +     hba = port->hba;
> > +
> > +     if (rport == NULL) {
> > +             printk(KERN_ALERT PFX "device_reset: rport is NULL\n");
> > +             rc = FAILED;
> > +             goto tmf_err;
> > +     }
> > +     rval = fc_remote_port_chkready(rport);
> > +     if (rval) {
> > +             printk(KERN_ALERT PFX "device_reset rport not ready\n");
> > +             rc = FAILED;
> > +             goto tmf_err;
> > +     }
> 
> 
> This has been replaced with the fc_block_scsi_eh calls now. Do not copy
> drivers when converting to it because some are broken. Be aware that its
> return value is 0 instead of something like SUCCESS.

OK.

> 
> 
> > +     if (lport->state != LPORT_ST_READY || !(lport->link_up)) {
> > +             printk(KERN_ERR PFX "device_reset: link is not ready\n");
> > +             rc = FAILED;
> > +             goto tmf_err;
> > +     }
> > +     /* rport and tgt are allocated together, so tgt should be non-NULL */
> > +     tgt = (struct bnx2fc_rport *)&rp[1];
> > +
> > +     if (!(test_bit(BNX2FC_FLAG_SESSION_READY,&tgt->flags))) {
> > +             printk(KERN_ERR PFX "device_reset: tgt not offloaded\n");
> > +             rc = FAILED;
> > +             goto tmf_err;
> > +     }
> > +retry_tmf:
> > +     io_req = bnx2fc_elstm_alloc(tgt, BNX2FC_TASK_MGMT_CMD);
> > +     if (!io_req) {
> > +             if (time_after(jiffies, start + HZ)) {
> > +                     printk(KERN_ERR PFX "tmf: Failed TMF");
> > +                     rc = FAILED;
> > +                     goto tmf_err;
> > +             }
> > +             msleep(20);
> > +             goto retry_tmf;
> > +     }
> > +     /* Initialize rest of io_req fields */
> > +     io_req->sc_cmd = sc_cmd;
> > +     io_req->port = port;
> > +     io_req->tgt = tgt;
> > +
> > +     tm_req = (struct bnx2fc_mp_req *)&(io_req->mp_req);
> > +
> > +     rc = bnx2fc_init_mp_req(io_req);
> > +     if (rc == FAILED) {
> > +             printk(KERN_ERR PFX "Task mgmt MP request init failed\n");
> > +             bnx2fc_cmd_release(io_req);
> > +             goto tmf_err;
> > +     }
> > +
> > +     /* Set TM flags */
> > +     io_req->io_req_flags = 0;
> > +     tm_req->tm_flags = tm_flags;
> > +
> > +     /* Fill FCP_CMND */
> > +     bnx2fc_build_fcp_cmnd(io_req, (struct fcp_cmnd *)tm_req->req_buf);
> > +     fcp_cmnd = (struct fcp_cmnd *)tm_req->req_buf;
> > +     memset(fcp_cmnd->fc_cdb, 0,  sc_cmd->cmd_len);
> > +     fcp_cmnd->fc_dl = 0;
> > +
> > +     /* Fill FC header */
> > +     fc_hdr =&(tm_req->req_fc_hdr);
> > +     sid = tgt->sid;
> > +     did = rport->port_id;
> > +     bnx2fc_fill_fc_hdr(fc_hdr, FC_RCTL_DD_UNSOL_CMD, sid, did,
> > +                        FC_TYPE_FCP, FC_FC_FIRST_SEQ | FC_FC_END_SEQ |
> > +                        FC_FC_SEQ_INIT, 0);
> > +     /* Obtain exchange id */
> > +     xid = bnx2fc_get_xid(io_req, hba);
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "TMF io_req xid = 0x%x\n", xid);
> > +     task_idx = xid/BNX2FC_TASKS_PER_PAGE;
> > +     index = xid % BNX2FC_TASKS_PER_PAGE;
> > +
> > +     /* Initialize task context for this IO request */
> > +     task_page = (struct fcoe_task_ctx_entry *) hba->task_ctx[task_idx];
> > +     task =&(task_page[index]);
> > +     bnx2fc_init_mp_task(io_req, task);
> > +
> > +     sc_cmd->SCp.ptr = (char *)io_req;
> > +
> > +     /* Obtain free SQ entry */
> > +     spin_lock_bh(&tgt->tgt_lock);
> > +     bnx2fc_add_2_sq(tgt, xid);
> > +
> > +     /* Enqueue the io_req to active_tm_queue */
> > +     io_req->on_tmf_queue = 1;
> > +     list_add_tail(&io_req->link,&tgt->active_tm_queue);
> > +
> > +     /* Ring doorbell */
> > +     bnx2fc_ring_doorbell(tgt);
> > +     spin_unlock_bh(&tgt->tgt_lock);
> > +
> > +     init_completion(&io_req->tm_done);
> > +     io_req->wait_for_comp = 1;
> 
> 
> I think you want to set that and init the completion before you send the
> IO right?

Yes, I'll change it.

> 
> 
> > +
> > +     rc = wait_for_completion_timeout(&io_req->tm_done,
> > +                                      BNX2FC_TM_TIMEOUT * HZ);
> > +     io_req->wait_for_comp = 0;
> > +
> > +     if (!(test_bit(BNX2FC_FLAG_TM_COMPL,&io_req->req_flags)))
> > +             set_bit(BNX2FC_FLAG_TM_TIMEOUT,&io_req->req_flags);
> > +     else
> > +             /* TM should have just completed */
> > +             return SUCCESS;
> 
> 
> I think you need to move where you set the BNX2FC_FLAG_TM_COMPL bit.
> bnx2fc_process_tm_compl set complete it in one context, we could wake up
> early due to timeout and see it here and return SUCCESS, scsi-ml's
> scsi_error.c code would think it owns the scsi_cmnd and start setting it
> up for a TUR, but then bnx2fc_process_tm_compl/bnx2fc_lun_reset_cmpl
> could be accessing the scsi_cmnd cleaning it up at the same time.

Yes. thanks for catching this. I'll take the tgt_lock before I check the
flags.

> 
> 
> 
> > +
> > +static int bnx2fc_abort_handler(struct bnx2fc_cmd *io_req)
> > +{
> > +     struct bnx2fc_rport *tgt = io_req->tgt;
> > +     int rc;
> > +
> > +     init_completion(&io_req->tm_done);
> > +     io_req->wait_for_comp = 1;
> 
> This seems racy too. I think you need to set that up before you send the
> abort, or the abort could complete and not see the wait_for_comp is set,
> and then we could set it here.

OK. I'll change this.

> 
> 
> > +     wait_for_completion(&io_req->tm_done);
> > +
> > +     spin_lock_bh(&tgt->tgt_lock);
> > +     io_req->wait_for_comp = 0;
> > +     if (!(test_and_set_bit(BNX2FC_FLAG_ABTS_DONE,
> > +                             &io_req->req_flags))) {
> > +             /* Let the scsi-ml try to recover this command */
> > +             printk(KERN_ERR PFX "abort failed, xid = 0x%x\n",
> > +                    io_req->xid);
> > +             rc = FAILED;
> > +     } else {
> > +             /*
> > +              * We come here even when there was a race condition
> > +              * between timeout and abts completion, and abts
> > +              * completion happens just in time.
> > +              */
> > +             bnx2fc_dbg(LOG_IOERR, "abort succeeded, xid = 0x%x\n",
> > +                        io_req->xid);
> > +             rc = SUCCESS;
> > +             bnx2fc_scsi_done(io_req, DID_ABORT);
> > +             bnx2fc_cmd_release(io_req);
> > +     }
> > +
> > +     /* release the reference taken in eh_abort */
> > +     bnx2fc_cmd_release(io_req);
> > +     spin_unlock_bh(&tgt->tgt_lock);
> > +     return rc;
> > +}
> > +
> > +/**
> > + * bnx2fc_eh_device_reset: Reset a single LUN
> > + * @sc_cmd:  SCSI command
> > + *
> > + * Set from SCSI host template to send task mgmt command to the target
> > + *   and wait for the response
> > + */
> > +int bnx2fc_eh_target_reset(struct scsi_cmnd *sc_cmd)
> > +{
> > +     int rc;
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "Entered bnx2fc_eh_target_reset\n");
> 
> Here you definately want better logging. Something like scmd_printk at
> least.

Yes. I'll print the host#.

> 
> 
> > +     rc = bnx2fc_initiate_tmf(sc_cmd, FCP_TMF_TGT_RESET);
> > +     return rc;
> 
> you can just do
> return bnx2fc_initiate_tmf(sc_cmd, FCP_TMF_TGT_RESET);
> and delete rc;

OK.

> 
> > +}
> > +
> > +/**
> > + * bnx2fc_eh_device_reset: Reset a single LUN
> > + * @sc_cmd:  SCSI command
> > + *
> > + * Set from SCSI host template to send task mgmt command to the target
> > + *   and wait for the response
> > + */
> > +int bnx2fc_eh_device_reset(struct scsi_cmnd *sc_cmd)
> > +{
> > +     int rc;
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "Entered bnx2fc_eh_device_reset\n");
> > +     /* bnx2fc_initiate_tmf is a blocking call */
> > +     rc = bnx2fc_initiate_tmf(sc_cmd, FCP_TMF_LUN_RESET);
> > +
> > +     return rc;
> 
> same as a bove.

OK.

> 
> 
> > +}
> > +
> > +/**
> > + * bnx2fc_eh_abort - eh_abort_handler api to abort an outstanding
> > + *                   SCSI command
> > + * @sc_cmd:  SCSI_ML command pointer
> > + *
> > + * SCSI abort request handler
> > + */
> > +int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd)
> > +{
> > +
> > +     struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd->device));
> > +     struct fc_rport_libfc_priv *rp = rport->dd_data;
> > +     struct bnx2fc_cmd *io_req;
> > +     struct fc_lport *lport;
> > +     struct bnx2fc_rport *tgt;
> > +     int rc = FAILED;
> > +
> > +     bnx2fc_dbg(LOG_IOERR, "Entered bnx2fc_eh_abort\n");
> > +
> > +     if (fc_remote_port_chkready(rport)) {
> > +             printk(KERN_ALERT PFX "bnx2fc_eh_abort: rport not ready\n");
> > +             return rc;
> > +     }
> 
> 
> Should be fc_block_scsi_eh.

OK.


> 
> 
> > +
> > +     lport = shost_priv(sc_cmd->device->host);
> > +     if ((lport->state != LPORT_ST_READY) || !(lport->link_up)) {
> > +             printk(KERN_ALERT PFX "eh_abort: link not ready\n");
> > +             return rc;
> > +     }
> > +
> > +     tgt = (struct bnx2fc_rport *)&rp[1];
> > +     spin_lock_bh(&tgt->tgt_lock);
> > +     io_req = (struct bnx2fc_cmd *)sc_cmd->SCp.ptr;
> > +     if (!io_req) {
> > +             /* Command might have just completed */
> > +             printk(KERN_ERR PFX "eh_abort: io_req is NULL\n");
> > +             spin_unlock_bh(&tgt->tgt_lock);
> > +             return SUCCESS;
> > +     }
> > +     printk(KERN_ERR PFX "eh_abort: xid = 0x%x refcnt = %d\n",
> > +              io_req->xid, io_req->cmd_refcnt.counter);
> > +
> > +     /* Hold IO request across abort processing */
> > +     bnx2fc_cmd_hold(io_req);
> > +
> > +     BUG_ON(tgt != io_req->tgt);
> > +
> > +     /* Remove the io_req from the active_q. */
> > +     /*
> > +      * Task Mgmt functions (LUN RESET&  TGT RESET) will not
> > +      * issue an ABTS on this particular IO req, as the
> > +      * io_req is no longer in the active_q.
> > +      */
> > +     if (tgt->flush_in_prog) {
> > +             printk(KERN_ALERT PFX "eh_abort: io_req (xid = 0x%x) "
> > +                     "flush in progress\n", io_req->xid);
> > +             bnx2fc_cmd_release(io_req);
> > +             spin_unlock_bh(&tgt->tgt_lock);
> > +             return SUCCESS;
> > +     }
> > +
> > +     if (io_req->on_active_queue == 0) {
> > +             printk(KERN_ALERT PFX "eh_abort: io_req (xid = 0x%x) "
> > +                             "not on active_q\n", io_req->xid);
> > +             /*
> > +              * This condition can happen only due to the FW bug,
> > +              * where we do not receive cleanup response from
> > +              * the FW. Handle this case gracefully by erroring
> > +              * back the IO request to SCSI-ml
> > +              */
> > +             bnx2fc_scsi_done(io_req, DID_ABORT);
> > +
> > +             bnx2fc_cmd_release(io_req);
> > +             spin_unlock_bh(&tgt->tgt_lock);
> > +             return SUCCESS;
> > +     }
> > +
> > +     /*
> > +      * Only eh_abort processing will remove the IO from
> > +      * active_cmd_q before processing the request. this is
> > +      * done to avoid race conditions between IOs aborted
> > +      * as part of task management completion and eh_abort
> > +      * processing
> > +      */
> > +     list_del_init(&io_req->link);
> > +     io_req->on_active_queue = 0;
> > +     /* Move IO req to retire queue */
> > +     list_add_tail(&io_req->link,&tgt->io_retire_queue);
> > +
> > +     if (!test_and_set_bit(BNX2FC_FLAG_ISSUE_ABTS,&io_req->req_flags)) {
> > +             /* Cancel the current timer running on this io_req */
> > +             if (cancel_delayed_work(&io_req->timeout_work))
> > +                     bnx2fc_cmd_release(io_req); /* drop timer hold */
> > +             set_bit(BNX2FC_FLAG_EH_ABORT,&io_req->req_flags);
> > +             rc = bnx2fc_initiate_abts(io_req);
> > +     } else {
> > +             printk(KERN_ALERT PFX "eh_abort: io_req (xid = 0x%x) "
> > +                             "already in abts processing\n", io_req->xid);
> > +             bnx2fc_cmd_release(io_req);
> > +             spin_unlock_bh(&tgt->tgt_lock);
> > +             return SUCCESS;
> > +     }
> > +     if (rc == FAILED) {
> > +             bnx2fc_cmd_release(io_req);
> > +             spin_unlock_bh(&tgt->tgt_lock);
> > +             return rc;
> > +     }
> > +     spin_unlock_bh(&tgt->tgt_lock);
> > +
> > +     rc = bnx2fc_abort_handler(io_req);
> > +     return rc;
> 
> 
> Just do
> 
> return bnx2fc_abort_handler(io_req);

OK.

> 
> > +}
> > +
> > +void bnx2fc_process_cleanup_compl(struct bnx2fc_cmd *io_req,
> > +                               struct fcoe_task_ctx_entry *task,
> > +                               u8 num_rq)
> > +{
> > +     bnx2fc_dbg(LOG_IOERR, "Entered process_cleanup_compl xid = 0x%x"
> > +                           "refcnt = %d, cmd_type = %d\n",
> > +                io_req->xid, io_req->cmd_refcnt.counter, io_req->cmd_type);
> > +     bnx2fc_scsi_done(io_req, DID_REQUEUE);
> 
> 
> I don't think you can use DID_REQUEUE here can you? In this path IO
> could have started, right? If so then DID_REQUEUE could end up retrying
> a partially run tape command.

What should be the 'result' here? should it be DID_TRANSPORT_DISRUPTED?

> 
> 
> > +     bnx2fc_cmd_release(io_req);
> > +     return;
> 
> 
> no return needed. There are others in the file.

OK.

> 
> 
> > +
> > +static void bnx2fc_lun_reset_cmpl(struct bnx2fc_cmd *io_req)
> > +{
> > +     struct scsi_cmnd *sc_cmd = io_req->sc_cmd;
> > +     struct bnx2fc_rport *tgt = io_req->tgt;
> > +     struct list_head *list;
> > +     struct list_head *tmp;
> > +     struct bnx2fc_cmd *cmd;
> > +     int tm_lun = sc_cmd->device->lun;
> > +     int rc = 0;
> > +     int lun;
> > +
> > +     /* called with tgt_lock held */
> > +     bnx2fc_dbg(LOG_IOERR, "Entered bnx2fc_lun_reset_cmpl\n");
> > +     /*
> > +      * Walk thru the active_ios queue and ABORT the IO
> > +      * that matches with the LUN that was reset
> > +      */
> > +     list_for_each_safe(list, tmp,&tgt->active_cmd_queue) {
> > +             bnx2fc_dbg(LOG_IOERR, "LUN RST cmpl: scan for pending IOs\n");
> > +             cmd = (struct bnx2fc_cmd *)list;
> > +             lun = cmd->sc_cmd->device->lun;
> > +             if (lun == tm_lun) {
> > +                     /* Initiate ABTS on this cmd */
> > +                     if (!test_and_set_bit(BNX2FC_FLAG_ISSUE_ABTS,
> > +                                     &cmd->req_flags)) {
> > +                             /* cancel the IO timeout */
> > +                             if (cancel_delayed_work(&io_req->timeout_work))
> > +                                     bnx2fc_cmd_release(io_req);
> > +                                                     /* timer hold */
> > +                             rc = bnx2fc_initiate_abts(cmd);
> > +                             /* abts shouldnt fail in this context */
> > +                             WARN_ON(rc != SUCCESS);
> > +                     } else
> > +                             printk(KERN_ERR PFX "lun_rst: abts already in"
> > +                                     " progress for this IO 0x%x\n",
> > +                                     cmd->xid);
> > +             }
> > +     }
> > +}
> > +
> > +static void bnx2fc_tgt_reset_cmpl(struct bnx2fc_cmd *io_req)
> > +{
> > +     struct bnx2fc_rport *tgt = io_req->tgt;
> > +     struct list_head *list;
> > +     struct list_head *tmp;
> > +     struct bnx2fc_cmd *cmd;
> > +     int rc = 0;
> > +
> > +     /* called with tgt_lock held */
> > +     bnx2fc_dbg(LOG_IOERR, "Entered bnx2fc_tgt_reset_cmpl\n");
> > +     /*
> > +      * Walk thru the active_ios queue and ABORT the IO
> > +      * that matches with the LUN that was reset
> > +      */
> > +     list_for_each_safe(list, tmp,&tgt->active_cmd_queue) {
> > +             bnx2fc_dbg(LOG_IOERR, "TGT RST cmpl: scan for pending IOs\n");
> > +             cmd = (struct bnx2fc_cmd *)list;
> > +             /* Initiate ABTS */
> > +             if (!test_and_set_bit(BNX2FC_FLAG_ISSUE_ABTS,
> > +                                                     &cmd->req_flags)) {
> > +                     /* cancel the IO timeout */
> > +                     if (cancel_delayed_work(&io_req->timeout_work))
> > +                             bnx2fc_cmd_release(io_req); /* timer hold */
> > +                     rc = bnx2fc_initiate_abts(cmd);
> > +                     /* abts shouldnt fail in this context */
> > +                     WARN_ON(rc != SUCCESS);
> > +
> > +             } else
> > +                     printk(KERN_ERR PFX "tgt_rst: abts already in progress"
> > +                             " for this IO 0x%x\n", cmd->xid);
> > +     }
> > +}
> 
> 
> Are you sending a abort when a lun or target reset has been successfully
> completed? Does your hw need the reset? SCSI spec wise the lun or target
> reset will take care of the scsi commands on its side, so there is no
> need to send an abort.

It is better to send ABTS on all the outstanding IOs after issuing
lun/target reset.  targets may take care of scsi commands on its side.
But the commands on the flight may have to be aborted. This is as per
the FCP-4 spec in "Table 8 - Clearing effects of initiator FCP-Port
actions" :-
"Exchanges are cleared internally within the target FCP_Port, but open
FCP Sequences shall be individually aborted by the initiator FCP_Port
using ABTS-LS that also has the effect of aborting the associated FCP
Exchange. See 12.3."


> 
> Does the fcoe card have the similar cleanup command as bnx2i and do you
> just need to run that?

Yes. But we need to send an ABTS here instead of cleanup.

> 
> 
> 
> 
> > +}
> > +
> > +static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
> > +{
> > +     struct bnx2fc_hba *hba = io_req->port->hba;
> > +     struct scsi_cmnd *sc = io_req->sc_cmd;
> > +     struct fcoe_bd_ctx *bd = io_req->bd_tbl->bd_tbl;
> > +     struct scatterlist *sg;
> > +     int byte_count = 0;
> > +     int sg_count = 0;
> > +     int bd_count = 0;
> > +     int sg_frags;
> > +     unsigned int sg_len;
> > +     u64 addr;
> > +     int i;
> > +
> > +     sg = scsi_sglist(sc);
> > +     sg_count = pci_map_sg(hba->pcidev, sg, scsi_sg_count(sc),
> > +                           sc->sc_data_direction);
> 
> 
> I think you could also use scsi_dma_map instead.
> 
> And if not I think we are supposed to be using the dma functions instead
> of the pci ones.

OK. I can use dma_map_sg() intead. But this function inturn calls
pci_map_sg(). Is there any reason why we cannot directly call it?

> 
> 
> 
> > +     for (i = 0; i<  sg_count; i++) {
> 
> You need to use the sg list accessors for_each_sg() for example.
> 
> 
> > +
> > +             sg_len = sg_dma_len(sg);
> > +             addr = sg_dma_address(sg);
> > +             if (sg_len>  BNX2FC_MAX_BD_LEN) {
> > +                     sg_frags = bnx2fc_split_bd(io_req, addr, sg_len,
> > +                                                bd_count);
> > +             } else {
> > +
> > +                     sg_frags = 1;
> > +                     bd[bd_count].buf_addr_lo = addr&  0xffffffff;
> > +                     bd[bd_count].buf_addr_hi  = addr>>  32;
> > +                     bd[bd_count].buf_len = (u16)sg_len;
> > +                     bd[bd_count].flags = 0;
> > +             }
> > +             bd_count += sg_frags;
> > +             byte_count += sg_len;
> > +             sg++;
> > +     }
> > +     if (byte_count != scsi_bufflen(sc))
> > +             printk(KERN_ERR PFX "byte_count = %d != scsi_bufflen = %d, "
> > +                     "task_id = 0x%x\n", byte_count, scsi_bufflen(sc),
> > +                     io_req->xid);
> > +     return bd_count;
> > +}
> > +
> 
> 
> > +
> > +static void bnx2fc_unmap_sg_list(struct bnx2fc_cmd *io_req)
> > +{
> > +     struct bnx2fc_hba *hba = io_req->port->hba;
> > +     struct scsi_cmnd *sc_cmd = io_req->sc_cmd;
> > +     struct scatterlist *sg;
> > +
> > +     if (io_req->bd_tbl->bd_valid&&  sc_cmd) {
> > +             if (scsi_sg_count(sc_cmd)) {
> > +                     sg = scsi_sglist(sc_cmd);
> > +                     pci_unmap_sg(hba->pcidev, sg, scsi_sg_count(sc_cmd),
> > +                                  sc_cmd->sc_data_direction);
> 
> 
> scsi_dma_unmap.
> 
> 
> 
> > +static void bnx2fc_parse_fcp_rsp(struct bnx2fc_cmd *io_req,
> > +                              struct fcoe_fcp_rsp_payload *fcp_rsp,
> > +                              u8 num_rq)
> > +{
> > +     struct scsi_cmnd *sc_cmd = io_req->sc_cmd;
> > +     struct bnx2fc_rport *tgt = io_req->tgt;
> > +     u8 rsp_flags = fcp_rsp->fcp_flags.flags;
> > +     u32 rq_buff_len = 0;
> > +     int i;
> > +     unsigned char *rq_data;
> > +     unsigned char *dummy;
> > +     int fcp_sns_len = 0;
> > +     int fcp_rsp_len = 0;
> > +
> > +     io_req->fcp_status = FC_GOOD;
> > +     io_req->fcp_resid = fcp_rsp->fcp_resid;
> > +
> > +     io_req->scsi_comp_flags = rsp_flags;
> > +     if (sc_cmd == NULL)
> > +             printk(KERN_ERR PFX "ERROR!! sc_cmd NULL\n");
> 
> 
> Does this ever happen? If so then handle it properly because you
> probably will not even see the printk and instead see a oops trample
> over it or a hung box when you access the cmd below. If it does not
> every happen then remove the goofy printk.

I'll remove this printk. This was done for debug purpose during initial
bring up, and somehow missed removing it.


> 
> 
> 
> > +
> > +/**
> > + * bnx2fc_queuecommand - Queuecommand function of the scsi template
> > + * @sc_cmd:  struct scsi_cmnd to be executed
> > + * @done:    Callback function to be called when sc_cmd is complted
> > + *
> > + * This is the IO strategy routine, called by SCSI-ML
> > + **/
> > +int bnx2fc_queuecommand(struct scsi_cmnd *sc_cmd,
> > +                             void (*done)(struct scsi_cmnd *))
> > +{
> > +     struct fc_lport *lport;
> > +     struct fc_rport *rport = starget_to_rport(scsi_target(sc_cmd->device));
> > +     struct fc_rport_libfc_priv *rp = rport->dd_data;
> > +     struct bnx2fc_port *port;
> > +     struct bnx2fc_hba *hba;
> > +     struct bnx2fc_rport *tgt;
> > +     struct Scsi_Host *host = sc_cmd->device->host;
> > +     struct bnx2fc_cmd *io_req;
> > +     int rc = 0;
> > +     int rval;
> > +     struct cnic_dev *dev;
> > +
> > +     lport = shost_priv(host);
> > +     spin_unlock_irq(host->host_lock);
> 
> Don't forget to update to the new locking when you resend.

Yes. I've seen the new changes. Will incorporate them.


> 
> 
> > +     sc_cmd->scsi_done = done;
> > +     port = lport_priv(lport);
> > +     hba = port->hba;
> > +     dev = hba->cnic;
> > +
> > +     rval = fc_remote_port_chkready(rport);
> > +     if (rval) {
> > +             sc_cmd->result = rval;
> > +             done(sc_cmd);
> > +             goto exit_qcmd;
> > +     }
> > +
> > +     if ((lport->state != LPORT_ST_READY) || !(lport->link_up)) {
> > +             rc = SCSI_MLQUEUE_HOST_BUSY;
> > +             goto exit_qcmd;
> > +     }
> > +
> > +     /* rport and tgt are allocated together, so tgt should be non-NULL */
> > +     tgt = (struct bnx2fc_rport *)&rp[1];
> > +
> > +     if (!test_bit(BNX2FC_FLAG_SESSION_READY,&tgt->flags)) {
> > +             /*
> > +              * Session is not offloaded yet. Let SCSI-ml retry
> > +              * the command.
> > +              */
> > +             rc = SCSI_MLQUEUE_HOST_BUSY;
> 
> You should use SCSI_MLQUEUE_TARGET_BUSY, so only the one target is
> blocked and not the whole host.

OK.


> 
> 
> 
> > +
> > +static int bnx2fc_post_io_req(struct bnx2fc_rport *tgt,
> > +                            struct bnx2fc_cmd *io_req)
> > +{
> 
> 
> > +
> > +     /* Time IO req */
> > +     bnx2fc_cmd_timer_set(io_req, BNX2FC_IO_TIMEOUT);
> 
> Hard coding this does not seem right becuase the scsi cmnd timer can be
> set through sysfs. It could be set lower than this which makes it
> useless. I think libfc is dropping their similar timer like this and
> just relying on the scsi_cmnd timer now (I think they just do the rec
> but not the abort now).

if it is a lower value, eh_abort_handler() code kicks in. Default scsi
cmd timer is 60 secs, which may be too long to wait to sends an ABTS.

> 
> 
> 
> 
> > +     /* Obtain free SQ entry */
> > +     bnx2fc_add_2_sq(tgt, xid);
> > +
> > +     /* Enqueue the io_req to active_cmd_queue */
> > +
> > +     io_req->on_active_queue = 1;
> > +     /* move io_req from pending_queue to active_queue */
> > +     list_add_tail(&io_req->link,&tgt->active_cmd_queue);
> > +
> > +     /* Ring doorbell */
> > +     bnx2fc_ring_doorbell(tgt);
> > +     spin_unlock_bh(&tgt->tgt_lock);
> > +     return 0;
> > +}
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_tgt.c b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
> > new file mode 100644
> > index 0000000..6739dce
> > --- /dev/null
> > +++ b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
> > @@ -0,0 +1,875 @@
> 
> 
> I have to do some other stuff now. Will continue from here Monday or Sunday.
> 






More information about the devel mailing list