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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210809091621.40c91f01@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 9 Aug 2021 09:16:21 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Joel Stanley <joel@....id.au>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Stafford Horne <shorne@...il.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Anton Blanchard <anton@...abs.org>,
        Gabriel Somlo <gsomlo@...il.com>, David Shah <dave@....me>,
        Karol Gugala <kgugala@...micro.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        devicetree <devicetree@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: 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 <kuba@...nel.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;
> > > +}