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

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 9 Aug 2021 12:03:36 +0000 Joel Stanley wrote:
> On Fri, 6 Aug 2021 at 23:10, Jakub Kicinski <kuba@kernel.org> wrote:
> > > +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.
 
> > > +     if (ret == -ETIMEDOUT) {
> > > +             netdev_err(netdev, "LITEETH_READER_READY timed out\n");  
> >
> > ratelimit this as well, please
> >  
> > > +             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.

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