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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
