From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Christie Subject: Re: [v5 PATCH 2/4] bnx2fc: Firmware interface and ELS handling Date: Wed, 02 Feb 2011 02:35:16 -0600 Message-ID: <4D491744.2020304@cs.wisc.edu> References: <1296266428.268.51.camel@LTLNR-SJCE10.corp.ad.broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from sabe.cs.wisc.edu ([128.105.6.20]:53424 "EHLO sabe.cs.wisc.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753758Ab1BBIfs (ORCPT ); Wed, 2 Feb 2011 03:35:48 -0500 In-Reply-To: <1296266428.268.51.camel@LTLNR-SJCE10.corp.ad.broadcom.com> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Bhanu Gollapudi Cc: James.Bottomley@suse.de, linux-scsi@vger.kernel.org, devel@open-fcoe.org, mchan@broadcom.com On 01/28/2011 08:00 PM, Bhanu Gollapudi wrote: > +void bnx2fc_fill_fc_hdr(struct fc_frame_header *fc_hdr, enum fc_rctl r_ctl, > + u32 sid, u32 did, enum fc_fh_type fh_type, > + u32 f_ctl, u32 parm_offset) > +{ > + hton24(fc_hdr->fh_s_id, sid); > + hton24(fc_hdr->fh_d_id, did); > + fc_hdr->fh_r_ctl = r_ctl; > + fc_hdr->fh_type = fh_type; > + hton24(fc_hdr->fh_f_ctl, f_ctl); > + fc_hdr->fh_cs_ctl = 0; > + fc_hdr->fh_df_ctl = 0; > + fc_hdr->fh_parm_offset = htonl(parm_offset); > +} This is just fc_fill_fc_hdr() right? > +} > + > +static int bnx2fc_initiate_els(struct bnx2fc_rport *tgt, unsigned int op, > + void *data, u32 data_len, > + void (*cb_func)(struct bnx2fc_els_cb_arg *cb_arg), > + struct bnx2fc_els_cb_arg *cb_arg, u32 timer_msec) > + els_req->cb_arg = cb_arg; > + > + mp_req = (struct bnx2fc_mp_req *)&(els_req->mp_req); > + rc = bnx2fc_init_mp_req(els_req); > + if (rc == FAILED) { > + printk(KERN_ALERT PFX "ELS MP request init failed\n"); > + spin_lock_bh(&tgt->tgt_lock); > + kref_put(&els_req->refcount, bnx2fc_cmd_release); > + spin_unlock_bh(&tgt->tgt_lock); > + goto els_err; Normally you return a -EXYZ in this function, but here you return the scsi value FAILED. > + > +els_err: > + return rc; > +} > +} > + > +void bnx2fc_process_l2_frame_compl(struct bnx2fc_rport *tgt, > + unsigned char *buf, > + u32 frame_len, u16 l2_oxid) > +{ > + struct fcoe_port *port = tgt->port; > + struct bnx2fc_hba *hba = port->priv; > + struct fc_lport *lport = port->lport; > + struct fc_frame *fp; > + struct fc_frame_header *fh; > + struct fcoe_hdr *hp; > + struct fcoe_crc_eof *cp; > + struct fcoe_rcv_info *fr; > + struct fcoe_percpu_s *bg; > + > + struct sk_buff *skb; > + unsigned int hlen; /* header length implies the version */ > + unsigned int tlen; /* trailer length */ > + unsigned int elen; /* eth header, may include vlan */ > + u32 crc; > + struct ethhdr *eh; > + > + u32 payload_len; > + u16 oxid; > + int idx; > + u8 op; > + > + BNX2FC_TGT_DBG(tgt, "l2_frame_compl l2_oxid = 0x%x, frame_len = %d\n", > + l2_oxid, frame_len); > + payload_len = frame_len - sizeof(struct fc_frame_header); > + > + fp = fc_frame_alloc(lport, payload_len); > + if (!fp) { > + printk(KERN_ERR PFX "fc_frame_alloc failure\n"); > + return; > + } > + > + fh = (struct fc_frame_header *) fc_frame_header_get(fp); > + /* Copy FC Frame header and payload into the frame */ > + memcpy(fh, buf, frame_len); > + > + if (l2_oxid != FC_XID_UNKNOWN) > + fh->fh_ox_id = htons(l2_oxid); > + > + skb = fp_skb(fp); > + > + if ((fh->fh_r_ctl == FC_RCTL_ELS_REQ) || > + (fh->fh_r_ctl == FC_RCTL_ELS_REP)) { > + > + if (fh->fh_type == FC_TYPE_ELS) { > + op = fc_frame_payload_op(fp); > + if ((op == ELS_TEST) || (op == ELS_ESTC) || > + (op == ELS_FAN) || (op == ELS_CSU)) { > + /* > + * No need to reply for these > + * ELS requests > + */ > + printk(KERN_ERR PFX "dropping ELS 0x%x\n", op); > + kfree_skb(skb); > + return; > + } > + } > + /* Pass it on to libfc for further processing */ I did not get this comment. Did you mean to put the code below in the fcoe lib, then call it? It looks identical to fcoe_xmit. > + elen = (hba->netdev->priv_flags& IFF_802_1Q_VLAN) ? > + sizeof(struct vlan_ethhdr) : > + sizeof(struct ethhdr); > + hlen = sizeof(struct fcoe_hdr); > + tlen = sizeof(struct fcoe_crc_eof); > + skb->ip_summed = CHECKSUM_NONE; > + crc = fcoe_fc_crc(fp); > + > + if (skb_is_nonlinear(skb)) { > + skb_frag_t *frag; > + if (bnx2fc_get_paged_crc_eof(skb, tlen)) { > + printk(KERN_ERR PFX "Frame crc error\n"); > + kfree_skb(skb); > + return; > + } > + idx = skb_shinfo(skb)->nr_frags - 1; > + frag =&skb_shinfo(skb)->frags[idx]; > + cp = kmap_atomic(frag->page, > + KM_SKB_DATA_SOFTIRQ) + > + frag->page_offset; > + } else { > + cp = (struct fcoe_crc_eof *)skb_put(skb, tlen); > + } > + memset(cp, 0, sizeof(*cp)); > + /* > + * We are supposed to receive single frame > + * sequences from the firmware > + */ > + cp->fcoe_eof = FC_EOF_T; > + cp->fcoe_crc32 = cpu_to_le32(~crc); > + > + if (skb_is_nonlinear(skb)) { > + kunmap_atomic(cp, KM_SKB_DATA_SOFTIRQ); > + cp = NULL; > + } > + > + skb_push(skb, hlen + elen); > + skb_reset_mac_header(skb); > + skb_set_network_header(skb, elen); > + skb_set_transport_header(skb, elen+hlen); > + skb->mac_len = elen; > + skb->protocol = htons(ETH_P_FCOE); > + skb->dev = hba->netdev; > + > + /* adjust skb->data to point to fcoe_hdr */ > + skb_pull(skb, elen); > + > + eh = eth_hdr(skb); > + eh->h_proto = htons(ETH_P_FCOE); > + > + /* Should we be setting eh->h_dest and h_source? */ > + memcpy(eh->h_dest, port->data_src_addr, ETH_ALEN); > + > + hp = (struct fcoe_hdr *)skb_network_header(skb); > + memset(hp, 0, sizeof(*hp)); > + if (FC_FCOE_VER) > + FC_FCOE_ENCAPS_VER(hp, FC_FCOE_VER); > + > + /* needs to be less than 0x30 - FC_SOF_N3 */ > + hp->fcoe_sof = FC_SOF_I3; > + > + fr = fcoe_dev_from_skb(skb); > + fr->fr_dev = lport; > + oxid = ntohs(fh->fh_ox_id); > + > + bg =&bnx2fc_global; > + spin_lock_bh(&bg->fcoe_rx_list.lock); > + > + if (unlikely(!bg->thread)) { > + spin_unlock_bh(&bg->fcoe_rx_list.lock); > + printk(KERN_ERR PFX "ERROR-thread is NULL\n"); > + kfree_skb(skb); > + return; > + } > + __skb_queue_tail(&bg->fcoe_rx_list, skb); > + if (bg->fcoe_rx_list.qlen == 1) > + wake_up_process(bg->thread); > + spin_unlock_bh(&bg->fcoe_rx_list.lock); > + } else { > + BNX2FC_HBA_DBG(lport, "fh_r_ctl = 0x%x\n", fh->fh_r_ctl); > + kfree_skb(skb); > + } > +} > + > +static void bnx2fc_process_unsol_compl(struct bnx2fc_rport *tgt, u16 wqe) > +{ > + > + num_rq = (frame_len + BNX2FC_RQ_BUF_SZ - 1) / BNX2FC_RQ_BUF_SZ; > + > + rq_data = (unsigned char *)bnx2fc_get_next_rqe(tgt, num_rq); > + if (rq_data) { > + buf = rq_data; > + } else { > + buf1 = buf = kmalloc((num_rq * BNX2FC_RQ_BUF_SZ), > + GFP_ATOMIC); Need to check for NULL before accessing buf1 below. > + for (i = 0; i< num_rq; i++) { > + rq_data = (unsigned char *) > + bnx2fc_get_next_rqe(tgt, 1); > + len = BNX2FC_RQ_BUF_SZ; > + memcpy(buf1, rq_data, len); > + buf1 += len; > + } > + } > + bnx2fc_process_l2_frame_compl(tgt, buf, frame_len, > + FC_XID_UNKNOWN); > + > + > +struct bnx2fc_work *bnx2fc_alloc_work(struct bnx2fc_rport *tgt, u16 wqe) > +{ > + struct bnx2fc_work *work; > + work = kzalloc(sizeof(struct bnx2fc_work), GFP_ATOMIC); > + if (!work) > + return NULL; > + > + INIT_LIST_HEAD(&work->list); > + work->tgt = tgt; > + work->wqe = wqe; > + return work; > +} > + > +int bnx2fc_process_new_cqes(struct bnx2fc_rport *tgt) > +{ > + struct fcoe_cqe *cq; > + u32 cq_cons; > + struct fcoe_cqe *cqe; > + u16 wqe; > + bool more_cqes_found = false; > + > + /* > + * cq_lock is a low contention lock used to protect > + * the CQ data structure from being freed up during > + * the upload operation > + */ > + spin_lock_bh(&tgt->cq_lock); > + > + if (!tgt->cq) { > + printk(KERN_ERR PFX "process_new_cqes: cq is NULL\n"); > + spin_unlock_bh(&tgt->cq_lock); > + return 0; > + } > + cq = tgt->cq; > + cq_cons = tgt->cq_cons_idx; > + cqe =&cq[cq_cons]; > + > + do { > + more_cqes_found ^= true; > + > + while (((wqe = cqe->wqe)& FCOE_CQE_TOGGLE_BIT) == > + (tgt->cq_curr_toggle_bit<< > + FCOE_CQE_TOGGLE_BIT_SHIFT)) { > + > + /* new entry on the cq */ > + if (wqe& FCOE_CQE_CQE_TYPE) { > + /* Unsolicited event notification */ > + bnx2fc_process_unsol_compl(tgt, wqe); > + } else { > + struct bnx2fc_work *work = NULL; > + struct bnx2fc_percpu_s *fps = NULL; > + unsigned int cpu = wqe % num_possible_cpus(); > + > + fps =&per_cpu(bnx2fc_percpu, cpu); > + spin_lock_bh(&fps->fp_work_lock); > + if (unlikely(!fps->iothread)) > + goto unlock; > + > + work = bnx2fc_alloc_work(tgt, wqe); > + if (!work) > + goto unlock; > + > + list_add_tail(&work->list,&fps->work_list); > +unlock: This seems like one of the most evil uses of goto :) Why not just do if (work) list_add_tail(&work->list,&fps->work_list); spin_unlock_bh(&fps->fp_work_lock); > + spin_unlock_bh(&fps->fp_work_lock); > + > + /* Pending work request completion */ > + if (fps->iothread&& work) > + wake_up_process(fps->iothread); This looks like blk io poll but with a thread instead of a softirq. Use what is in the kernel already. > + else > + bnx2fc_process_cq_compl(tgt, wqe); > + }