Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

Alexey Dobriyan
2006-08-18T14:44:41Z

On Fri, Aug 18, 2006 at 01:29:01PM +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 struct net_device_stats *ehea_get_stats(struct net_device *dev)
> +{
> + int i;
> + u64 hret = H_HARDWARE;

Useless assignment here and everywhere.

> + u64 rx_packets = 0;
> + struct ehea_port *port = netdev_priv(dev);
> + struct hcp_query_ehea_port_cb_2 *cb2 = NULL;
> + struct net_device_stats *stats = &port->stats;
> +
> + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> + if (!cb2) {
> + ehea_error("no mem for cb2");
> + goto get_stat_exit;
> + }
> +
> + hret = ehea_h_query_ehea_port(port->adapter->handle,
> + port->logical_port_id,
> + H_PORT_CB2,
> + H_PORT_CB2_ALL,
> + cb2);

> +static inline int ehea_refill_rq1(struct ehea_port_res *pr, int index,
> + int nr_of_wqes)
> +{
> + int i;
> + int ret = 0;
> + struct sk_buff **skb_arr_rq1 = pr->skb_arr_rq1;
> + int max_index_mask = pr->skb_arr_rq1_len - 1;
> +
> + if (!nr_of_wqes)
> + return 0;
> +
> + for (i = 0; i < nr_of_wqes; i++) {
> + if (!skb_arr_rq1[index]) {
> + skb_arr_rq1[index] = dev_alloc_skb(EHEA_LL_PKT_SIZE);
> +
> + if (!skb_arr_rq1[index]) {
> + ehea_error("no mem for skb/%d wqes filled", i);
> + ret = -ENOMEM;
> + break;
> + }
> + }
> + index--;
> + index &= max_index_mask;
> + }
> + /* Ring doorbell */
> + ehea_update_rq1a(pr->qp, i);
> +
> + return ret;
> +}
> +
> +static int ehea_init_fill_rq1(struct ehea_port_res *pr, int nr_rq1a)
> +{
> + int i;
> + int ret = 0;
> + struct sk_buff **skb_arr_rq1 = pr->skb_arr_rq1;
> +
> + for (i = 0; i < pr->skb_arr_rq1_len; i++) {
> + skb_arr_rq1[i] = dev_alloc_skb(EHEA_LL_PKT_SIZE);
> + if (!skb_arr_rq1[i]) {
> + ehea_error("no mem for skb/%d skbs filled.", i);
> + ret = -ENOMEM;
> + goto nomem;
> + }
> + }
> + /* Ring doorbell */
> + ehea_update_rq1a(pr->qp, nr_rq1a);
> +nomem:
> + return ret;
> +}
> +
> +static inline int ehea_refill_rq2_def(struct ehea_port_res *pr, int nr_of_wqes)
> +{
> + int i;
> + int ret = 0;
> + struct ehea_qp *qp;
> + struct ehea_rwqe *rwqe;
> + struct sk_buff **skb_arr_rq2 = pr->skb_arr_rq2;
> + int index, max_index_mask;
> +
> + if (!nr_of_wqes)
> + return 0;
> +
> + qp = pr->qp;
> +
> + index = pr->skb_rq2_index;
> + max_index_mask = pr->skb_arr_rq2_len - 1;
> + for (i = 0; i < nr_of_wqes; i++) {
> + struct sk_buff *skb = dev_alloc_skb(EHEA_RQ2_PKT_SIZE
> + + NET_IP_ALIGN);
> + if (!skb) {
> + ehea_error("no mem for skb/%d wqes filled", i);
> + ret = -ENOMEM;
> + break;
> + }
> + skb_reserve(skb, NET_IP_ALIGN);
> +
> + skb_arr_rq2[index] = skb;
> +
> + rwqe = ehea_get_next_rwqe(qp, 2);
> + rwqe->wr_id = EHEA_BMASK_SET(EHEA_WR_ID_TYPE, EHEA_RWQE2_TYPE)
> + | EHEA_BMASK_SET(EHEA_WR_ID_INDEX, index);
> + rwqe->sg_list[0].l_key = pr->recv_mr.lkey;
> + rwqe->sg_list[0].vaddr = (u64)skb->data;
> + rwqe->sg_list[0].len = EHEA_RQ2_PKT_SIZE;
> + rwqe->data_segments = 1;
> +
> + index++;
> + index &= max_index_mask;
> + }
> + pr->skb_rq2_index = index;
> +
> + /* Ring doorbell */
> + iosync();
> + ehea_update_rq2a(qp, i);
> + return ret;
> +}
> +
> +
> +static inline int ehea_refill_rq2(struct ehea_port_res *pr, int nr_of_wqes)
> +{
> + return ehea_refill_rq2_def(pr, nr_of_wqes);
> +}
> +
> +static inline int ehea_refill_rq3_def(struct ehea_port_res *pr, int nr_of_wqes)
> +{
> + int i;
> + int ret = 0;
> + struct ehea_qp *qp = pr->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;
> + int index, max_index_mask;
> +
> + if (nr_of_wqes == 0)
> + return -EINVAL;
> +
> + index = pr->skb_rq3_index;
> + max_index_mask = skb_arr_rq3_len - 1;
> + for (i = 0; i < nr_of_wqes; i++) {
> + struct sk_buff *skb = dev_alloc_skb(EHEA_MAX_PACKET_SIZE
> + + NET_IP_ALIGN);
> + if (!skb) {
> + ehea_error("no mem for skb/%d wqes filled", i);
> + ret = -ENOMEM;
> + break;
> + }
> + skb_reserve(skb, NET_IP_ALIGN);
> +
> + rwqe = ehea_get_next_rwqe(qp, 3);
> +
> + 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 = pr->recv_mr.lkey;
> + rwqe->sg_list[0].vaddr = (u64)skb->data;
> + rwqe->sg_list[0].len = EHEA_MAX_PACKET_SIZE;
> + rwqe->data_segments = 1;
> +
> + index++;
> + index &= max_index_mask;
> + }
> + pr->skb_rq3_index = index;
> +
> + /* Ring doorbell */
> + iosync();
> + ehea_update_rq3a(qp, i);
> + return ret;
> +}

This one looks too big to be inlined as well as other similalry named
functions.

> +static inline int ehea_refill_rq3(struct ehea_port_res *pr, int nr_of_wqes)
> +{
> + return ehea_refill_rq3_def(pr, nr_of_wqes);
> +}
> +
> +static inline int ehea_check_cqe(struct ehea_cqe *cqe, int *rq_num)
> +{
> + *rq_num = (cqe->type & EHEA_CQE_TYPE_RQ) >> 5;
> + if ((cqe->status & EHEA_CQE_STAT_ERR_MASK) == 0)
> + return 0;
> + if (((cqe->status & EHEA_CQE_STAT_ERR_TCP) != 0)
> + && (cqe->header_length == 0))
> + return 0;
> + return -EINVAL;
> +}
> +
> +static inline void ehea_fill_skb(struct net_device *dev,
> + struct sk_buff *skb, struct ehea_cqe *cqe)
> +{
> + int length = cqe->num_bytes_transfered - 4; /*remove CRC */
> + skb_put(skb, length);
> + skb->dev = dev;
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + skb->protocol = eth_type_trans(skb, dev);
> +}

> +static int ehea_init_port_res(struct ehea_port *port, struct ehea_port_res *pr,
> + struct port_res_cfg *pr_cfg, int queue_token)
> +{
> + int ret = -EINVAL;
> + int max_rq_entries = 0;
> + enum ehea_eq_type eq_type = EHEA_EQ;
> + struct ehea_qp_init_attr *init_attr = NULL;
> + struct ehea_adapter *adapter = port->adapter;
> +
> + memset(pr, 0, sizeof(struct ehea_port_res));
> +
> + pr->skb_arr_rq3 = NULL;
> + pr->skb_arr_rq2 = NULL;
> + pr->skb_arr_rq1 = NULL;
> + pr->skb_arr_sq = NULL;
> + pr->qp = NULL;
> + pr->send_cq = NULL;
> + pr->recv_cq = NULL;
> + pr->send_eq = NULL;
> + pr->recv_eq = NULL;

After memset unneeded. ;-)

> +static int ehea_clean_port_res(struct ehea_port *port, struct ehea_port_res *pr)

Freeing/deallocating/cleaning functions usually return void. The fact
that you always return -EINVAL only reaffirmes my belief. ;-)

