RE: [PATCH 2/2] netdev: driver: ethernet: Add TI CPSW driver

Eric Dumazet

-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
Sent: Monday, February 13, 2012 11:45 PM
To: N, Mugunthan V
Cc: netdev@vger.kernel.org; davem@davemloft.net
Subject: Re: [PATCH 2/2] netdev: driver: ethernet: Add TI CPSW driver

Le lundi 13 février 2012 à 11:34 +0530, Mugunthan V N a écrit :

quoted

This patch adds support for TI's CPSW driver.

The three port switch gigabit ethernet subsystem provides ethernet

packet

quoted

communication and can be configured as an ethernet switch. Supports
10/100/1000 Mbps.

Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Sriramakrishnan A G <redacted>
Signed-off-by: Mugunthan V N <redacted>

...

quoted

+
+static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
+				       struct net_device *ndev)
+{
+	struct cpsw_priv *priv = netdev_priv(ndev);
+	int ret;
+
+	ndev->trans_start = jiffies;

	Are you sure trans_start needs to  be updated ?

It holds time of last tx, so I am sure it has to be updated on every tx.

quoted

+
+	ret = skb_padto(skb, CPSW_MIN_PACKET_SIZE);
+	if (unlikely(ret < 0)) {
+		msg(err, tx_err, "packet pad failed");
+		goto fail;

Ouch... if skb_padto() fails, we stop queue forever, and skb is freed
twice...

correct sequence is :

	if (skb_padto(skb, CPSW_MIN_PACKET_SIZE)) {
		priv->stats.tx_dropped++;
		return NETDEV_TX_OK;
	}

By the way, skb_padto() doesnt change skb->len, be warned !

Yes, will update the patch in next version

quoted

+	}
+
+	ret = cpdma_chan_submit(priv->txch, skb, skb->data,
+				skb->len, GFP_KERNEL);

This GFP_KERNEL cannot be right, even if cpdma_chan_submit() ignores its
last argument.

Yes, Good catch. I just reviewed davinci_cpdma code, the argument is not used. I will send a separate patch set to remove this argument as this is already used in another driver "davinci_emcac.c"

quoted

+	if (unlikely(ret != 0)) {
+		msg(err, tx_err, "desc submit failed");
+		goto fail;
+	}
+
+	return NETDEV_TX_OK;
+fail:
+	priv->stats.tx_dropped++;
+	netif_stop_queue(ndev);
+	return NETDEV_TX_BUSY;
+}
+