Re: [PATCH 4/6] myri10ge - First half of the driver

Francois Romieu
2006-05-10T23:15:28Z

Brice Goglin <bgoglin@myri.com> :
> [PATCH 4/6] myri10ge - First half of the driver
>
> The first half of the myri10ge driver core.
>
> Signed-off-by: Brice Goglin <brice@myri.com>
> Signed-off-by: Andrew J. Gallatin <gallatin@myri.com>
>
> myri10ge.c | 1483 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 1483 insertions(+)
>
> --- /dev/null 2006-05-09 19:43:19.324446250 +0200
> +++ linux/drivers/net/myri10ge/myri10ge.c 2006-05-09 23:00:55.000000000 +0200
[...]
> +module_param(myri10ge_flow_control, int, S_IRUGO);
> +module_param(myri10ge_deassert_wait, int, S_IRUGO | S_IWUSR);
> +module_param(myri10ge_force_firmware, int, S_IRUGO);
> +module_param(myri10ge_skb_cross_4k, int, S_IRUGO | S_IWUSR);
> +module_param(myri10ge_initial_mtu, int, S_IRUGO);
> +module_param(myri10ge_napi, int, S_IRUGO);
> +module_param(myri10ge_napi_weight, int, S_IRUGO);
> +module_param(myri10ge_watchdog_timeout, int, S_IRUGO);
> +module_param(myri10ge_max_irq_loops, int, S_IRUGO);

MODULE_PARM_DESC() would be nice.

> +
> +#define MYRI10GE_FW_OFFSET 1024*1024
> +#define MYRI10GE_HIGHPART_TO_U32(X) \
> +(sizeof (X) == 8) ? ((uint32_t)((uint64_t)(X) >> 32)) : (0)
> +#define MYRI10GE_LOWPART_TO_U32(X) ((uint32_t)(X))
> +
> +#define myri10ge_pio_copy(to,from,size) __iowrite64_copy(to,from,size/8)
> +
> +int myri10ge_hyper_msi_cap_on(struct pci_dev *pdev)

static int ?