> +{
> + int i;
> + int ret = -EINVAL;
> +
> + ehea_destroy_qp(pr->qp);
> + ehea_destroy_cq(pr->send_cq);
> + ehea_destroy_cq(pr->recv_cq);
> + ehea_destroy_eq(pr->send_eq);
> + ehea_destroy_eq(pr->recv_eq);
> +
> + for (i = 0; i < pr->skb_arr_rq1_len; i++) {
> + if (pr->skb_arr_rq1[i])
> + dev_kfree_skb(pr->skb_arr_rq1[i]);
> + }
> +
> + for (i = 0; i < pr->skb_arr_rq2_len; i++)
> + if (pr->skb_arr_rq2[i])
> + dev_kfree_skb(pr->skb_arr_rq2[i]);
> +
> + for (i = 0; i < pr->skb_arr_rq3_len; i++)
> + if (pr->skb_arr_rq3[i])
> + dev_kfree_skb(pr->skb_arr_rq3[i]);
> +
> + for (i = 0; i < pr->skb_arr_sq_len; i++)
> + if (pr->skb_arr_sq[i])
> + dev_kfree_skb(pr->skb_arr_sq[i]);
> +
> + vfree(pr->skb_arr_sq);
> + vfree(pr->skb_arr_rq1);
> + vfree(pr->skb_arr_rq2);
> + vfree(pr->skb_arr_rq3);
> +
> + ehea_rem_smrs(pr);
> + return ret;
> +}

