Re: [PATCH 1/4] net: mvneta: driver for Marvell Armada 370/XP network unit

[Florian Fainelli <florian@openwrt.org>]

Hello Thomas,

The overall driver looks very nice, my biggest concern with this driver being 
that it does not implement phylib and therefore reimplements a bit of existing 
code. I am not commentin on how to represent this MDIO/PHY devices using 
Device Tree since this has been addressed already.

Once you register a MDIO bus for your interface, please make sure that you 
give it a unique name in the system (<pdev->name>-<pdev->id>).

Other comments inline.

On Tuesday 04 September 2012 15:06:41 Thomas Petazzoni wrote:
[snip]
> +
> +/* Increment txq get counter */
> +static void mvneta_inc_get(struct mvneta_tx_queue *txq)
> +{
> +	txq->txq_get_index++;
> +	if (txq->txq_get_index == txq->size)
> +		txq->txq_get_index = 0;
> +}
> +
> +/* Increment txq put counter */
> +static void mvneta_inc_put(struct mvneta_tx_queue *txq)
> +{
> +	txq->txq_put_index++;
> +	if (txq->txq_put_index == txq->size)
> +		txq->txq_put_index = 0;
> +}

I would make it clear that these helpers operate on the txq, and suffix it with 
_txq.

> +
> +
> +/* Clear all MIB counters */
> +static void mvneta_mib_counters_clear(struct mvneta_port *pp)
> +{
> +	int i;
> +	u32 dummy;
> +
> +	/* Perform dummy reads from MIB counters */
> +	for (i = 0; i < MVNETA_MIB_LATE_COLLISION; i += 4)
> +		dummy = mvreg_read(pp, (MVNETA_MIB_COUNTERS_BASE + i));
> +}
> +
> +/* Read speed, duplex, and flow control from port status register */
> +static int mvneta_link_status(struct mvneta_port *pp,
> +			      struct mvneta_lnk_status *status)
> +{
> +	u32 val;
> +
> +	val = mvreg_read(pp, MVNETA_GMAC_STATUS);
> +
> +	if (val & MVNETA_GMAC_SPEED_1000_MASK)
> +		status->speed = MVNETA_SPEED_1000;
> +	else if (val & MVNETA_GMAC_SPEED_100_MASK)
> +		status->speed = MVNETA_SPEED_100;
> +	else
> +		status->speed = MVNETA_SPEED_10;
> +
> +	if (val & MVNETA_GMAC_LINK_UP_MASK)
> +		status->linkup = 1;
> +	else
> +		status->linkup = 0;
> +
> +	if (val & MVNETA_GMAC_FULL_DUPLEX_MASK)
> +		status->duplex = MVNETA_DUPLEX_FULL;
> +	else
> +		status->duplex = MVNETA_DUPLEX_HALF;
> +
> +	if (val & MVNETA_GMAC_TX_FLOW_CTRL_ACTIVE_MASK)
> +		status->tx_fc = MVNETA_FC_ACTIVE;
> +	else if (val & MVNETA_GMAC_TX_FLOW_CTRL_ENABLE_MASK)
> +		status->tx_fc = MVNETA_FC_ENABLE;
> +	else
> +		status->tx_fc = MVNETA_FC_DISABLE;
> +
> +	if (val & MVNETA_GMAC_RX_FLOW_CTRL_ACTIVE_MASK)
> +		status->rx_fc = MVNETA_FC_ACTIVE;
> +	else if (val & MVNETA_GMAC_RX_FLOW_CTRL_ENABLE_MASK)
> +		status->rx_fc = MVNETA_FC_ENABLE;
> +	else
> +		status->rx_fc = MVNETA_FC_DISABLE;
> +

I would rather see you use a struct phy_device and update its properties 
instead of keeping a local copy of it. This would allow you to have consistent 
reporting through ethtool, I have more comments on this later on as well.


> +static void mvneta_rxq_non_occup_desc_add(struct mvneta_port *pp,
> +					  struct mvneta_rx_queue *rxq,
> +					  int rx_desc)
> +{
> +	u32 val;
> +
> +	/* Only 255 descriptors can be added at once */
> +	while (rx_desc > 0xff) {
> +		val = (0xff << MVNETA_RXQ_ADD_NON_OCCUPIED_OFFS);
> +		mvreg_write(pp, MVNETA_RXQ_STATUS_UPDATE_REG(rxq->id), val);
> +		rx_desc = rx_desc - 0xff;
> +	}

You could probably use a define here for 255/0xff.

[snip]

> +	m_delay = 0;

This does not look like an useful name, count would be better

> +	do {
> +		if (m_delay >= MVNETA_RX_DISABLE_TIMEOUT_MSEC) {
> +			netdev_info(pp->dev,
> +				"TIMEOUT for RX stopped ! rx_queue_cmd: 0x08%x\n",
> +				val);

Please use a different logging level such as netdev_err() or netdev_warn() for 
instance.

> +			break;
> +		}
> +		mdelay(1);
> +		m_delay++;

What about using msleep() instead here?

> +
> +		val = mvreg_read(pp, MVNETA_RXQ_CMD);
> +	} while (val & 0xff);
> +
> +	/* Stop Tx port activity. Check port Tx activity. Issue stop
> +	   command for active channels only  */
> +	val = (mvreg_read(pp, MVNETA_TXQ_CMD)) & MVNETA_TXQ_ENABLE_MASK;
> +
> +	if (val != 0)
> +		mvreg_write(pp, MVNETA_TXQ_CMD,
> +			(val << MVNETA_TXQ_DISABLE_OFFS));
> +
> +	/* Wait for all Tx activity to terminate. */
> +	m_delay = 0;
> +	do {
> +		if (m_delay >= MVNETA_TX_DISABLE_TIMEOUT_MSEC) {
> +			netdev_info(pp->dev,
> +				"TIMEOUT for TX stopped tx_queue_cmd - 0x%08x\n",
> +				val);
> +			break;
> +		}
> +		mdelay(1);
> +		m_delay++;
> +
> +		/* Check TX Command reg that all Txqs are stopped */
> +		val = mvreg_read(pp, MVNETA_TXQ_CMD);

Ditto

> +
> +	} while (val & 0xff);
> +	tx_fifo_empty_mask |= MVNETA_TX_FIFO_EMPTY_MASK;
> +	tx_in_prog_mask    |= MVNETA_TX_IN_PRGRS_MASK;
> +
> +	/* Double check to verify that TX FIFO is empty */
> +	m_delay = 0;
> +	while (1) {
> +		do {
> +			if (m_delay >= MVNETA_TX_FIFO_EMPTY_TIMEOUT) {
> +				netdev_info(pp->dev,
> +					    "TX FIFO empty timeout status=0x08%x, 
empty=%x, in_prog=%x",
> +					    val, tx_fifo_empty_mask,
> +					    tx_in_prog_mask);
> +				break;
> +			}
> +			mdelay(1);
> +			m_delay++;

Ditto

> +
> +			val = mvreg_read(pp, MVNETA_PORT_STATUS);
> +		} while (((val & tx_fifo_empty_mask) != tx_fifo_empty_mask)
> +			 || ((val & tx_in_prog_mask) != 0));
> +
> +		if (m_delay >= MVNETA_TX_FIFO_EMPTY_TIMEOUT)
> +			break;
> +
> +		val = mvreg_read(pp, MVNETA_PORT_STATUS);
> +		if (((val & tx_fifo_empty_mask) == tx_fifo_empty_mask) &&
> +		    ((val & tx_in_prog_mask) == 0))
> +			break;
> +		else
> +			netdev_info(pp->dev, "TX FIFO Empty double check failed. %d 
msec status=0x%x, empty=0x%x, in_prog=0x%x\n",
> +				    m_delay, val, tx_fifo_empty_mask,
> +				    tx_in_prog_mask);
> +	}
> +
> +	udelay(200);
> +}

[snip]

> +
> +/* This method sets defaults to the NETA port:
> + *	Clears interrupt Cause and Mask registers.
> + *	Clears all MAC tables.
> + *	Sets defaults to all registers.
> + *	Resets RX and TX descriptor rings.
> + *	Resets PHY.
> + * This method can be called after mvneta_port_down() to return the port
> + *	settings to defaults.
> + */

Please use standard kernel-doc style comments.

[snip]

> +/* Read the Link Up bit (LinkUp) in port MAC control register */
> +static int mvneta_link_is_up(struct mvneta_port *pp)
> +{
> +	u32 val;
> +	val = mvreg_read(pp, MVNETA_GMAC_STATUS);
> +	if (val & MVNETA_GMAC_LINK_UP_MASK)
> +		return 1;

	return mvreg_read(pp, MVNETA_GMAC_STATUS) & MVNETA_GMAC_LINK_UP_MASK;
and make it static inline.

> +
> +	return 0;
> +}
> +
> +/* Get phy address */
> +static int mvneta_phy_addr_get(struct mvneta_port *pp)
> +{
> +	unsigned int val;
> +
> +	val = mvreg_read(pp, MVNETA_PHY_ADDR);
> +	val &= 0x1f;

Use PHY_MAX_ADDR - 1 instead here.

> +	return val;
> +}
> +

[snip]

> +/* Display status (link, duplex, speed) of the port */
> +void mvneta_link_status_print(struct mvneta_port *pp)
> +{
> +	struct mvneta_lnk_status link;
> +	char *speedstr, *duplexstr;
> +
> +	mvneta_link_status(pp, &link);
> +
> +	if (link.linkup) {
> +		if (link.speed == MVNETA_SPEED_1000)
> +			speedstr = "1 Gbps";
> +		else if (link.speed == MVNETA_SPEED_100)
> +			speedstr = "100 Mbps";
> +		else
> +			speedstr = "10 Mbps";
> +
> +		if (link.duplex == MVNETA_DUPLEX_FULL)
> +			duplexstr = "full";
> +		else
> +			duplexstr = "half";
> +
> +		netdev_info(pp->dev,
> +			    "link up, %s duplex, speed %s\n",
> +			    duplexstr, speedstr);
> +	} else
> +		netdev_info(pp->dev, "link down\n");
> +}

You should rather define a phylib adjust_link callback to do this. Otherwise 
please reduce the indentation by handling the case when the link is down first.

> +
> +/* Display more error info */
> +static void mvneta_rx_error(struct mvneta_port *pp,
> +			    struct mvneta_rx_desc *rx_desc)
> +{
> +	u32 status = rx_desc->status;
> +
> +	if (pp->dev)
> +		pp->dev->stats.rx_errors++;

Please do this outside of this function and just let it print the error.

> +
> +	if ((status & MVNETA_RXD_FIRST_LAST_DESC_MASK)
> +	    != MVNETA_RXD_FIRST_LAST_DESC_MASK) {
> +		netdev_err(pp->dev,
> +			   "bad rx status %08x (buffer oversize), size=%d\n",
> +			   rx_desc->status, rx_desc->data_size);
> +		return;
> +	}

[snip]

> +
> +/* Refill processing */
> +static int mvneta_rx_refill(struct mvneta_port *pp,
> +			    struct mvneta_rx_desc *rx_desc)
> +
> +{
> +	unsigned long phys_addr;
> +	struct sk_buff *skb;
> +
> +	skb = netdev_alloc_skb(pp->dev, pp->pkt_size);
> +	if (!skb) {
> +		mvneta_add_cleanup_timer(pp);
> +		return 1;
> +	}
> +
> +	phys_addr = dma_map_single(pp->dev->dev.parent, skb->head,
> +				   MVNETA_RX_BUF_SIZE(pp->pkt_size),
> +				   DMA_FROM_DEVICE);

Check that your phys_addr cookie has been successfully mapped using 
dma_mapping_error().

[snip]

> +/* Main tx processing */
> +static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +
> +	int frags = 0;
> +	int res = NETDEV_TX_OK;
> +	u32 tx_cmd;
> +	struct mvneta_tx_queue *txq = NULL;
> +	struct mvneta_tx_desc *tx_desc;
> +
> +	if (!test_bit(MVNETA_F_STARTED_BIT, &pp->flags))
> +		goto out;

Is not this equivalent to !netif_running(dev)? At least print some message so 
we know that this is not supposed to happen.

> +
> +	txq = &pp->txqs[mvneta_txq_def];
> +
> +	frags = skb_shinfo(skb)->nr_frags + 1;
> +
> +	tx_desc = mvneta_tx_desc_get(pp, txq, frags);
> +	if (tx_desc == NULL) {
> +		frags = 0;
> +		dev->stats.tx_dropped++;
> +		res = NETDEV_TX_BUSY;
> +		goto out;
> +	}


> +
> +	tx_cmd = mvneta_skb_tx_csum(pp, skb);
> +
> +	tx_desc->data_size = skb_headlen(skb);
> +
> +	tx_desc->buf_phys_addr = dma_map_single(dev->dev.parent, skb->data,
> +						tx_desc->data_size,
> +						DMA_TO_DEVICE);

Please check this dma_map_single() return value too.

[snip]

> +static int mvneta_addr_crc(unsigned char *addr)
> +{
> +	int crc = 0;
> +	int i;
> +
> +	for (i = 0; i < 6; i++) {

ETH_ALEN instead of 6 to make it clear it operates on addresses.

> +		int j;
> +
> +		crc = (crc ^ addr[i]) << 8;
> +		for (j = 7; j >= 0; j--) {
> +			if (crc & (0x100 << j))
> +				crc ^= 0x107 << j;
> +		}
> +	}
> +
> +	return crc;
> +}

[snip]

> +
> +/* Interrupt handling - the callback for request_irq() */
> +static irqreturn_t mvneta_isr(int irq, void *dev_id)
> +{
> +	struct mvneta_port *pp = (struct mvneta_port *)dev_id;
> +
> +	/* Mask all interrupts */
> +	mvreg_write(pp, MVNETA_INTR_NEW_MASK, 0);
> +
> +	/* Verify that the device not already on the polling list */
> +	if (napi_schedule_prep(&pp->napi))
> +		__napi_schedule(&pp->napi);

Does not the hardware generate interrupts for tx completion, PHY interrupts 
etc ...?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* Handle link event */
> +static void mvneta_link_event(struct mvneta_port *pp)
> +{
> +	struct net_device *dev = pp->dev;
> +
> +	/* Check Link status on ethernet port */
> +
> +	if (mvneta_link_is_up(pp)) {
> +		mvneta_port_up(pp);
> +		set_bit(MVNETA_F_LINK_UP_BIT, &pp->flags);
> +
> +		if (dev) {
> +			netif_carrier_on(dev);
> +			netif_tx_wake_all_queues(dev);
> +		}
> +	} else {
> +		if (dev) {
> +			netif_carrier_off(dev);
> +			netif_tx_stop_all_queues(dev);
> +		}
> +		mvneta_port_down(pp);
> +		clear_bit(MVNETA_F_LINK_UP_BIT, &pp->flags);
> +	}
> +
> +	mvneta_link_status_print(pp);

Again, this is taken care of by phylib nicely, and does not require you to 
have this F_LINK_UP_BIT.

[snip]

> +
> +/* Handle rxq fill: allocates rxq skbs; called when initializing a port */
> +static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue 
*rxq,
> +			   int num)
> +{
> +	int i;
> +	struct sk_buff *skb;
> +	struct mvneta_rx_desc *rx_desc;
> +	unsigned long phys_addr;
> +	struct net_device *dev = pp->dev;
> +
> +	for (i = 0; i < num; i++) {
> +		skb = dev_alloc_skb(pp->pkt_size);
> +		if (!skb) {
> +			netdev_err(pp->dev, "%s:rxq %d, %d of %d buffs filled\n",
> +				   __func__, rxq->id, i, num);
> +			break;
> +		}
> +
> +		rx_desc = rxq->descs + i;
> +		memset(rx_desc, 0, sizeof(struct mvneta_rx_desc));
> +		phys_addr = dma_map_single(dev->dev.parent, skb->head,
> +					   MVNETA_RX_BUF_SIZE(pp->pkt_size),
> +					   DMA_FROM_DEVICE);

Here again, check phys_addr.

> +		mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)skb);
> +	}
> +
> +	/* add this num of RX descriptors as non occupied (ready to get pkts) */
> +	mvneta_rxq_non_occup_desc_add(pp, rxq, i);
> +
> +	return i;
> +}
> +
[snip]
> +
> +/* Create a specified RX queue */
> +static int mvneta_rxq_init(struct mvneta_port *pp,
> +			   struct mvneta_rx_queue *rxq)
> +
> +{
> +	rxq->size = pp->rx_ring_size;
> +
> +	/* Allocate DMA descriptors array */
> +	rxq->descs_orig = dma_alloc_coherent(pp->dev->dev.parent,
> +					     MVNETA_RX_TOTAL_DESCS_SIZE(rxq),
> +					     &rxq->descs_phys_orig,
> +					     GFP_KERNEL);
> +	if (rxq->descs_orig == NULL) {

Use dma_mapping_error() instead.

> +		netdev_err(pp->dev, "rxQ=%d: Can't allocate %d bytes for %d RX 
descr\n",
> +			   rxq->id, MVNETA_RX_TOTAL_DESCS_SIZE(rxq), rxq->size);
> +		return -ENOMEM;
> +	}
> +
> +	/* Make sure descriptor address is cache line size aligned  */
> +	rxq->descs = PTR_ALIGN(rxq->descs_orig, MVNETA_CPU_D_CACHE_LINE_SIZE);
> +	rxq->descs_phys = ALIGN(rxq->descs_phys_orig,
> +				MVNETA_CPU_D_CACHE_LINE_SIZE);
> +
> +	rxq->last_desc = rxq->size - 1;

Don't you need some kind of barrier here? I do not know exactly how coherent 
your peripherals and memory are, just wondering.

> +
> +	/* Set Rx descriptors queue starting address */
> +	mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys);
> +	mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
> +
> +	/* Set Offset */
> +	mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
> +
> +	/* Set coalescing pkts and time */
> +	mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
> +	mvneta_rx_time_coal_set(pp, rxq, rxq->time_coal);
> +
> +	/* Fill RXQ with buffers from RX pool */
> +	mvneta_rxq_buf_size_set(pp, rxq, MVNETA_RX_BUF_SIZE(pp->pkt_size));
> +	mvneta_rxq_bm_disable(pp, rxq);
> +	mvneta_rxq_fill(pp, rxq, rxq->size);
> +
> +	return 0;
> +}
> +
[snip]

> +static int mvneta_txq_init(struct mvneta_port *pp,
> +			   struct mvneta_tx_queue *txq)
> +{
> +	txq->size = pp->tx_ring_size;
> +
> +	/* Allocate DMA descriptors array */
> +	txq->descs_orig = dma_alloc_coherent(pp->dev->dev.parent,
> +					     MVNETA_TX_TOTAL_DESCS_SIZE(txq),
> +					     &txq->descs_phys_orig,
> +					     GFP_KERNEL);
> +	if (txq->descs_orig == NULL) {

Use dma_mapping_error().

> +		netdev_err(pp->dev, "txQ=%d: Can't allocate %d bytes for %d TX 
descr\n",
> +			   txq->id, MVNETA_TX_TOTAL_DESCS_SIZE(txq), txq->size);
> +		return -ENOMEM;
> +	}
> +
[snip]

> +/* Fill rx buffers, start Rx/Tx activity, set coalesing,
> +*  clear and unmask interrupt bits
> +*/
> +static int mvneta_start_internals(struct mvneta_port *pp, int mtu)
> +{
> +	int err = 0;
> +
> +	pp->pkt_size = MVNETA_RX_PKT_SIZE(mtu);
> +	if (test_bit(MVNETA_F_STARTED_BIT, &pp->flags))
> +		return -EINVAL;

You probably mean -EBUSY here instead?

> +
> +	if (mvneta_max_rx_size_set(pp, MVNETA_RX_PKT_SIZE(mtu))) {
> +		netdev_err(pp->dev,
> +			   "%s: can't set maxRxSize=%d mtu=%d\n",
> +			   __func__, MVNETA_RX_PKT_SIZE(mtu), mtu);
> +		return -EINVAL;
> +	}
> +
> +	err = mvneta_setup_rxqs(pp);
> +	if (unlikely(err))
> +		return err;
> +
> +	err = mvneta_setup_txqs(pp);
> +	if (unlikely(err)) {
> +		mvneta_cleanup_rxqs(pp);
> +		return err;
> +	}
> +
> +	mvneta_txq_max_tx_size_set(pp, MVNETA_RX_PKT_SIZE(mtu));
> +
> +	/* start the Rx/Tx activity */
> +	mvneta_port_enable(pp);
> +
> +	set_bit(MVNETA_F_LINK_UP_BIT, &pp->flags);
> +	set_bit(MVNETA_F_STARTED_BIT, &pp->flags);
> +
> +	return 0;
> +}
> +
> +/* Stop port Rx/Tx activity, free skb's from Rx/Tx rings */
> +static int mvneta_stop_internals(struct mvneta_port *pp)
> +{
> +	clear_bit(MVNETA_F_STARTED_BIT, &pp->flags);
> +
> +	/* Stop the port activity */
> +	mvneta_port_disable(pp);
> +
> +	/* Clear all ethernet port interrupts */
> +	mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> +	mvreg_write(pp, MVNETA_INTR_OLD_CAUSE, 0);
> +
> +	/* Mask all interrupts */
> +	mvneta_interrupts_mask(pp);
> +	smp_call_function_many(cpu_online_mask, mvneta_interrupts_mask,
> +			       pp, 1);
> +
> +	/* Reset TX port here. */
> +	mvneta_tx_reset(pp);
> +
> +	mvneta_cleanup_rxqs(pp);
> +	mvneta_cleanup_txqs(pp);
> +
> +	return 0;
> +
> +}
> +
> +/* Start the port, connect to port interrupt line, unmask interrupts  */
> +static int mvneta_start(struct net_device *dev)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +
> +	/* In default link is down */
> +	netif_carrier_off(dev);
> +	netif_tx_stop_all_queues(dev);
> +
> +	/* Fill rx buffers, start Rx/Tx activity, set coalescing */
> +	if (mvneta_start_internals(pp, dev->mtu) != 0) {
> +		netdev_err(dev, "start internals failed\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Enable polling on the port, must be used after netif_poll_disable */
> +	napi_enable(&pp->napi);
> +
> +	if (pp->flags & MVNETA_F_LINK_UP) {
> +		netif_carrier_on(dev);
> +		netif_tx_wake_all_queues(dev);
> +	} else {
> +		netdev_info(dev, "%s: NOT MVNETA_F_LINK_UP\n", __func__);
> +	}

Remove this message as well.

> +
> +	/* Connect to port interrupt line */
> +	if (request_irq(dev->irq, mvneta_isr, (IRQF_DISABLED), "mv_eth", pp)) {
> +		netdev_err(dev, "cannot request irq %d\n", dev->irq);
> +		napi_disable(&pp->napi);
> +		goto error;
> +	}

You should probably request the interrupt prior to calling napi_enable()

> +
> +	/* Unmask interrupts */
> +	mvneta_interrupts_unmask(pp);
> +	smp_call_function_many(cpu_online_mask,
> +			       mvneta_interrupts_unmask,
> +			       pp, 1);
> +
> +	netdev_info(dev, "started\n");

Remove this please.

> +	return 0;
> +
> +error:
> +	netdev_err(dev, "start failed\n");
> +	mvneta_cleanup_rxqs(pp);
> +	mvneta_cleanup_txqs(pp);
> +
> +	return -ENODEV;
> +}
> +

> +	if (dev->irq != 0)
> +		free_irq(dev->irq, pp);

This looks superfluous, you refuse to bring up the interface if the interrupt 
requesting fails.

> +
> +	netdev_info(dev, "stopped\n");
> +
> +	return 0;
> +}
> +
> +
> +/* tx timeout callback - display a message and stop/start the network 
device */
> +static void mvneta_tx_timeout(struct net_device *dev)
> +{
> +	netdev_info(dev, "tx timeout\n");
> +	if (netif_running(dev)) {
> +		mvneta_stop(dev);
> +		mvneta_start(dev);
> +	}

You should never end-up with the case where the interface is not running and 
you face a transmit timeout.

[snip]

> +/* Change the device mtu */
> +static int mvneta_change_mtu(struct net_device *dev, int mtu)
> +{
> +	int old_mtu = dev->mtu;
> +
> +	mtu = mvneta_check_mtu_valid(dev, mtu);
> +	if (mtu < 0)
> +		return -EINVAL;
> +
> +	dev->mtu = mtu;
> +
> +	if (!netif_running(dev)) {
> +		netdev_info(dev, "change mtu %d (buffer-size %d) to %d (buffer-size 
%d)\n",
> +			old_mtu, MVNETA_RX_PKT_SIZE(old_mtu),
> +			dev->mtu, MVNETA_RX_PKT_SIZE(dev->mtu));
> +		return 0;

Remove this message.

> +	}
> +
> +	if (mvneta_stop(dev)) {
> +		netdev_err(dev, "stop interface failed\n");
> +		goto error;
> +	}
> +
> +	if (mvneta_start(dev)) {
> +		netdev_err(dev, "start interface failed\n");
> +		goto error;
> +	}

Propagate the returned error codes back to the caller.

> +
> +	netdev_info(dev, "change mtu %d (buffer-size %d) to %d (buffer-size %d)\n",
> +		old_mtu, MVNETA_RX_PKT_SIZE(old_mtu),
> +		dev->mtu, MVNETA_RX_PKT_SIZE(dev->mtu));

Remove this message too.

> +
> +	return 0;
> +
> +error:
> +	netdev_info(dev, "change mtu failed\n");
> +	return -EINVAL;
> +}
> +
> +/* Handle setting mac address (low level) */
> +static int mvneta_set_mac_addr_internals(struct net_device *dev, void 
*addr)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +	u8 *mac = addr + 2;
> +	int i;
> +
> +	/* Remove previous address table entry */
> +	if (mvneta_mac_addr_set(pp, dev->dev_addr, -1) != 0) {
> +		netdev_err(dev, "mvneta_mac_addr_set failed\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set new addr in hw */
> +	if (mvneta_mac_addr_set(pp, mac, mvneta_rxq_def) != 0) {
> +		netdev_err(dev, "mvneta_mac_addr_set failed\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Set addr in the device */
> +	for (i = 0; i < MVNETA_MAC_ADDR_SIZE; i++)
> +		dev->dev_addr[i] = mac[i];

ETH_ALEN.

> +
> +	netdev_info(dev, "mac address changed\n");

Remove this please.

> +
> +	return 0;
> +}
> +
> +/* Handle setting mac address */
> +static int mvneta_set_mac_addr(struct net_device *dev, void *addr)
> +{
> +	if (!netif_running(dev)) {
> +		if (mvneta_set_mac_addr_internals(dev, addr) == -1)
> +			goto error;
> +		return 0;
> +	}

Usually you just check if the interface is running, and if it is return 
something like -EBUSY.

> +
> +	if (mvneta_stop(dev)) {
> +		netdev_err(dev, "stop interface failed\n");
> +		goto error;
> +	}
> +
> +	if (mvneta_set_mac_addr_internals(dev, addr) == -1)
> +		goto error;
> +
> +	if (mvneta_start(dev)) {
> +		netdev_err(dev, "start interface failed\n");
> +		goto error;
> +	}

Propagate error codes here too please.

> +
> +	return 0;
> +
> +error:
> +	netdev_err(dev, "set mac addr failed\n");
> +	return -EINVAL;
> +}
> +
> +/*
> + * Called when a network interface is made active.
> + * Returns 0 on success, -EINVAL or =ENODEV on failure
> + * mvneta_open() is called when a network interface is made
> + * active by the system (IFF_UP). We set the mac address and
> + * invoke mvneta_start() to start the device.
> + */
> +static int mvneta_open(struct net_device *dev)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +	int queue = mvneta_rxq_def;
> +
> +	if (mvneta_mac_addr_set(pp, dev->dev_addr, queue) != 0) {
> +		netdev_err(dev, "mvneta_mac_addr_set failed\n");
> +		return -EINVAL;
> +	}
> +
> +	if (mvneta_start(dev)) {
> +		netdev_err(dev, "start interface failed\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;

Propagate the error code here too.

[snip]

> +static void mvneta_ethtool_get_drvinfo(struct net_device *dev,
> +				    struct ethtool_drvinfo *drvinfo)
> +{
> +	strlcpy(drvinfo->driver, mvneta_driver_name,
> +		sizeof(drvinfo->driver));
> +	strlcpy(drvinfo->version, mvneta_driver_version,
> +		sizeof(drvinfo->version));

You can probably also provide informations about the firmware version, bus_info 
at least.

[snip]

> +/* Device initialization routine */
> +static int __devinit mvneta_probe(struct platform_device *pdev)
> +{
> +	int err = -EINVAL;
> +	struct mvneta_port *pp;
> +	struct net_device *dev;
> +	u32 phy_addr, clk;
> +	int phy_mode;
> +	const char *mac_addr;
> +	const struct mbus_dram_target_info *dram_target_info;
> +	struct device_node *dn = pdev->dev.of_node;
> +
> +	dev = alloc_etherdev_mq(sizeof(struct mvneta_port), 8);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->irq = irq_of_parse_and_map(dn, 0);
> +	if (dev->irq == 0) {
> +		err = -EINVAL;
> +		goto err_irq;
> +	}
> +
> +	if (of_property_read_u32(dn, "phy-addr", &phy_addr) != 0) {
> +		dev_err(&pdev->dev, "could not read phy_addr\n");
> +		err = -ENODEV;
> +		goto err_node;
> +	}
> +
> +	phy_mode = of_get_phy_mode(dn);
> +	if (phy_mode < 0) {
> +		dev_err(&pdev->dev, "wrong phy-mode\n");
> +		err = -EINVAL;
> +		goto err_node;
> +	}
> +
> +	if (of_property_read_u32(dn, "clock-frequency", &clk) != 0) {
> +		dev_err(&pdev->dev, "could not read clock-frequency\n");
> +		err = -EINVAL;
> +		goto err_node;
> +	}
> +
> +	mac_addr = of_get_mac_address(dn);
> +
> +	if (!mac_addr || !is_valid_ether_addr(mac_addr))
> +		eth_hw_addr_random(dev);
> +	else
> +		memcpy(dev->dev_addr, mac_addr, 6);
> +
> +	dev->tx_queue_len = MVNETA_MAX_TXD;
> +	dev->watchdog_timeo = 5 * HZ;
> +	dev->netdev_ops = &mvneta_netdev_ops;
> +
> +	SET_ETHTOOL_OPS(dev, &mvneta_eth_tool_ops);
> +
> +	pp = netdev_priv(dev);
> +
> +	pp->tx_done_timer.function = mvneta_tx_done_timer_callback;
> +	init_timer(&pp->tx_done_timer);
> +	clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
> +	pp->cleanup_timer.function = mvneta_cleanup_timer_callback;
> +	init_timer(&pp->cleanup_timer);
> +	clear_bit(MVNETA_F_CLEANUP_TIMER_BIT, &pp->flags);
> +
> +	pp->weight = MVNETA_RX_POLL_WEIGHT;
> +	pp->clk = clk;

Rename this clk_freq so make it less ambiguous, because this is not a proper 
struct clk pointer.

> +
> +	pp->base = of_iomap(dn, 0);
> +	if (pp->base == NULL) {
> +		err = -ENOMEM;
> +		goto err_node;
> +	}
> +
> +	pp->tx_done_timer.data = (unsigned long)dev;
> +	pp->cleanup_timer.data = (unsigned long)dev;
> +
> +	pp->tx_ring_size = MVNETA_MAX_TXD;
> +	pp->rx_ring_size = MVNETA_MAX_RXD;
> +
> +	pp->dev = dev;
> +
> +	if (mvneta_init(pp, phy_addr)) {
> +		dev_err(&pdev->dev, "can't init eth hal\n");
> +		err = -ENODEV;
> +		goto err_base;
> +	}
> +	mvneta_port_power_up(pp, phy_mode);
> +
> +	dram_target_info = mv_mbus_dram_info();
> +	if (dram_target_info)
> +		mvneta_conf_mbus_windows(pp, dram_target_info);
> +
> +	netif_napi_add(dev, &pp->napi, mvneta_poll, pp->weight);
> +
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	if (register_netdev(dev)) {
> +		dev_err(&pdev->dev, "failed to register\n");
> +		err = ENOMEM;
> +		goto err_base;
> +	}
> +
> +	dev->features = NETIF_F_SG;
> +	dev->hw_features =  NETIF_F_SG;
> +	dev->priv_flags |= IFF_UNICAST_FLT;
> +
> +	if (dev->mtu <= MVNETA_TX_CSUM_MAX_SIZE) {
> +		dev->features |= NETIF_F_IP_CSUM;
> +		dev->hw_features |= NETIF_F_IP_CSUM;
> +	}

At this point, the condition is always true, so just set these features and 
update them when the MTU changes.

> +
> +	dev_info(&pdev->dev, "%s, mac: %pM pp->base=%p\n", dev->name,
> +		 dev->dev_addr, pp->base);
> +
> +	platform_set_drvdata(pdev, pp->dev);
> +
> +	return 0;
> +err_base:
> +	iounmap(pp->base);
> +err_node:
> +	irq_dispose_mapping(dev->irq);
> +err_irq:
> +	free_netdev(dev);
> +	return err;
> +}
> +
> +/* Device removal routine */
> +static int __devexit mvneta_remove(struct platform_device *pdev)
> +{
> +	struct net_device  *dev = platform_get_drvdata(pdev);
> +	struct mvneta_port *pp = netdev_priv(dev);
> +
> +	dev_info(&pdev->dev, "Removing Marvell Ethernet Driver\n");

I would remove this message.