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

On Mon, 9 Aug 2021 12:03:36 +0000 Joel Stanley wrote:

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

quoted

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.

In that case it's probably best to stop the Tx queue in the xmit routine
once all the lots are used, and restart it from the interrupt. I was
guessing maybe the IRQ is not always there, but that doesn't seem to be
the case.
 

quoted

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?

Either that or allocate a small array (num_tx_slots) to save the
lengths for IRQ routine to use.

quoted

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;
+}