Re: [PATCH 2/2] net: Add driver for LiteX's LiteETH network interface

Thanks for the review Jakub.

On Fri, 6 Aug 2021 at 23:10, Jakub Kicinski [off-list ref] wrote:

quoted

+static int liteeth_start_xmit(struct sk_buff *skb, struct net_device *netdev)
+{
+     struct liteeth *priv = netdev_priv(netdev);
+     void __iomem *txbuffer;
+     int ret;
+     u8 val;
+
+     /* Reject oversize packets */
+     if (unlikely(skb->len > MAX_PKT_SIZE)) {
+             if (net_ratelimit())
+                     netdev_dbg(netdev, "tx packet too big\n");
+             goto drop;
+     }
+
+     txbuffer = priv->tx_base + priv->tx_slot * LITEETH_BUFFER_SIZE;
+     memcpy_toio(txbuffer, skb->data, skb->len);
+     writeb(priv->tx_slot, priv->base + LITEETH_READER_SLOT);
+     writew(skb->len, priv->base + LITEETH_READER_LENGTH);
+
+     ret = readl_poll_timeout_atomic(priv->base + LITEETH_READER_READY, val, val, 5, 1000);

Why the need for poll if there is an interrupt?
Why not stop the Tx queue once you're out of slots and restart
it when the completion interrupt comes?

That makes sense.

In testing I have not been able to hit the LITEETH_READER_READY
not-ready state. I assume it's there to say that the slots are full.

quoted

+     if (ret == -ETIMEDOUT) {
+             netdev_err(netdev, "LITEETH_READER_READY timed out\n");

ratelimit this as well, please

quoted

+             goto drop;
+     }
+
+     writeb(1, priv->base + LITEETH_READER_START);
+
+     netdev->stats.tx_bytes += skb->len;

Please count bytes and packets in the same place

AFAIK we don't know the length when the interrupt comes in, so we need
to count both here in xmit?

quoted

+     priv->tx_slot = (priv->tx_slot + 1) % priv->num_tx_slots;
+     dev_kfree_skb_any(skb);
+     return NETDEV_TX_OK;
+drop:
+     /* Drop the packet */
+     dev_kfree_skb_any(skb);
+     netdev->stats.tx_dropped++;
+
+     return NETDEV_TX_OK;
+}