On Mon, 2011-01-17 at 18:44 -0800, Mike Christie wrote:
> On 12/24/2010 12:02 AM, Bhanu Gollapudi wrote:
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_tgt.c b/drivers/scsi/bnx2fc/bnx2fc_tgt.c
>
> > +
> > +static void bnx2fc_offload_session(struct bnx2fc_port *port,
> > + struct bnx2fc_rport *tgt,
> > + struct fc_rport_priv *rdata)
> > +{
> > + struct fc_lport *lport = rdata->local_port;
> > + struct fc_rport *rport = rdata->rport;
> > + struct bnx2fc_hba *hba = port->hba;
> > + int rval;
> > + int i = 0;
> > +
> > + /* Initialize bnx2fc_rport */
> > + /* NOTE: tgt is already bzero'd */
> > + rval = bnx2fc_init_tgt(tgt, port, rdata);
> > + if (rval) {
> > + printk(KERN_ERR PFX "Failed to allocate conn id for "
> > + "port_id (%6x)\n", rport->port_id);
> > + goto ofld_err;
> > + }
> > +
> > + if (hba->num_ofld_sess>= BNX2FC_NUM_MAX_SESS) {
> > + printk(KERN_ERR PFX "exceeded max sessions port_id (%6x)\n",
> > + rport->port_id);
> > + goto ofld_err;
> > + }
> > +
> > + /* Allocate session resources */
> > + rval = bnx2fc_alloc_session_resc(hba, tgt);
> > + if (rval) {
> > + printk(KERN_ERR PFX "Failed to allocate resources\n");
> > + goto ofld_err;
> > + }
> > +
> > + /*
> > + * Initialize FCoE session offload process.
> > + * Upon completion of offload process add
> > + * rport to list of rports
> > + */
> > +retry_ofld:
> > + clear_bit(BNX2FC_FLAG_OFLD_REQ_CMPL,&tgt->flags);
> > + rval = bnx2fc_send_session_ofld_req(port, tgt);
> > + if (rval) {
> > + printk(KERN_ERR PFX "ofld_req failed\n");
> > + goto ofld_err;
> > + }
> > +
> > + /*
> > + * wait for the session is offloaded and enabled. 20 Secs
> > + * should be ample time for this process to complete.
> > + */
> > + init_timer(&tgt->ofld_timer);
> > + tgt->ofld_timer.expires = (20 * HZ) + jiffies;
> > + tgt->ofld_timer.function = bnx2fc_ofld_timer;
> > + tgt->ofld_timer.data = (unsigned long)tgt;
>
>
> setup_timer()
OK. will use. Is there any advantage of using setup_timer()? This is not
a perf path.
>
> > +
> > + add_timer(&tgt->ofld_timer);
> > +
> > + wait_event_interruptible(tgt->ofld_wait,
> > + (test_bit(
> > + BNX2FC_FLAG_OFLD_REQ_CMPL,
> > + &tgt->flags)));
>
>
>
> This gets run from libfc's work queue, right? It does not seem nice to
> sleep here for so long and block other drivers.
> For multipath setups having to wait this longs would be very bad.
20 secs sounds long. But this is a local completion with the FW which
hardly takes any time.
>
>
> > +
> > +void bnx2fc_flush_active_ios(struct bnx2fc_rport *tgt)
> > +{
> > + struct bnx2fc_cmd *io_req;
> > + struct list_head *list;
> > + struct list_head *tmp;
> > + int rc;
> > + int i = 0;
> > + bnx2fc_dbg(LOG_IOERR, "Entered flush_active_ios - %d, port_id = 0x%x\n",
> > + tgt->num_active_ios, tgt->rdata->ids.port_id);
> > +
> > + spin_lock_bh(&tgt->tgt_lock);
> > + tgt->flush_in_prog = 1;
> > +
> > + list_for_each_safe(list, tmp,&tgt->active_cmd_queue) {
> > + i++;
> > + io_req = (struct bnx2fc_cmd *)list;
> > + list_del_init(&io_req->link);
> > + io_req->on_active_queue = 0;
> > + bnx2fc_dbg(LOG_IOERR, "cmd_queue cleanup - xid = 0x%x ref_cnt ="
> > + " %d\n", io_req->xid, io_req->cmd_refcnt.counter);
> > +
> > + if (cancel_delayed_work(&io_req->timeout_work)) {
> > + if (test_and_clear_bit(BNX2FC_FLAG_EH_ABORT,
> > + &io_req->req_flags)) {
> > + /* Handle eh_abort timeout */
> > + bnx2fc_dbg(LOG_IOERR, "eh_abort for IO "
> > + "with oxid = 0x%x "
> > + "cleaned up\n", io_req->xid);
> > + complete(&io_req->tm_done);
> > + }
> > + bnx2fc_cmd_release(io_req); /* drop timer hold */
> > + }
> > +
> > + set_bit(BNX2FC_FLAG_IO_COMPL,&io_req->req_flags);
> > + set_bit(BNX2FC_FLAG_IO_CLEANUP,&io_req->req_flags);
> > + rc = bnx2fc_initiate_cleanup(io_req);
> > + BUG_ON(rc);
> > + }
> > +
> > + list_for_each_safe(list, tmp,&tgt->els_queue) {
> > + i++;
> > + io_req = (struct bnx2fc_cmd *)list;
> > + list_del_init(&io_req->link);
> > + io_req->on_active_queue = 0;
> > +
> > + bnx2fc_dbg(LOG_IOERR, "els_queue cleanup - xid = 0x%x"
> > + " ref_cnt = %d\n", io_req->xid,
> > + io_req->cmd_refcnt.counter);
> > +
> > + if (cancel_delayed_work(&io_req->timeout_work))
> > + bnx2fc_cmd_release(io_req); /* drop timer hold */
> > +
> > + if ((io_req->cb_func)&& (io_req->cb_arg)) {
> > + io_req->cb_func(io_req->cb_arg);
> > + io_req->cb_arg = NULL;
> > + }
> > +
> > + rc = bnx2fc_initiate_cleanup(io_req);
> > + BUG_ON(rc);
> > + }
> > +
> > + list_for_each_safe(list, tmp,&tgt->io_retire_queue) {
> > + i++;
> > + io_req = (struct bnx2fc_cmd *)list;
> > + list_del_init(&io_req->link);
> > +
> > + bnx2fc_dbg(LOG_IOERR, "retire_queue flush - xid = 0x%x"
> > + " ref_cnt = %d\n", io_req->xid,
> > + io_req->cmd_refcnt.counter);
> > +
> > + if (cancel_delayed_work(&io_req->timeout_work))
> > + bnx2fc_cmd_release(io_req);
> > +
> > + clear_bit(BNX2FC_FLAG_ISSUE_RRQ,&io_req->req_flags);
> > + }
> > +
> > + bnx2fc_dbg(LOG_IOERR, "IOs flushed = %d\n", i);
> > + i = 0;
> > + spin_unlock_bh(&tgt->tgt_lock);
> > + /* wait for active_ios to go to 0 */
> > + while ((tgt->num_active_ios != 0)&& (i++< 120))
> > + msleep(25);
>
>
> Are you waiting for bnx2fc_cmd_timeout to stop running (for the case
> where the cancel did not indicate it was pending) or to get a response
> to when you did bnx2fc_initiate_cleanup?
Waiting to get the response for bnx2fc_initiate_cleanup().
>
> For bnx2fc_cmd_timeout you should just flush the workqueue_struct.
>
> For bnx2fc_initiate_cleanup, where did the 120 * 25msecs limit come from?
This is again a local completion from FW to get the cleanup completion
for the outstanding IOs, and should not take longer. I'll probably use a
symbol instead of hard coded 120.
>
>
>
> > + if (tgt->num_active_ios != 0)
> > + printk(KERN_ERR PFX "CLEANUP on port 0x%x:"
> > + " active_ios = %d\n",
> > + tgt->rdata->ids.port_id, tgt->num_active_ios);
> > + spin_lock_bh(&tgt->tgt_lock);
> > + tgt->flush_in_prog = 0;
> > + spin_unlock_bh(&tgt->tgt_lock);
> > +}
> > +
> > +static void bnx2fc_upload_session(struct bnx2fc_port *port,
> > + struct bnx2fc_rport *tgt)
> > +{
> > + struct bnx2fc_hba *hba = port->hba;
> > +
> > + bnx2fc_dbg(LOG_SESS, "upload_session: active_ios = %d\n",
> > + tgt->num_active_ios);
> > +
> > + /*
> > + * Called with hba->hba_mutex held.
> > + * This is a blocking call
> > + */
> > + clear_bit(BNX2FC_FLAG_UPLD_REQ_COMPL,&tgt->flags);
> > + bnx2fc_send_session_disable_req(port, tgt);
> > +
> > + /*
> > + * wait for upload to complete. 20 Secs
> > + * should be sufficient time for this process to complete.
> > + */
> > + init_timer(&tgt->upld_timer);
> > + tgt->upld_timer.expires = (20 * HZ) + jiffies;
> > + tgt->upld_timer.function = bnx2fc_upld_timer;
> > + tgt->upld_timer.data = (unsigned long)tgt;
> > +
>
>
> setup_timer() Check for other places in the code for this.
Yes. will do.
>
>
> > + add_timer(&tgt->upld_timer);
> > +
> > + bnx2fc_dbg(LOG_SESS, "waiting for disable compl\n");
> > + wait_event_interruptible(tgt->upld_wait,
> > + (test_bit(
> > + BNX2FC_FLAG_UPLD_REQ_COMPL,
> > + &tgt->flags)));
> > +
> > + if (signal_pending(current))
> > + flush_signals(current);
> > +
> > + del_timer_sync(&tgt->upld_timer);
> > +
> > + bnx2fc_dbg(LOG_SESS, "disable wait complete flags = 0x%lx\n",
> > + tgt->flags);
> > +
> > + /*
> > + * traverse thru the active_q and tmf_q and cleanup
> > + * IOs in these lists
> > + */
> > + bnx2fc_dbg(LOG_SESS, "flush/upload\n");
> > + bnx2fc_flush_active_ios(tgt);
> > +
> > + /* Issue destroy KWQE */
> > + if (test_bit(BNX2FC_FLAG_DISABLED,&tgt->flags)) {
> > + bnx2fc_dbg(LOG_SESS, "send destroy req\n");
> > + clear_bit(BNX2FC_FLAG_UPLD_REQ_COMPL,&tgt->flags);
> > + bnx2fc_send_session_destroy_req(hba, tgt);
> > +
> > + /* wait for destroy to complete */
> > + init_timer(&tgt->upld_timer);
> > + tgt->upld_timer.expires = (20 * HZ) + jiffies;
> > + tgt->upld_timer.function = bnx2fc_upld_timer;
> > + tgt->upld_timer.data = (unsigned long)tgt;
> > +
> > + add_timer(&tgt->upld_timer);
> > +
> > + wait_event_interruptible(tgt->upld_wait,
> > + (test_bit(
> > + BNX2FC_FLAG_UPLD_REQ_COMPL,
> > + &tgt->flags)));
> > +
> > + if (!(test_bit(BNX2FC_FLAG_DESTROYED,&tgt->flags)))
> > + printk(KERN_ERR PFX "ERROR!! destroy timed out\n");
> > +
> > + bnx2fc_dbg(LOG_SESS, "destroy wait complete flags = 0x%lx\n",
> > + tgt->flags);
> > + if (signal_pending(current))
> > + flush_signals(current);
> > +
> > + del_timer_sync(&tgt->upld_timer);
> > +
> > + } else {
> > + printk(KERN_ERR PFX "ERROR!! DISABLE req timed out, destroy"
> > + " not sent to FW\n");
> > + }
>
>
> No need for extra brackets.
OK.
>
> > +
> > +
> > + /* Free session resources */
> > + spin_lock_bh(&tgt->cq_lock);
> > + bnx2fc_free_session_resc(hba, tgt);
> > + bnx2fc_free_conn_id(hba, tgt->fcoe_conn_id);
> > + spin_unlock_bh(&tgt->cq_lock);
> > + return;
>
> extra return
OK.
>
>
> > +/**
> > + * bnx2fc_tgt_lookup() - Lookup a bnx2fc_rport by port_id
> > + * @port: bnx2fc_port struct to lookup the target port on
> > + * @port_id: The remote port ID to look up
> > + */
> > +struct bnx2fc_rport *bnx2fc_tgt_lookup(struct bnx2fc_port *port,
> > + u32 port_id)
> > +{
> > + struct bnx2fc_hba *hba = port->hba;
> > + struct bnx2fc_rport *tgt;
> > + struct fc_rport_priv *rdata;
> > + int i;
> > +
> > + for (i = 0; i< BNX2FC_NUM_MAX_SESS; i++) {
> > + tgt = hba->tgt_ofld_list[i];
> > + if ((tgt)&& (tgt->port == port)) {
> > + rdata = tgt->rdata;
> > + if (rdata->ids.port_id == port_id) {
> > + if (rdata->rp_state != RPORT_ST_DELETE) {
> > + bnx2fc_dbg(LOG_IOERR, "rport 0x%x "
> > + "obtained\n",
> > + rdata->ids.port_id);
> > + return tgt;
> > + } else {
> > + printk(KERN_ERR PFX "rport 0x%x "
> > + "is in DELETED state\n",
> > + rdata->ids.port_id);
> > + return NULL;
> > + }
> > + }
> > + }
> > + }
> > + return NULL;
> > +}
>
>
> It seems this should go in scsi_transport_fc. I thought there was a
> similar lookup function in libfc too, and we were trying to move that to
> the transport class.
this is bnx2fc driver specific lookup, looking if the target is
offloaded or not.
>
>
>
>
> > +
> > +
> > +/**
> > + * bnx2fc_alloc_conn_id - allocates FCOE Connection id
> > + *
> > + * @hba: pointer to adapter structure
> > + * @tgt: pointer to bnx2fc_rport structure
> > + */
> > +static u32 bnx2fc_alloc_conn_id(struct bnx2fc_hba *hba,
> > + struct bnx2fc_rport *tgt)
> > +{
> > + u32 conn_id, next;
> > +
> > + /* called with hba mutex held */
> > +
> > + /*
> > + * tgt_ofld_list access is synchronized using
> > + * both hba mutex and hba lock. Atleast hba mutex or
> > + * hba lock needs to be held for read access.
> > + */
> > +
> > + spin_lock_bh(&hba->hba_lock);
> > + next = hba->next_conn_id;
> > + conn_id = hba->next_conn_id++;
> > + if (hba->next_conn_id == BNX2FC_NUM_MAX_SESS)
> > + hba->next_conn_id = 0;
> > +
> > + while (hba->tgt_ofld_list[conn_id] != NULL) {
> > + conn_id++;
> > + if (conn_id == BNX2FC_NUM_MAX_SESS)
> > + conn_id = 0;
> > +
> > + if (conn_id == next) {
> > + /* No free conn_ids are available */
> > + conn_id = -1;
> > + return conn_id;
>
> Just do return -1;
OK.
>
>
>
>
> > +static int bnx2fc_alloc_session_resc(struct bnx2fc_hba *hba,
> > + struct bnx2fc_rport *tgt)
> > +{
> > + dma_addr_t page;
> > + int num_pages;
> > + u32 *pbl;
> > +
> > + /* Allocate and map SQ */
> > + tgt->sq_mem_size = tgt->max_sqes * BNX2FC_SQ_WQE_SIZE;
> > + tgt->sq_mem_size = (tgt->sq_mem_size + (PAGE_SIZE - 1))& PAGE_MASK;
> > +
> > + tgt->sq = dma_alloc_coherent(&hba->pcidev->dev, tgt->sq_mem_size,
> > + &tgt->sq_dma, GFP_KERNEL);
> > + if (!tgt->sq) {
> > + printk(KERN_ALERT PFX "unable to allocate SQ memory %d\n",
> > + tgt->sq_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->sq, 0, tgt->sq_mem_size);
> > +
> > + /* Allocate and map CQ */
> > + tgt->cq_mem_size = tgt->max_cqes * BNX2FC_CQ_WQE_SIZE;
> > + tgt->cq_mem_size = (tgt->cq_mem_size + (PAGE_SIZE - 1))& PAGE_MASK;
> > +
> > + tgt->cq = dma_alloc_coherent(&hba->pcidev->dev, tgt->cq_mem_size,
> > + &tgt->cq_dma, GFP_KERNEL);
> > + if (!tgt->cq) {
> > + printk(KERN_ALERT PFX "unable to allocate CQ memory %d\n",
> > + tgt->cq_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->cq, 0, tgt->cq_mem_size);
> > +
> > + /* Allocate and map RQ and RQ PBL */
> > + tgt->rq_mem_size = tgt->max_rqes * BNX2FC_RQ_WQE_SIZE;
> > + tgt->rq_mem_size = (tgt->rq_mem_size + (PAGE_SIZE - 1))& PAGE_MASK;
> > +
> > + tgt->rq = dma_alloc_coherent(&hba->pcidev->dev, tgt->rq_mem_size,
> > + &tgt->rq_dma, GFP_KERNEL);
> > + if (!tgt->rq) {
> > + printk(KERN_ALERT PFX "unable to allocate RQ memory %d\n",
> > + tgt->rq_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->rq, 0, tgt->rq_mem_size);
> > +
> > + tgt->rq_pbl_size = (tgt->rq_mem_size / PAGE_SIZE) * sizeof(void *);
> > + tgt->rq_pbl_size = (tgt->rq_pbl_size + (PAGE_SIZE - 1))& PAGE_MASK;
> > +
> > + tgt->rq_pbl = dma_alloc_coherent(&hba->pcidev->dev, tgt->rq_pbl_size,
> > + &tgt->rq_pbl_dma, GFP_KERNEL);
> > + if (!tgt->rq_pbl) {
> > + printk(KERN_ALERT PFX "unable to allocate RQ PBL %d\n",
> > + tgt->rq_pbl_size);
> > + goto mem_alloc_failure;
> > + }
> > +
> > + memset(tgt->rq_pbl, 0, tgt->rq_pbl_size);
> > + num_pages = tgt->rq_mem_size / PAGE_SIZE;
> > + page = tgt->rq_dma;
> > + pbl = (u32 *)tgt->rq_pbl;
> > +
>
>
> What is the code below supposed to be doing?
It is just filling the PBL (aka, buffer descriptor list) for RQ.
>
>
> > + while (num_pages--) {
> > + *pbl = (u32)page;
> > + pbl++;
> > + *pbl = (u32)((u64)page>> 32);
> > + pbl++;
> > + page += PAGE_SIZE;
> > + }
> > +
> > + /* Allocate and map XFERQ */
> > + tgt->xferq_mem_size = tgt->max_sqes * BNX2FC_XFERQ_WQE_SIZE;
> > + tgt->xferq_mem_size = (tgt->xferq_mem_size + (PAGE_SIZE - 1))&
> > + PAGE_MASK;
> > +
> > + tgt->xferq = dma_alloc_coherent(&hba->pcidev->dev, tgt->xferq_mem_size,
> > + &tgt->xferq_dma, GFP_KERNEL);
> > + if (!tgt->xferq) {
> > + printk(KERN_ALERT PFX "unable to allocate XFERQ %d\n",
> > + tgt->xferq_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->xferq, 0, tgt->xferq_mem_size);
> > +
> > + /* Allocate and map CONFQ& CONFQ PBL */
> > + tgt->confq_mem_size = tgt->max_sqes * BNX2FC_CONFQ_WQE_SIZE;
> > + tgt->confq_mem_size = (tgt->confq_mem_size + (PAGE_SIZE - 1))&
> > + PAGE_MASK;
> > +
> > + tgt->confq = dma_alloc_coherent(&hba->pcidev->dev, tgt->confq_mem_size,
> > + &tgt->confq_dma, GFP_KERNEL);
> > + if (!tgt->confq) {
> > + printk(KERN_ALERT PFX "unable to allocate CONFQ %d\n",
> > + tgt->confq_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->confq, 0, tgt->confq_mem_size);
> > +
> > + tgt->confq_pbl_size =
> > + (tgt->confq_mem_size / PAGE_SIZE) * sizeof(void *);
> > + tgt->confq_pbl_size =
> > + (tgt->confq_pbl_size + (PAGE_SIZE - 1))& PAGE_MASK;
> > +
> > + tgt->confq_pbl = dma_alloc_coherent(&hba->pcidev->dev,
> > + tgt->confq_pbl_size,
> > + &tgt->confq_pbl_dma, GFP_KERNEL);
> > + if (!tgt->confq_pbl) {
> > + printk(KERN_ALERT PFX "unable to allocate CONFQ PBL %d\n",
> > + tgt->confq_pbl_size);
> > + goto mem_alloc_failure;
> > + }
> > +
> > + memset(tgt->confq_pbl, 0, tgt->confq_pbl_size);
> > + num_pages = tgt->confq_mem_size / PAGE_SIZE;
> > + page = tgt->confq_dma;
> > + pbl = (u32 *)tgt->confq_pbl;
> > +
> > + while (num_pages--) {
> > + *pbl = (u32)page;
> > + pbl++;
> > + *pbl = (u32)((u64)page>> 32);
> > + pbl++;
> > + page += PAGE_SIZE;
> > + }
> > +
> > + /* Allocate and map ConnDB */
> > + tgt->conn_db_mem_size = sizeof(struct fcoe_conn_db);
> > +
> > + tgt->conn_db = dma_alloc_coherent(&hba->pcidev->dev,
> > + tgt->conn_db_mem_size,
> > + &tgt->conn_db_dma, GFP_KERNEL);
> > + if (!tgt->conn_db) {
> > + printk(KERN_ALERT PFX "unable to allocate conn_db %d\n",
> > + tgt->conn_db_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->conn_db, 0, tgt->conn_db_mem_size);
> > +
> > +
> > + /* Allocate and map LCQ */
> > + tgt->lcq_mem_size = (tgt->max_sqes + 8) * BNX2FC_SQ_WQE_SIZE;
> > + tgt->lcq_mem_size = (tgt->lcq_mem_size + (PAGE_SIZE - 1))&
> > + PAGE_MASK;
> > +
> > + tgt->lcq = dma_alloc_coherent(&hba->pcidev->dev, tgt->lcq_mem_size,
> > + &tgt->lcq_dma, GFP_KERNEL);
> > +
> > + if (!tgt->lcq) {
> > + printk(KERN_ALERT PFX "unable to allocate lcq %d\n",
> > + tgt->lcq_mem_size);
> > + goto mem_alloc_failure;
> > + }
> > + memset(tgt->lcq, 0, tgt->lcq_mem_size);
> > +
> > + /* Arm CQ */
> > + tgt->conn_db->cq_arm.lo = -1;
> > + tgt->conn_db->rq_prod = 0x8000;
> > +
> > + return 0;
> > +
> > +mem_alloc_failure:
> > + bnx2fc_free_session_resc(hba, tgt);
> > + bnx2fc_free_conn_id(hba, tgt->fcoe_conn_id);
> > + return -ENOMEM;
> > +}
> > +
>
Thanks again for your valuable comments, Mike.
Regards,
Bhanu
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
