Re: [PATCH v3 1/7] AMSO1100 Low Level Driver.

From: Arjan van de Ven
Date: Tue Jun 20 2006 - 16:42:44 EST


On Tue, 2006-06-20 at 15:30 -0500, Steve Wise wrote:

> +/*
> + * Allocate TX ring elements and chain them together.
> + * One-to-one association of adapter descriptors with ring elements.
> + */
> +static int c2_tx_ring_alloc(struct c2_ring *tx_ring, void *vaddr,
> + dma_addr_t base, void __iomem * mmio_txp_ring)
> +{
> + struct c2_tx_desc *tx_desc;
> + struct c2_txp_desc __iomem *txp_desc;
> + struct c2_element *elem;
> + int i;
> +
> + tx_ring->start = kmalloc(sizeof(*elem) * tx_ring->count, GFP_KERNEL);

I would think this needs a dma_alloc_coherent() rather than a kmalloc...


> +
> +/* Free all buffers in RX ring, assumes receiver stopped */
> +static void c2_rx_clean(struct c2_port *c2_port)
> +{
> + struct c2_dev *c2dev = c2_port->c2dev;
> + struct c2_ring *rx_ring = &c2_port->rx_ring;
> + struct c2_element *elem;
> + struct c2_rx_desc *rx_desc;
> +
> + elem = rx_ring->start;
> + do {
> + rx_desc = elem->ht_desc;
> + rx_desc->len = 0;
> +
> + __raw_writew(0, elem->hw_desc + C2_RXP_STATUS);
> + __raw_writew(0, elem->hw_desc + C2_RXP_COUNT);
> + __raw_writew(0, elem->hw_desc + C2_RXP_LEN);

you seem to be a fan of the __raw_write() functions... any reason why?
__raw_ is not a magic "go faster" prefix....

Also on a related note, have you checked the driver for the needed PCI
posting flushes?

> +
> + /* Disable IRQs by clearing the interrupt mask */
> + writel(1, c2dev->regs + C2_IDIS);
> + writel(0, c2dev->regs + C2_NIMR0);

like here...
> +
> + elem = tx_ring->to_use;
> + elem->skb = skb;
> + elem->mapaddr = mapaddr;
> + elem->maplen = maplen;
> +
> + /* Tell HW to xmit */
> + __raw_writeq(cpu_to_be64(mapaddr), elem->hw_desc + C2_TXP_ADDR);
> + __raw_writew(cpu_to_be16(maplen), elem->hw_desc + C2_TXP_LEN);
> + __raw_writew(cpu_to_be16(TXP_HTXD_READY), elem->hw_desc + C2_TXP_FLAGS);

or here

> +static int c2_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> + int ret = 0;
> +
> + if (new_mtu < ETH_ZLEN || new_mtu > ETH_JUMBO_MTU)
> + return -EINVAL;
> +
> + netdev->mtu = new_mtu;
> +
> + if (netif_running(netdev)) {
> + c2_down(netdev);
> +
> + c2_up(netdev);
> + }

this looks odd...



-
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/