> +static inline void write_swqe2_data(struct sk_buff *skb,
> + struct net_device *dev,
> + struct ehea_swqe *swqe,
> + u32 lkey)
> +{

Way too big.

> + int skb_data_size, nfrags, headersize, i, sg1entry_contains_frag_data;
> + struct ehea_vsgentry *sg_list;
> + struct ehea_vsgentry *sg1entry;
> + struct ehea_vsgentry *sgentry;
> + u8 *imm_data;
> + u64 tmp_addr;
> + skb_frag_t *frag;
> +
> + skb_data_size = skb->len - skb->data_len;
> + nfrags = skb_shinfo(skb)->nr_frags;
> + sg1entry = &swqe->u.immdata_desc.sg_entry;
> + sg_list = (struct ehea_vsgentry*)&swqe->u.immdata_desc.sg_list;
> + imm_data = &swqe->u.immdata_desc.immediate_data[0];
> + swqe->descriptors = 0;
> + sg1entry_contains_frag_data = 0;
> +
> + if ((dev->features & NETIF_F_TSO) && skb_shinfo(skb)->gso_size) {
> + /* Packet is TCP with TSO enabled */
> + swqe->tx_control |= EHEA_SWQE_TSO;
> + swqe->mss = skb_shinfo(skb)->gso_size;
> + /* copy only eth/ip/tcp headers to immediate data and
> + * the rest of skb->data to sg1entry
> + */
> + headersize = ETH_HLEN + (skb->nh.iph->ihl * 4)
> + + (skb->h.th->doff * 4);
> +
> + skb_data_size = skb->len - skb->data_len;
> +
> + if (skb_data_size >= headersize) {
> + /* copy immediate data */
> + memcpy(imm_data, skb->data, headersize);
> + swqe->immediate_data_length = headersize;
> +
> + if (skb_data_size > headersize) {
> + /* set sg1entry data */
> + sg1entry->l_key = lkey;
> + sg1entry->len = skb_data_size - headersize;
> +
> + tmp_addr = (u64)(skb->data + headersize);
> + sg1entry->vaddr = tmp_addr;
> + swqe->descriptors++;
> + }
> + } else
> + ehea_error("cannot handle fragmented headers");
> + } else {
> + /* Packet is any nonTSO type
> + *
> + * Copy as much as possible skb->data to immediate data and
> + * the rest to sg1entry
> + */
> + if (skb_data_size >= SWQE2_MAX_IMM) {
> + /* copy immediate data */
> + memcpy(imm_data, skb->data, SWQE2_MAX_IMM);
> +
> + swqe->immediate_data_length = SWQE2_MAX_IMM;
> +
> + if (skb_data_size > SWQE2_MAX_IMM) {
> + /* copy sg1entry data */
> + sg1entry->l_key = lkey;
> + sg1entry->len = skb_data_size - SWQE2_MAX_IMM;
> + tmp_addr = (u64)(skb->data + SWQE2_MAX_IMM);
> + sg1entry->vaddr = tmp_addr;
> + swqe->descriptors++;
> + }
> + } else {
> + memcpy(imm_data, skb->data, skb_data_size);
> + swqe->immediate_data_length = skb_data_size;
> + }
> + }
> +
> + /* write descriptors */
> + if (nfrags > 0) {
> + if (swqe->descriptors == 0) {
> + /* sg1entry not yet used */
> + frag = &skb_shinfo(skb)->frags[0];
> +
> + /* copy sg1entry data */
> + sg1entry->l_key = lkey;
> + sg1entry->len = frag->size;
> + tmp_addr = (u64)(page_address(frag->page)
> + + frag->page_offset);
> + sg1entry->vaddr = tmp_addr;
> + swqe->descriptors++;
> + sg1entry_contains_frag_data = 1;
> + }
> +
> + for (i = sg1entry_contains_frag_data; i < nfrags; i++) {
> +
> + frag = &skb_shinfo(skb)->frags[i];
> + sgentry = &sg_list[i - sg1entry_contains_frag_data];
> +
> + sgentry->l_key = lkey;
> + sgentry->len = frag->size;
> +
> + tmp_addr = (u64)(page_address(frag->page)
> + + frag->page_offset);
> + sgentry->vaddr = tmp_addr;
> + }
> + }
> +}

