From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mtagate2.de.ibm.com (mtagate2.de.ibm.com [195.212.29.151]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mtagate2.de.ibm.com", Issuer "Equifax" (verified OK)) by ozlabs.org (Postfix) with ESMTP id E49BC67B54 for ; Fri, 11 Aug 2006 01:28:00 +1000 (EST) Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate2.de.ibm.com (8.13.7/8.13.7) with ESMTP id k7AFRuTf112516 for ; Thu, 10 Aug 2006 15:27:56 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.6/8.13.6/NCO v8.1.1) with ESMTP id k7AFVbP4143562 for ; Thu, 10 Aug 2006 17:31:37 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id k7AFRtEx019967 for ; Thu, 10 Aug 2006 17:27:55 +0200 Message-ID: <44DB4782.9050602@de.ibm.com> Date: Thu, 10 Aug 2006 16:49:38 +0200 From: Jan-Bernd Themann MIME-Version: 1.0 To: Alexey Dobriyan Subject: Re: [PATCH 1/6] ehea: interface to network stack References: <44D99EFC.3000105@de.ibm.com> <20060809130646.GA6846@martell.zuzino.mipt.ru> In-Reply-To: <20060809130646.GA6846@martell.zuzino.mipt.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Cc: Thomas Klein , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org, Christoph Raisch , Marcus Eder List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi, thanks for your comments! We'll post a modified patch very soon. Jan-Bernd Alexey Dobriyan wrote: > On Wed, Aug 09, 2006 at 10:38:20AM +0200, Jan-Bernd Themann wrote: >> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c >> +++ kernel/drivers/net/ehea/ehea_main.c > >> +static inline u64 get_swqe_addr(u64 tmp_addr, int addr_seg) >> +{ >> + u64 addr; >> + addr = tmp_addr; >> + return addr; >> +} >> + >> +static inline u64 get_rwqe_addr(u64 tmp_addr) >> +{ >> + return tmp_addr; >> +} > > The point of this exercise? has been removed > >> +static inline int ehea_refill_rq3_def(struct ehea_port_res *pr, int nr_of_wqes) > > Way too big to be inline function. > >> +{ >> + int i; >> + int ret = 0; >> + struct ehea_qp *qp; >> + struct ehea_rwqe *rwqe; >> + int skb_arr_rq3_len = pr->skb_arr_rq3_len; >> + struct sk_buff **skb_arr_rq3 = pr->skb_arr_rq3; >> + EDEB_EN(8, "pr=%p, nr_of_wqes=%d", pr, nr_of_wqes); >> + if (nr_of_wqes == 0) >> + return -EINVAL; >> + qp = pr->qp; >> + for (i = 0; i < nr_of_wqes; i++) { >> + int index = pr->skb_rq3_index++; >> + struct sk_buff *skb = dev_alloc_skb(EHEA_MAX_PACKET_SIZE >> + + NET_IP_ALIGN); >> + >> + if (!skb) { >> + EDEB_ERR(4, "No memory for skb. Only %d rwqe >> filled.", >> + i); >> + ret = -ENOMEM; >> + break; >> + } >> + skb_reserve(skb, NET_IP_ALIGN); >> + >> + rwqe = ehea_get_next_rwqe(qp, 3); >> + pr->skb_rq3_index %= skb_arr_rq3_len; >> + skb_arr_rq3[index] = skb; >> + rwqe->wr_id = EHEA_BMASK_SET(EHEA_WR_ID_TYPE, >> EHEA_RWQE3_TYPE) >> + | EHEA_BMASK_SET(EHEA_WR_ID_INDEX, index); >> + rwqe->sg_list[0].l_key = ehea_get_recv_lkey(pr); >> + rwqe->sg_list[0].vaddr = get_rwqe_addr((u64)skb->data); >> + rwqe->sg_list[0].len = EHEA_MAX_PACKET_SIZE; >> + rwqe->data_segments = 1; >> + } >> + >> + /* Ring doorbell */ >> + iosync(); >> + ehea_update_rq3a(qp, i); >> + EDEB_EX(8, ""); >> + return ret; >> +} >> + >> + >> +static inline int ehea_refill_rq3(struct ehea_port_res *pr, int nr_of_wqes) >> +{ >> + return ehea_refill_rq3_def(pr, nr_of_wqes); >> +} > > ehea_refill_rq3[123] appears to be 1:1 wrappers around > ehea_refill_rq3[123]_def. Any idea behind them? > introduced for near future features >> + init_attr = (struct ehea_qp_init_attr*) >> + kzalloc(sizeof(struct ehea_qp_init_attr), GFP_KERNEL); > > Useless cast. > removed >> + pr->skb_arr_sq = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*) >> + * (max_rq_entries + 1)); > > Useless cast removed > >> + pr->skb_arr_rq1 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*) >> + * (max_rq_entries + 1)); > >> + pr->skb_arr_rq2 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*) >> + * (max_rq_entries + 1)); > >> + pr->skb_arr_rq3 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*) >> + * (max_rq_entries + 1)); > >> +static int ehea_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) >> +{ >> + EDEB_ERR(4, "ioctl not supported: dev=%s cmd=%d", dev->name, cmd); > > Then copy NULL into ->do_ioctl! > done >> + return -EOPNOTSUPP; >> +} > >> + ehea_port_cb_0 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL); >> + >> + if (!ehea_port_cb_0) { >> + EDEB_ERR(4, "No memory for ehea_port control block"); >> + ret = -ENOMEM; >> + goto kzalloc_failed; >> + } >> + >> + memcpy((u8*)(&(ehea_port_cb_0->port_mac_addr)), >> + (u8*)&(mac_addr->sa_data[0]), 6); > > No casts on memcpy arguments. done > >> + memcpy((u8*)&ehea_mcl_entry->macaddr, mc_mac_addr, ETH_ALEN); > >> +static inline void ehea_xmit2(struct sk_buff *skb, >> + struct net_device *dev, struct ehea_swqe *swqe, >> + u32 lkey) >> +{ >> + int nfrags; >> + unsigned short skb_protocol = skb->protocol; > > Useless variable. And it should be __be16, FYI. > changed >> + nfrags = skb_shinfo(skb)->nr_frags; >> + EDEB_EN(7, "skb->nfrags=%d (0x%X)", nfrags, nfrags); >> + >> + if (skb_protocol == ETH_P_IP) { > > ITYM, htons(ETH_P_IP). > good point, thx >> +static inline void ehea_xmit3(struct sk_buff *skb, >> + struct net_device *dev, struct ehea_swqe *swqe) >> +{ >> + int i; >> + skb_frag_t *frag; >> + int nfrags = skb_shinfo(skb)->nr_frags; >> + u8 *imm_data = &swqe->u.immdata_nodesc.immediate_data[0]; >> + u64 skb_protocol = skb->protocol; > > Useless var. removed > >> + >> + EDEB_EN(7, ""); >> + if (likely(skb_protocol == ETH_P_IP)) { > > htons(ETH_P_IP) >