lkml.org 
[lkml]   [2006]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 3/4] myri10ge - Driver core
    Date
    Am Thursday 18 May 2006 00:06 schrieb Brice Goglin:

    > +static char *myri10ge_fw_name = NULL;
    > +static char *myri10ge_fw_unaligned = "myri10ge_ethp_z8e.dat";
    > +static char *myri10ge_fw_aligned = "myri10ge_eth_z8e.dat";
    > +static int myri10ge_ecrc_enable = 1;
    > +static int myri10ge_max_intr_slots = 1024;
    > +static int myri10ge_small_bytes = -1; /* -1 == auto */
    > +static int myri10ge_msi = 1; /* enable msi by default */
    > +static int myri10ge_intr_coal_delay = 25;
    > +static int myri10ge_flow_control = 1;
    > +static int myri10ge_deassert_wait = 1;
    > +static int myri10ge_force_firmware = 0;
    > +static int myri10ge_skb_cross_4k = 0;
    > +static int myri10ge_initial_mtu = MYRI10GE_MAX_ETHER_MTU - ETH_HLEN;
    > +static int myri10ge_napi_weight = 64;
    > +static int myri10ge_watchdog_timeout = 1;
    > +static int myri10ge_max_irq_loops = 1048576;
    > +
    > +module_param(myri10ge_fw_name, charp, S_IRUGO | S_IWUSR);
    > +MODULE_PARM_DESC(myri10ge_fw_name, "Firmware image name\n");
    > +module_param(myri10ge_max_intr_slots, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_max_intr_slots, "Interrupt queue slots\n");
    > +module_param(myri10ge_small_bytes, int, S_IRUGO | S_IWUSR);
    > +MODULE_PARM_DESC(myri10ge_small_bytes, "Threshold of small packets\n");
    > +module_param(myri10ge_msi, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_msi, "Enable Message Signalled Interrupts\n");
    > +module_param(myri10ge_intr_coal_delay, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_intr_coal_delay, "Interrupt coalescing
    > delay\n"); +module_param(myri10ge_ecrc_enable, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_ecrc_enable, "Enable Extended CRC on PCI-E\n");
    > +module_param(myri10ge_flow_control, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_flow_control, "Pause parameter\n");
    > +module_param(myri10ge_deassert_wait, int, S_IRUGO | S_IWUSR);
    > +MODULE_PARM_DESC(myri10ge_deassert_wait,
    > + "Wait when deasserting legacy interrupts\n");
    > +module_param(myri10ge_force_firmware, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_force_firmware,
    > + "Force firmware to assume aligned completions\n");
    > +module_param(myri10ge_skb_cross_4k, int, S_IRUGO | S_IWUSR);
    > +MODULE_PARM_DESC(myri10ge_skb_cross_4k,
    > + "Can a small skb cross a 4KB boundary?\n");
    > +module_param(myri10ge_initial_mtu, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_initial_mtu, "Initial MTU\n");
    > +module_param(myri10ge_napi_weight, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_napi_weight, "Set NAPI weight\n");
    > +module_param(myri10ge_watchdog_timeout, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_watchdog_timeout, "Set watchdog timeout\n");
    > +module_param(myri10ge_max_irq_loops, int, S_IRUGO);
    > +MODULE_PARM_DESC(myri10ge_max_irq_loops,
    > + "Set stuck legacy IRQ detection threshold\n");

    How about writing the module_param() and MODULE_PARM_DESC() calls
    directly after each declaration? That would make it clearer
    that they are all parameters.

    > + response->result = 0xffffffff;

    0xffffffff appears throughout your code as a return value. maybe
    use a named constant for it?

    > + for (sleep_total = 0;
    > + sleep_total < (15 * 1000) && response->result == 0xffffffff;
    > + sleep_total += 10) {
    > + udelay(10);
    > + }

    udelay does not sleep. If you want to sleep, use msleep instead.

    > +
    > + myri10ge_pio_copy((void __iomem *)submit, &buf, sizeof(buf));
    > + for (i = 0; *confirm != 0xffffffff && i < 20; i++)
    > + udelay(1000);
    > + if (*confirm != 0xffffffff) {
    > + dev_err(&mgp->pdev->dev, "dummy rdma %s failed\n",
    > + (enable ? "enable" : "disable"));
    > + }
    > +}

    Can you use msleep here instead of udelay?

    > +static int myri10ge_load_firmware(struct myri10ge_priv *mgp)
    > +{
    > + volatile u32 *confirm;
    > + volatile char __iomem *submit;

    The __iomem variable need not be volatile.

    > + myri10ge_pio_copy((void __iomem *)submit, &buf, sizeof(buf));
    > + mb();
    > + udelay(1000);
    > + mb();

    can't you use msleep(1) instead?

    > +static inline void
    > +myri10ge_submit_8rx(struct mcp_kreq_ether_recv __iomem * dst,
    > + struct mcp_kreq_ether_recv *src)
    > +{
    > + u32 low;
    > +
    > + low = src->addr_low;
    > + src->addr_low = DMA_32BIT_MASK;
    > + myri10ge_pio_copy(dst, src, 8 * sizeof(*src));
    > + mb();
    > + src->addr_low = low;
    > + *(u32 __force *) & dst->addr_low = src->addr_low;
    > + mb();
    > +}

    The __force dereference seems fishy.

    > + if (unlikely(((end >> 12) != (data >> 12)) && (data & 4095UL))) {
    > + printk
    > + ("myri10ge_alloc_small: small skb crossed 4KB boundary\n");

    Printk level is missing.


    > +static int myri10ge_open(struct net_device *dev)

    This function is too long to read easily.

    > + /* allocate the host shadow rings */
    > +
    > + bytes = 8 + (MYRI10GE_MCP_ETHER_MAX_SEND_DESC_TSO + 4)
    > + * sizeof(*mgp->tx.req_list);
    > + mgp->tx.req_bytes = kmalloc(bytes, GFP_KERNEL);
    > + if (mgp->tx.req_bytes == NULL)
    > + goto abort_with_nothing;
    > + memset(mgp->tx.req_bytes, 0, bytes);
    > +
    > + /* ensure req_list entries are aligned to 8 bytes */
    > + mgp->tx.req_list = (struct mcp_kreq_ether_send *)
    > + ALIGN((unsigned long)mgp->tx.req_bytes, 8);
    > +
    > + bytes = rx_ring_entries * sizeof(*mgp->rx_small.shadow);
    > + mgp->rx_small.shadow = kmalloc(bytes, GFP_KERNEL);
    > + if (mgp->rx_small.shadow == NULL)
    > + goto abort_with_tx_req_bytes;
    > + memset(mgp->rx_small.shadow, 0, bytes);
    > +
    > + bytes = rx_ring_entries * sizeof(*mgp->rx_big.shadow);
    > + mgp->rx_big.shadow = kmalloc(bytes, GFP_KERNEL);
    > + if (mgp->rx_big.shadow == NULL)
    > + goto abort_with_rx_small_shadow;
    > + memset(mgp->rx_big.shadow, 0, bytes);
    > +
    > + /* allocate the host info rings */
    > +
    > + bytes = tx_ring_entries * sizeof(*mgp->tx.info);
    > + mgp->tx.info = kmalloc(bytes, GFP_KERNEL);
    > + if (mgp->tx.info == NULL)
    > + goto abort_with_rx_big_shadow;
    > + memset(mgp->tx.info, 0, bytes);
    > +
    > + bytes = rx_ring_entries * sizeof(*mgp->rx_small.info);
    > + mgp->rx_small.info = kmalloc(bytes, GFP_KERNEL);
    > + if (mgp->rx_small.info == NULL)
    > + goto abort_with_tx_info;
    > + memset(mgp->rx_small.info, 0, bytes);
    > +
    > + bytes = rx_ring_entries * sizeof(*mgp->rx_big.info);
    > + mgp->rx_big.info = kmalloc(bytes, GFP_KERNEL);
    > + if (mgp->rx_big.info == NULL)
    > + goto abort_with_rx_small_info;
    > + memset(mgp->rx_big.info, 0, bytes);
    > +

    Can you do all these allocations at once? Maybe you can even
    move them into the size passed to alloc_etherdev.

    If you need separate allocations, using kzalloc simplifies
    your code.

    > + /* re-write the last 32-bits with the valid flags */
    > + src->flags = last_flags;
    > + src_ints = (u32 *) src;
    > + src_ints += 3;
    > + dst_ints = (u32 __iomem *) dst;
    > + dst_ints += 3;
    > + *(u32 __force *) dst_ints = *src_ints;
    > + tx->req += cnt;
    > + mb();
    > +}

    All these casts indicate that you do something wrong here.
    In particular, dereferencing an __iomem pointer should
    not be done in a device driver.

    > + /* Break the SKB or Fragment up into pieces which
    > + * do not cross mgp->tx.boundary */
    > + low = MYRI10GE_LOWPART_TO_U32(bus);
    > + high_swapped = htonl(MYRI10GE_HIGHPART_TO_U32(bus));
    > + while (len) {
    > + u8 flags_next;
    > + int cum_len_next;
    > +
    > + if (unlikely(count == max_segments))
    > + goto abort_linearize;
    > +
    > + boundary = (low + tx->boundary) & ~(tx->boundary - 1);
    > + seglen = boundary - low;
    > + if (seglen > len)
    > + seglen = len;
    > + flags_next = flags & ~MYRI10GE_MCP_ETHER_FLAGS_FIRST;
    > + cum_len_next = cum_len + seglen;
    > +#ifdef NETIF_F_TSO
    > + if (mss) { /* TSO */
    > + (req - rdma_count)->rdma_count = rdma_count + 1;
    > +
    > + if (likely(cum_len >= 0)) { /* payload */
    > + int next_is_first, chop;
    > +
    > + chop = (cum_len_next > mss);
    > + cum_len_next = cum_len_next % mss;
    > + next_is_first = (cum_len_next == 0);
    > + flags |= chop *
    > + MYRI10GE_MCP_ETHER_FLAGS_TSO_CHOP;
    > + flags_next |= next_is_first *
    > + MYRI10GE_MCP_ETHER_FLAGS_FIRST;
    > + rdma_count |= -(chop | next_is_first);
    > + rdma_count += chop & !next_is_first;
    > + } else if (likely(cum_len_next >= 0)) { /* header ends */
    > + int small;
    > +
    > + rdma_count = -1;
    > + cum_len_next = 0;
    > + seglen = -cum_len;
    > + small =
    > + (mss <=
    > + MYRI10GE_MCP_ETHER_SEND_SMALL_SIZE);
    > + flags_next =
    > + MYRI10GE_MCP_ETHER_FLAGS_TSO_PLD |
    > + MYRI10GE_MCP_ETHER_FLAGS_FIRST |
    > + (small *
    > + MYRI10GE_MCP_ETHER_FLAGS_SMALL);
    > + }
    > + }

    100 characters per line are too much, as are six levels of intentation,
    or the number of lines in this function.
    You should try to split into into smaller ones.

    >
    > + if ((new_mtu < 68) || (ETH_HLEN + new_mtu > MYRI10GE_MAX_ETHER_MTU)) {
    > + printk(KERN_ERR "myri10ge: %s: new mtu (%d) is not valid\n",
    > + dev->name, new_mtu);
    > + return -EINVAL;
    > + }
    > + printk("%s: changing mtu from %d to %d\n",
    > + dev->name, dev->mtu, new_mtu);

    You shouldn't use printk as a basic feedback mechanism to user space.
    The return code already contains the information.
    Also the printk misses a message level.

    > + pci_set_power_state(pdev, 0); /* zeros conf space as a side effect */
    > + udelay(5000); /* give card time to respond */
    > + pci_read_config_word(mgp->pdev, PCI_VENDOR_ID, &vendor);

    5000 is a long time for udelay. If you can't convert this to msleep,
    use at least mdelay.

    > + printk(KERN_ERR "myri10ge: %s: device timeout, resetting\n",
    > + mgp->dev->name);
    > + printk("myri10ge: %s: %d %d %d %d %d\n", mgp->dev->name,
    > + mgp->tx.req, mgp->tx.done, mgp->tx.pkt_start,
    > + mgp->tx.pkt_done,
    > + (int)ntohl(mgp->fw_stats->send_done_count));
    > + set_current_state(TASK_UNINTERRUPTIBLE);
    > + schedule_timeout(HZ * 2);
    > + set_current_state(TASK_RUNNING);
    > + printk("myri10ge: %s: %d %d %d %d %d\n", mgp->dev->name,
    > + mgp->tx.req, mgp->tx.done, mgp->tx.pkt_start,
    > + mgp->tx.pkt_done,
    > + (int)ntohl(mgp->fw_stats->send_done_count));

    missing printk levels here.

    instead of schedule_timeout, you probably want to use msleep().

    > + if (status != 0) {
    > + printk(KERN_ERR "myri10ge: %s: failed to load firmware\n",
    > + mgp->dev->name);

    dev_err?

    > + for (i = 0; i < ETH_ALEN; i++) {
    > + netdev->dev_addr[i] = mgp->mac_addr[i];
    > + }

    Don't need the curly braces here.

    > +
    > + printk("myri10ge: %s: %s IRQ %d, tx bndry %d, fw %s, WC %s\n",
    > + netdev->name, (mgp->msi_enabled ? "MSI" : "xPIC"),
    > + pdev->irq, mgp->tx.boundary, mgp->fw_name,
    > + (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
    > +

    missing printk level (KERN_DEBUG?). Could probably use dev_printk.

    > + abort_with_irq:
    > + free_irq(pdev->irq, mgp);
    > + if (mgp->msi_enabled)
    > + pci_disable_msi(pdev);
    > +
    > + abort_with_firmware:
    > + myri10ge_dummy_rdma(mgp, 0);
    > +
    > + abort_with_rx_done:
    > + bytes = myri10ge_max_intr_slots * sizeof(*mgp->rx_done.entry);
    > + pci_free_consistent(pdev, bytes, mgp->rx_done.entry, mgp->rx_done.bus);
    > +
    > + abort_with_ioremap:
    > + iounmap((void __iomem *)mgp->sram);
    > +
    > + abort_with_wc:
    > +#ifdef CONFIG_MTRR
    > + if (mgp->mtrr >= 0)
    > + mtrr_del(mgp->mtrr, mgp->iomem_base, mgp->board_span);
    > +#endif
    > + pci_free_consistent(pdev, sizeof(*mgp->fw_stats),
    > + mgp->fw_stats, mgp->fw_stats_bus);
    > +
    > + abort_with_cmd:
    > + pci_free_consistent(pdev, sizeof(*mgp->cmd), mgp->cmd, mgp->cmd_bus);
    > +
    > + abort_with_netdev:
    > +
    > + free_netdev(netdev);
    > + return status;
    > +}

    Goto labels are conventionally indented all the way
    to the left. Yes, lindent/indent gets this wrong.

    > + iounmap((void __iomem *)mgp->sram);

    unnecessary cast.

    > +
    > +#define MYRI10GE_PCI_VENDOR_MYRICOM 0x14c1
    > +#define MYRI10GE_PCI_DEVICE_Z8E 0x0008

    Shouldn't the vendor ID go to pci_ids.h?

    > +
    > +static __init int myri10ge_init_module(void)
    > +{
    > + printk("%s: Version %s\n", myri10ge_driver.name, MYRI10GE_VERSION_STR);
    > + return pci_register_driver(&myri10ge_driver);
    > +}
    > +
    > +static __exit void myri10ge_cleanup_module(void)
    > +{
    > + pci_unregister_driver(&myri10ge_driver);
    > +}
    > +
    > +module_init(myri10ge_init_module);

    This line should go right under the function it refers to.

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

    \
     
     \ /
      Last update: 2006-05-18 01:11    [W:3.009 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site