Got it?

> +static inline void ehea_xmit2(struct sk_buff *skb,
> + struct net_device *dev, struct ehea_swqe *swqe,
> + u32 lkey)
> +{
> + int nfrags;
> + nfrags = skb_shinfo(skb)->nr_frags;
> +
> + if (skb->protocol == htons(ETH_P_IP)) {
> + /* IPv4 */
> + swqe->tx_control |= EHEA_SWQE_CRC
> + | EHEA_SWQE_IP_CHECKSUM
> + | EHEA_SWQE_TCP_CHECKSUM
> + | EHEA_SWQE_IMM_DATA_PRESENT
> + | EHEA_SWQE_DESCRIPTORS_PRESENT;
> +
> + write_ip_start_end(swqe, skb);
> +
> + if (skb->nh.iph->protocol == IPPROTO_UDP) {
> + if ((skb->nh.iph->frag_off & IP_MF)
> + || (skb->nh.iph->frag_off & IP_OFFSET))
> + /* IP fragment, so don't change cs */
> + swqe->tx_control &= ~EHEA_SWQE_TCP_CHECKSUM;
> + else
> + write_udp_offset_end(swqe, skb);
> +
> + } else if (skb->nh.iph->protocol == IPPROTO_TCP) {
> + write_tcp_offset_end(swqe, skb);
> + }
> +
> + /* icmp (big data) and ip segmentation packets (all other ip
> + packets) do not require any special handling */
> +
> + } else {
> + /* Other Ethernet Protocol */
> + swqe->tx_control |= EHEA_SWQE_CRC
> + | EHEA_SWQE_IMM_DATA_PRESENT
> + | EHEA_SWQE_DESCRIPTORS_PRESENT;
> + }
> +
> + write_swqe2_data(skb, dev, swqe, lkey);
> +}
> +
> +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];
> +
> + if (likely(skb->protocol == htons(ETH_P_IP))) {
> + /* IPv4 */
> + write_ip_start_end(swqe, skb);
> +
> + if (skb->nh.iph->protocol == IPPROTO_TCP) {
> + swqe->tx_control |= EHEA_SWQE_CRC
> + | EHEA_SWQE_IP_CHECKSUM
> + | EHEA_SWQE_TCP_CHECKSUM
> + | EHEA_SWQE_IMM_DATA_PRESENT;
> +
> + write_tcp_offset_end(swqe, skb);
> +
> + } else if (skb->nh.iph->protocol == IPPROTO_UDP) {
> + if ((skb->nh.iph->frag_off & IP_MF)
> + || (skb->nh.iph->frag_off & IP_OFFSET))
> + /* IP fragment, so don't change cs */
> + swqe->tx_control |= EHEA_SWQE_CRC
> + | EHEA_SWQE_IMM_DATA_PRESENT;
> + else {
> + swqe->tx_control |= EHEA_SWQE_CRC
> + | EHEA_SWQE_IP_CHECKSUM
> + | EHEA_SWQE_TCP_CHECKSUM
> + | EHEA_SWQE_IMM_DATA_PRESENT;
> +
> + write_udp_offset_end(swqe, skb);
> + }
> + } else {
> + /* icmp (big data) and
> + ip segmentation packets (all other ip packets) */
> + swqe->tx_control |= EHEA_SWQE_CRC
> + | EHEA_SWQE_IP_CHECKSUM
> + | EHEA_SWQE_IMM_DATA_PRESENT;
> + }
> + } else {
> + /* Other Ethernet Protocol */
> + swqe->tx_control |= EHEA_SWQE_CRC | EHEA_SWQE_IMM_DATA_PRESENT;
> + }
> + /* copy (immediate) data */
> + if (nfrags == 0) {
> + /* data is in a single piece */
> + memcpy(imm_data, skb->data, skb->len);
> + } else {
> + /* first copy data from the skb->data buffer ... */
> + memcpy(imm_data, skb->data, skb->len - skb->data_len);
> + imm_data += skb->len - skb->data_len;
> +
> + /* ... then copy data from the fragments */
> + for (i = 0; i < nfrags; i++) {
> + frag = &skb_shinfo(skb)->frags[i];
> + memcpy(imm_data,
> + page_address(frag->page) + frag->page_offset,
> + frag->size);
> + imm_data += frag->size;
> + }
> + }
> + swqe->immediate_data_length = skb->len;
> + dev_kfree_skb(skb);
> +}

