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

[Thomas Klein]

Hi Francois,

thanks for your review and your comments. See below our answers.

Regards
Thomas



Francois Romieu wrote:

>> +    cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
>> +    if (!cb2) {
>> +            ehea_error("no mem for cb2");
>> +            goto kzalloc_failed;
>
> It's better when the label tell what it does than where it comes from.
> If it's numbered too, one can check them without going back and forth.
>> +    stats->tx_packets = cb2->txucp + cb2->txmcp + cb2->txbcp;
>> +    stats->multicast = cb2->rxmcp;
>> +    stats->rx_errors = cb2->rxuerr;
>> +    stats->rx_bytes = cb2->rxo;
>> +    stats->tx_bytes = cb2->txo;
>> +    stats->rx_packets = rx_packets;
>> +
>> +hcall_failed:
>> +        kfree(cb2);
>
> Tab was turned into spaces.

Fixed.

>> +static inline int ehea_refill_rq1(struct ehea_port_res *pr, int index,
>
> Avoid inline ?

Inline declaration was removed from this one and several other functions.

>> +    for (i = 0; i < nr_of_wqes; i++) {
>> +            if (!skb_arr_rq1[index]) {
>> +                    skb_arr_rq1[index] = dev_alloc_skb(EHEA_LL_PKT_SIZE);
>
> netdev_alloc_skb ?

Agreed & done.

>
>> +
>> +                    if (!skb_arr_rq1[index]) {
>> +                            ehea_error("no mem for skb/%d wqes filled", i);
>> +                            ret = -ENOMEM;
>
> The caller does not check the returned value.

Agreed. fn returns void now.

>> +            if (!skb_arr_rq1[i]) {
>> +                    ehea_error("no mem for skb/%d skbs filled.", i);
>> +                    ret = -ENOMEM;
>> +                    goto exit0;
>
> s/exit0/out/

Goto target naming was reworked throughout the whole driver and basically
uses the style used by Dave M. and Jeff G. in the Tigon3 driver.

>> +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))
>
> && on the previous line please.

Changed at all occurences.

>> +static inline struct sk_buff *get_skb_by_index(struct sk_buff **skb_array,
>> +                                           int arr_len,
>> +                                           struct ehea_cqe *cqe)
>> +{
>> +    int skb_index = EHEA_BMASK_GET(EHEA_WR_ID_INDEX, cqe->wr_id);
>> +    struct sk_buff *skb;
>> +    void *pref;
>> +    int x;
>> +
>> +    x = skb_index + 1;
>> +    x &= (arr_len - 1);
>> +
>> +    pref = (void*)skb_array[x];
>
> Useless cast.

Agreed -> removed.

>> +                            if (unlikely(!skb)) {
>> +                                    if (netif_msg_rx_err(port))
>> +                                            ehea_error("LL rq1: skb=NULL");
>> +                                            skb = dev_alloc_skb(EHEA_LL_PKT_SIZE);
>
> Tab/space

Fixed.

>> +irqreturn_t ehea_qp_aff_irq_handler(int irq, void *param, struct pt_regs * regs)
>
> static ?

Agreed.

>> +int ehea_sense_port_attr(struct ehea_port *port)
>
> static ?

No -> used in ehea_ethtool.c

>> +    } else {
>> +            if (hret == H_AUTHORITY)
>> +            {
>
> Misplaced curly brace.

Fixed.

>
>> +                    ehea_info("Hypervisor denied setting port speed. Either"
>> +                              " this partition is not authorized to set "
>> +                              "port speed or another partition has modified"
>> +                              " port speed first.");
>> +                    ret = -EPERM;
>> +            } else
>> +            {
>
> Misplaced curly brace.

Fixed.

>
>> +                    ret = -EIO;
>> +                    ehea_error("Failed setting port speed");
>> +            }
>> +    }
>> +    netif_carrier_on(port->netdev);
>> +exit0:
>> +    kfree(cb4);
>
> cb4 is NULL. Not wrong per se but I'd rather move the label one line down.

Agreed.

>> +void ehea_neq_tasklet(unsigned long data)
>
> static ?

Agreed.

>> +irqreturn_t ehea_interrupt_neq(int irq, void *param, struct pt_regs *regs)
>
> static ?

Agreed.

>> +{
>> +    struct ehea_adapter *adapter = (struct ehea_adapter*)param;
>
> Useless cast.

Fixed.

>> +static int ehea_fill_port_res(struct ehea_port_res *pr)
>> +{
>> +    int ret;
>> +    struct ehea_qp_init_attr *init_attr = &pr->qp->init_attr;
>> +
>> +    /* RQ 1 */
>> +    ret = ehea_init_fill_rq1(pr, init_attr->act_nr_rwqes_rq1
>> +                                 - init_attr->act_nr_rwqes_rq2
>> +                                 - init_attr->act_nr_rwqes_rq3 - 1);
>> +    /* RQ 2 */
>
> Useless comment.

Removed.

>> +                    for (k = 0; k < i; k++) {
>> +                            u32 ist = port->port_res[k].recv_eq->attr.ist1;
>> +                            ibmebus_free_irq(NULL, ist, &port->port_res[k]);
>> +                    }
>> +                    goto failure;
>
> Poor label (and bloaty release practice too: remove k, reuse "i" below
> and more importantly release the things in allocation-reversed order).

Somehow I don't get your point concerning the usage of 'k'. We need another
iterator as the for loops using 'k' use 'i' as their terminating condition.

>
>> +            }
>> +            if (netif_msg_ifup(port))
>> +                    ehea_info("irq_handle 0x%X for funct ehea_recv_int %d "
>> +                              "registered", pr->recv_eq->attr.ist1, i);
>> +    }
>> +
>> +    snprintf(port->int_aff_name, EHEA_IRQ_NAME_SIZE - 1,
>> +             "%s-aff", dev->name);
>> +    ret = ibmebus_request_irq(NULL, port->qp_eq->attr.ist1,
>> +                              ehea_qp_aff_irq_handler,
>> +                              SA_INTERRUPT, port->int_aff_name, port);
>> +    if (ret) {
>> +            ehea_error("failed registering irq for qp_aff_irq_handler:"
>> +                       " ist=%X", port->qp_eq->attr.ist1);
>> +            goto failure2;
>> +    }
>> +    if (netif_msg_ifup(port))
>> +            ehea_info("irq_handle 0x%X for function qp_aff_irq_handler "
>> +                      "registered", port->qp_eq->attr.ist1);
>> +
>> +    for (i = 0; i < port->num_def_qps + port->num_add_tx_qps; i++) {
>> +            pr = &port->port_res[i];
>> +            snprintf(pr->int_send_name, EHEA_IRQ_NAME_SIZE - 1,
>> +                     "%s-send%d", dev->name, i);
>> +            ret = ibmebus_request_irq(NULL, pr->send_eq->attr.ist1,
>> +                                      ehea_send_irq_handler,
>> +                                      SA_INTERRUPT, pr->int_send_name,
>> +                                      pr);
>> +            if (ret) {
>> +                    ehea_error("failed registering irq for ehea_send"
>> +                               " port_res_nr:%d, ist=%X", i,
>> +                               pr->send_eq->attr.ist1);
>> +                    for (k = 0; k < i; k++) {
>> +                            u32 ist = port->port_res[k].send_eq->attr.ist1;
>> +                            ibmebus_free_irq(NULL, ist, &port->port_res[i]);
>> +                    }
>> +                    goto failure3;
>> +            }
>> +            if (netif_msg_ifup(port))
>> +                    ehea_info("irq_handle 0x%X for function ehea_send_int "
>> +                              "%d registered", pr->send_eq->attr.ist1, i);
>> +    }
>> +    return ret;
>> +failure3:
>> +    for (i = 0; i < port->num_def_qps; i++)
>> +            ibmebus_free_irq(NULL, port->port_res[i].recv_eq->attr.ist1,
>> +                             &port->port_res[i]);
>
> Compare with:
>               u32 ist = port->port_res[k].recv_eq->attr.ist1;
>               ibmebus_free_irq(NULL, ist, &port->port_res[k]);
>
> It was the first loop above. :o/

Agreed, that's slightly prettier.

>> +    /* send */
>> +    for (i = 0; i < port->num_def_qps + port->num_add_tx_qps; i++) {
>> +            ibmebus_free_irq(NULL, port->port_res[i].send_eq->attr.ist1,
>> +                             &port->port_res[i]);
>
> Please add a local 'struct shnortz *foo = port->port_res + i;'

Agreed & done.

>> +    cb0->port_rc = EHEA_BMASK_SET(PXLY_RC_VALID, 1)
>> +                 | EHEA_BMASK_SET(PXLY_RC_IP_CHKSUM, 1)
>> +                 | EHEA_BMASK_SET(PXLY_RC_TCP_UDP_CHKSUM, 1)
>> +                     | EHEA_BMASK_SET(PXLY_RC_VLAN_XTRACT, 1)
>
> Tab/space

Fixed.

>> +static int ehea_init_port_res(struct ehea_port *port, struct ehea_port_res *pr,
>> +                          struct port_res_cfg *pr_cfg, int queue_token)
>> +{
>> +    struct ehea_adapter *adapter = port->adapter;
>> +    struct ehea_qp_init_attr *init_attr = NULL;
>
> Useless initialization.

init_attr must be initialized as there are goto statements which may
pass the init_attr = kzalloc() statement and jump to ehea_init_port_res_err
where we want to kfree() init_attr without having to care for its state.

>> +    pr->send_eq = ehea_create_eq(adapter, eq_type, EHEA_MAX_ENTRIES_EQ, 0);
>> +    if (!pr->send_eq) {
>> +            ehea_error("create_eq failed (send_eq)");
>> +            ret = -EIO;
>> +            goto ehea_init_port_res_err;
>
> Should factor 'ret = -EIO' before the sequence.

Ok, done.

>> +    if (!ret) {
>> +            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->rq1_skba.len; i++)
>> +                    if (pr->rq1_skba.arr[i])
>> +                            dev_kfree_skb(pr->rq1_skba.arr[i]);
>> +
>> +            for (i = 0; i < pr->rq2_skba.len; i++)
>> +                    if (pr->rq2_skba.arr[i])
>> +                            dev_kfree_skb(pr->rq2_skba.arr[i]);
>> +
>> +            for (i = 0; i < pr->rq3_skba.len; i++)
>> +                    if (pr->rq3_skba.arr[i])
>> +                            dev_kfree_skb(pr->rq3_skba.arr[i]);
>> +
>> +            for (i = 0; i < pr->sq_skba.len; i++)
>> +                    if (pr->sq_skba.arr[i])
>> +                            dev_kfree_skb(pr->sq_skba.arr[i]);
>
> Feels like a 0..4 loop is missing above.

The send queue and the receive queues are not related to eachother
in any way. Thus using a joint array for them isn't appropriate.

>> +static int ehea_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +    struct ehea_port *port = netdev_priv(dev);
>> +    struct ehea_port_res *pr;
>> +    struct ehea_swqe *swqe;
>> +    unsigned long flags;
>> +    u32 lkey;
>> +    int swqe_index;
>> +
>> +    pr = &port->port_res[0];
>
> Initialization and declaration can happen at the same time.

Agreed.

>> +    if (unlikely(atomic_read(&pr->swqe_avail) <= 1)) {
>> +            spin_lock_irqsave(&pr->netif_queue, flags);
>> +            if (unlikely(atomic_read(&pr->swqe_avail) <= 1)) {
>> +                    netif_stop_queue(dev);
>> +                    pr->queue_stopped = 1;
>> +                    spin_unlock_irqrestore(&pr->netif_queue, flags);
>> +                    return NETDEV_TX_BUSY;
>
> 1 - this is considered a severe bug. You should stop queueing before it
>     happens.
> 2 - don't mix spinlocked sections and stealth return.

Fixed.

>> +port_res_setup_failed:
>> +    for(k = 0; k < i; k++) {
>> +            ehea_clean_port_res(port, &port->port_res[k]);
>
> Useless k ?

No, i is used as terminating condition and may not be reused as iterator
in this loop.

>> +int ehea_up(struct net_device *dev)
>> +int ehea_open(struct net_device *dev)
>
> static

Agreed.

>> +{
>> +    int ret;
>> +    struct ehea_port *port = netdev_priv(dev);
>> +
>> +    down(&port->port_lock);
>> +
>> +    if (netif_msg_ifup(port))
>> +            ehea_info("enabling port %s", dev->name);
>> +        ret = ehea_up(dev);
>
> Broken indent.

Fixed.

>> +    dev->open = ehea_open;
>> +    dev->poll = ehea_poll;
>> +    dev->weight = 64;
>> +    dev->stop = ehea_stop;
>> +    dev->hard_start_xmit = ehea_start_xmit;
>> +    dev->get_stats = ehea_get_stats;
>> +    dev->set_multicast_list = ehea_set_multicast_list;
>> +    dev->set_mac_address = ehea_set_mac_addr;
>> +    dev->change_mtu = ehea_change_mtu;
>> +    dev->vlan_rx_register = ehea_vlan_rx_register;
>> +    dev->vlan_rx_add_vid = ehea_vlan_rx_add_vid;
>> +    dev->vlan_rx_kill_vid = ehea_vlan_rx_kill_vid;
>> +    dev->features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_TSO
>> +                  | NETIF_F_HIGHDMA | NETIF_F_HW_CSUM | NETIF_F_HW_VLAN_TX
>> +                  | NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_FILTER
>> +                  | NETIF_F_LLTX;
>> +    dev->tx_timeout = &ehea_tx_watchdog;
>> +    dev->watchdog_timeo = EHEA_WATCH_DOG_TIMEOUT;
>> +
>> +    INIT_WORK(&port->reset_task,
>> +              (void (*)(void *)) ehea_reset_port, dev);
>
> Why not modify ehea_reset_port ?

Done.

>> +    ehea_set_ethtool_ops(dev);
>
> This function does not appear in the current patch.

It appears in [2.6.19 PATCH 4/7] ehea: ethtool interface

>> +int check_module_parm(void)
>
> static

Agreed.

>> +    if ((rq1_entries < EHEA_MIN_ENTRIES_QP)
>> +        || (rq1_entries > EHEA_MAX_ENTRIES_RQ1)) {
>
> || is misplaced.

Changed at all occurences.

>> +static struct of_device_id ehea_device_table[] = {
>> +    {
>> +     .name = "lhea",
>> +     .compatible = "IBM,lhea",
>> +     },
>
> Indent seems strange.

Fixed.

>> +    printk("IBM eHEA ethernet device driver (Release %s)\n", DRV_VERSION);
>
> Missing KERN_XYZ

KERN_INFO set.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/