[...]
> +static int
> +myri10ge_send_cmd(struct myri10ge_priv *mgp, uint32_t cmd,
> + myri10ge_cmd_t *data)
> +{
> + mcp_cmd_t *buf;
> + char buf_bytes[sizeof(*buf) + 8];
> + volatile mcp_cmd_response_t *response = mgp->cmd;
> + volatile char __iomem *cmd_addr = mgp->sram + MYRI10GE_MCP_CMD_OFFSET;
> + uint32_t dma_low, dma_high;
> + int sleep_total = 0;
> +
> + /* ensure buf is aligned to 8 bytes */
> + buf = (mcp_cmd_t *) ((unsigned long)(buf_bytes + 7) & ~7UL);
> +
> + buf->data0 = htonl(data->data0);
> + buf->data1 = htonl(data->data1);
> + buf->data2 = htonl(data->data2);
> + buf->cmd = htonl(cmd);
> + dma_low = MYRI10GE_LOWPART_TO_U32(mgp->cmd_bus);
> + dma_high = MYRI10GE_HIGHPART_TO_U32(mgp->cmd_bus);
> +
> + buf->response_addr.low = htonl(dma_low);
> + buf->response_addr.high = htonl(dma_high);
> + spin_lock(&mgp->cmd_lock);
> + response->result = 0xffffffff;
> + mb();
> + myri10ge_pio_copy((void __iomem *) cmd_addr, buf, sizeof (*buf));
> +
> + /* wait up to 2 seconds */

You must not hold a spinlock for up to 2 seconds.

> + for (sleep_total = 0; sleep_total < (2 * 1000); sleep_total += 10) {
> + mb();
> + if (response->result != 0xffffffff) {
> + if (response->result == 0) {
> + data->data0 = ntohl(response->data);
> + spin_unlock(&mgp->cmd_lock);
> + return 0;
> + } else {
> + dev_err(&mgp->pdev->dev,
> + "command %d failed, result = %d\n",
> + cmd, ntohl(response->result));
> + spin_unlock(&mgp->cmd_lock);
> + return -ENXIO;

Return in a middle of a spinlock-intensive function. :o(

> + }
> + }
> + udelay(1000 * 10);
> + }
> + spin_unlock(&mgp->cmd_lock);
> + dev_err(&mgp->pdev->dev, "command %d timed out, result = %d\n",
> + cmd, ntohl(response->result));
> + return -EAGAIN;
> +}
> +
> +
> +/*
> + * The eeprom strings on the lanaiX have the format
> + * SN=x\0
> + * MAC=x:x:x:x:x:x\0
> + * PT:ddd mmm xx xx:xx:xx xx\0
> + * PV:ddd mmm xx xx:xx:xx xx\0
> + */
> +int
> +myri10ge_read_mac_addr(struct myri10ge_priv *mgp)

static int ?

[...]
> +static void
> +myri10ge_dummy_rdma(struct myri10ge_priv *mgp, int enable)
> +{
> + volatile uint32_t *confirm;
> + volatile char __iomem *submit;
> + uint32_t buf[16];
> + uint32_t dma_low, dma_high;
> + int i;
> +
> + /* clear confirmation addr */
> + confirm = (volatile uint32_t *) mgp->cmd;
> + *confirm = 0;
> + mb();
> +
> + /* send a rdma command to the PCIe engine, and wait for the
> + * response in the confirmation address. The firmware should
> + * write a -1 there to indicate it is alive and well
> + */
> + dma_low = MYRI10GE_LOWPART_TO_U32(mgp->cmd_bus);
> + dma_high = MYRI10GE_HIGHPART_TO_U32(mgp->cmd_bus);
> +
> + buf[0] = htonl(dma_high); /* confirm addr MSW */
> + buf[1] = htonl(dma_low); /* confirm addr LSW */
> + buf[2] = htonl(0xffffffff); /* confirm data */
> + buf[3] = htonl(dma_high); /* dummy addr MSW */
> + buf[4] = htonl(dma_low); /* dummy addr LSW */
> + buf[5] = htonl(enable); /* enable? */
> +
> + submit = mgp->sram + 0xfc01c0;
> +
> + myri10ge_pio_copy((void __iomem *) submit, &buf, sizeof (buf));
> + mb();
> + udelay(1000);
> + mb();
> + i = 0;
> + while (*confirm != 0xffffffff && i < 20) {
> + udelay(1000);
> + i++;
> + }

for (i = 0; *confirm != 0xffffffff && i < 20; i++)
udelay(1000);

[...]
> +static int
> +myri10ge_adopt_running_firmware(struct myri10ge_priv *mgp)
> +{
> + mcp_gen_header_t *hdr;
> + struct device *dev = &mgp->pdev->dev;
> + size_t bytes, hdr_offset;
> + int status;
> +
> + /* find running firmware header */
> + hdr_offset = ntohl(__raw_readl(mgp->sram + MCP_HEADER_PTR_OFFSET));
> +
> + if ((hdr_offset & 3) || hdr_offset + sizeof(*hdr) > mgp->sram_size) {
> + dev_err(dev, "Running firmware has bad header offset (%d)\n",
> + (int)hdr_offset);
> + return -EIO;
> + }
> +
> + /* copy header of running firmware from SRAM to host memory to
> + * validate firmware */
> + bytes = sizeof (mcp_gen_header_t);

const size_t bytes = ...

> + hdr = (mcp_gen_header_t *) kmalloc(bytes, GFP_KERNEL);

Useless cast.

[...]
> +static int
> +myri10ge_change_pause(struct myri10ge_priv *mgp, int pause)
> +{
> + myri10ge_cmd_t cmd;
> + int status;
> +
> + if (pause)
> + status = myri10ge_send_cmd(mgp, MYRI10GE_MCP_ENABLE_FLOW_CONTROL, &cmd);
> + else
> + status = myri10ge_send_cmd(mgp, MYRI10GE_MCP_DISABLE_FLOW_CONTROL, &cmd);

ctl = pause ? MYRI10GE_MCP_ENABLE_FLOW_CONTROL :
MYRI10GE_MCP_DISABLE_FLOW_CONTROL;

status = myri10ge_send_cmd(mgp, ctl, ...)

> +
> + if (status) {
> + printk(KERN_ERR "myri10ge: %s: Failed to set flow control mode\n",
> + mgp->dev->name);
> + return -ENXIO;

Why not use the status code returned by myri10ge_send_cmd() ?

[...]
> +static int
> +myri10ge_reset(struct myri10ge_priv *mgp)
> +{
[...]
> + cmd.data0 = MYRI10GE_LOWPART_TO_U32(mgp->rx_done.bus);
> + cmd.data1 = MYRI10GE_HIGHPART_TO_U32(mgp->rx_done.bus);
> + cmd.data2 = len * 0x10001;
> + status |= myri10ge_send_cmd(mgp, MYRI10GE_MCP_DMA_TEST, &cmd);

The status code is not used.

> + mgp->read_write_dma = ((cmd.data0>>16) * len * 2 * 2) /
> + (cmd.data0 & 0xffff);
> +
> + memset(mgp->rx_done.entry, 0, bytes);
> +
> + /* reset mcp/driver shared state back to 0 */
> + mgp->tx.req = 0;
> + mgp->tx.done = 0;
> + mgp->tx.pkt_start = 0;
> + mgp->tx.pkt_done = 0;
> + mgp->rx_big.cnt = 0;
> + mgp->rx_small.cnt = 0;
> + mgp->rx_done.idx = 0;
> + mgp->rx_done.cnt = 0;
> + status = myri10ge_update_mac_address(mgp, mgp->dev->dev_addr);
> + myri10ge_change_promisc(mgp, 0);
> + myri10ge_change_pause(mgp, mgp->pause);
> + return status;
> +}
> +
> +static inline void
> +myri10ge_submit_8rx(mcp_kreq_ether_recv_t __iomem *dst, mcp_kreq_ether_recv_t *src)
> +{
> + uint32_t low;
> +
> + low = src->addr_low;
> + src->addr_low = 0xffffffff;

DMA_32BIT_MASK ?

> + myri10ge_pio_copy(dst, src, 8 * sizeof(*src));
> + mb();
> + src->addr_low = low;
> + *(uint32_t __force *) &dst->addr_low = src->addr_low;
> + mb();
> +}
> +
> +/*
> + * Set of routunes to get a new receive buffer. Any buffer which
> + * crosses a 4KB boundary must start on a 4KB boundary due to PCIe
> + * wdma restrictions. We also try to align any smaller allocation to
> + * at least a 16 byte boundary for efficiency. We assume the linux
> + * memory allocator works by powers of 2, and will not return memory
> + * smaller than 2KB which crosses a 4KB boundary. If it does, we fall
> + * back to allocating 2x as much space as required.
> + */
> +
> +static inline struct sk_buff *
> +myri10ge_alloc_big(int bytes)

It fits on a single line.

> +{
> + struct sk_buff *skb;
> + unsigned long data, roundup;
> +
> + skb = dev_alloc_skb(bytes + 4096 + MYRI10GE_MCP_ETHER_PAD);
> + if (skb == NULL)
> + return NULL;

Imho you will want to work directly with pages shortly.

[...]
> +static irqreturn_t
> +myri10ge_napi_intr(int irq, void *arg, struct pt_regs *regs)
> +{
> + struct myri10ge_priv *mgp = (struct myri10ge_priv *) arg;

Useless cast.

[...]
> +static int
> +myri10ge_set_settings(struct net_device *netdev, struct ethtool_cmd *cmd)
> +{
> + return -EINVAL;
> +}

Useless.

--
Ueimor