Ditto.

> +static void ehea_vlan_rx_register(struct net_device *dev,
> + struct vlan_group *grp)
> +{
> + u64 hret = H_HARDWARE;
> + struct hcp_query_ehea_port_cb_1 *cb1 = NULL;
> + struct ehea_port *port = netdev_priv(dev);
> + struct ehea_adapter *adapter = port->adapter;
> +
> + port->vgrp = grp;
> +
> + cb1 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> + if (!cb1) {
> + ehea_error("no mem for cb1");
> + goto vlan_reg_exit;
> + }
> +
> + if (grp)
> + memset(cb1->vlan_filter, 0, sizeof(cb1->vlan_filter));
> + else
> + memset(cb1->vlan_filter, 1, sizeof(cb1->vlan_filter));

Just to be sure, this should be 1 not 0xff?

> + hret = ehea_h_modify_ehea_port(adapter->handle,
> + port->logical_port_id,
> + H_PORT_CB1,
> + H_PORT_CB1_ALL,
> + cb1);
> + if (hret != H_SUCCESS)
> + ehea_error("modify_ehea_port failed");
> +
> + kfree(cb1);
> +
> +vlan_reg_exit:
> + return;
> +}

> +void ehea_clean_all_port_res(struct ehea_port *port)
> +{
> + int ret;
> + int i;
> + for(i = 0; i < port->num_def_qps + port->num_tx_qps; i++)
> + ehea_clean_port_res(port, &port->port_res[i]);
> +
> + ret = ehea_destroy_eq(port->qp_eq);
> +}

ret is entirely useless.

> +int __init ehea_module_init(void)
static

> +{
> + int ret = -EINVAL;
> +
> + printk("IBM eHEA Ethernet Device Driver (Release %s)\n", DRV_VERSION);
> +
> + ret = ibmebus_register_driver(&ehea_driver);
> + if (ret) {
> + ehea_error("failed registering eHEA device driver on ebus");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}

Pass ret to upper layer. Simplest way is:

static int __init ehea_module_init(void)
{
return ibmebus_register_driver(&ehea_driver);
}