Message-ID: <CAK8P3a3=xgbvqrSpCK5h96eRH32AA7xnoK2ossvT0-cLFLzmXA@mail.gmail.com>
Date:   Thu, 14 May 2020 18:19:37 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Jonathan Corbet <corbet@....net>, Rob Herring <robh+dt@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Matthias Brugger <matthias.bgg@...il.com>,
        John Crispin <john@...ozen.org>,
        Sean Wang <sean.wang@...iatek.com>,
        Mark Lee <Mark-MC.Lee@...iatek.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Fabien Parent <fparent@...libre.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Edwin Peer <edwin.peer@...adcom.com>,
        DTML <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-mediatek@...ts.infradead.org>,
        Stephane Le Provost <stephane.leprovost@...iatek.com>,
        Pedro Tsai <pedro.tsai@...iatek.com>,
        Andrew Perepech <andrew.perepech@...iatek.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH v3 10/15] net: ethernet: mtk-eth-mac: new driver

On Thu, May 14, 2020 at 10:00 AM Bartosz Golaszewski <brgl@...ev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@...libre.com>
>
> This adds the driver for the MediaTek Ethernet MAC used on the MT8* SoC
> family. For now we only support full-duplex.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>

Looks very nice overall. Just a few things I noticed, and some ideas
that may or may not make sense:

> +/* This is defined to 0 on arm64 in arch/arm64/include/asm/processor.h but
> + * this IP doesn't work without this alignment being equal to 2.
> + */
> +#ifdef NET_IP_ALIGN
> +#undef NET_IP_ALIGN
> +#endif
> +#define NET_IP_ALIGN                           2

Maybe you should just define your own macro instead of replacing
the normal one then?

> +static void mtk_mac_lock(struct mtk_mac_priv *priv)
> +{
> +       spin_lock_irqsave(&priv->lock, priv->lock_flags);
> +}
> +
> +static void mtk_mac_unlock(struct mtk_mac_priv *priv)
> +{
> +       spin_unlock_irqrestore(&priv->lock, priv->lock_flags);
> +}

This looks wrong: you should not have shared 'flags' passed into
spin_lock_irqsave(), and I don't even see a need to use the
irqsave variant of the lock in the first place.

Maybe start by open-coding the lock and remove the wrappers
above.

Then see if you can use a cheaper spin_lock_bh() or plain spin_lock()
instead of irqsave.

Finally, see if this can be done in a lockless way by relying on
appropriate barriers and separating the writers into separate
cache lines. From a brief look at the driver I think it can be done
without too much trouble.

> +static unsigned int mtk_mac_intr_read_and_clear(struct mtk_mac_priv *priv)
> +{
> +       unsigned int val;
> +
> +       regmap_read(priv->regs, MTK_MAC_REG_INT_STS, &val);
> +       regmap_write(priv->regs, MTK_MAC_REG_INT_STS, val);
> +
> +       return val;
> +}

Do you actually need to read the register? That is usually a relatively
expensive operation, so if possible try to use clear the bits when
you don't care which bits were set.

> +/* All processing for TX and RX happens in the napi poll callback. */
> +static irqreturn_t mtk_mac_handle_irq(int irq, void *data)
> +{
> +       struct mtk_mac_priv *priv;
> +       struct net_device *ndev;
> +
> +       ndev = data;
> +       priv = netdev_priv(ndev);
> +
> +       if (netif_running(ndev)) {
> +               mtk_mac_intr_mask_all(priv);
> +               napi_schedule(&priv->napi);
> +       }
> +
> +       return IRQ_HANDLED;


> +static int mtk_mac_netdev_start_xmit(struct sk_buff *skb,
> +                                    struct net_device *ndev)
> +{
> +       struct mtk_mac_priv *priv = netdev_priv(ndev);
> +       struct mtk_mac_ring *ring = &priv->tx_ring;
> +       struct device *dev = mtk_mac_get_dev(priv);
> +       struct mtk_mac_ring_desc_data desc_data;
> +
> +       desc_data.dma_addr = mtk_mac_dma_map_tx(priv, skb);
> +       if (dma_mapping_error(dev, desc_data.dma_addr))
> +               goto err_drop_packet;
> +
> +       desc_data.skb = skb;
> +       desc_data.len = skb->len;
> +
> +       mtk_mac_lock(priv);
> +       mtk_mac_ring_push_head_tx(ring, &desc_data);
> +
> +       if (mtk_mac_ring_full(ring))
> +               netif_stop_queue(ndev);
> +       mtk_mac_unlock(priv);
> +
> +       mtk_mac_dma_resume_tx(priv);
> +
> +       return NETDEV_TX_OK;
> +
> +err_drop_packet:
> +       dev_kfree_skb(skb);
> +       ndev->stats.tx_dropped++;
> +       return NETDEV_TX_BUSY;
> +}

I would always add BQL flow control in new drivers, using
netdev_sent_queue here...

> +static int mtk_mac_tx_complete_one(struct mtk_mac_priv *priv)
> +{
> +       struct mtk_mac_ring *ring = &priv->tx_ring;
> +       struct mtk_mac_ring_desc_data desc_data;
> +       int ret;
> +
> +       ret = mtk_mac_ring_pop_tail(ring, &desc_data);
> +       if (ret)
> +               return ret;
> +
> +       mtk_mac_dma_unmap_tx(priv, &desc_data);
> +       dev_kfree_skb_irq(desc_data.skb);
> +
> +       return 0;
> +}

... and netdev_completed_queue()  here.

> +static void mtk_mac_tx_complete_all(struct mtk_mac_priv *priv)
> +{
> +       struct mtk_mac_ring *ring = &priv->tx_ring;
> +       struct net_device *ndev = priv->ndev;
> +       int ret;
> +
> +       for (;;) {
> +               mtk_mac_lock(priv);
> +
> +               if (!mtk_mac_ring_descs_available(ring)) {
> +                       mtk_mac_unlock(priv);
> +                       break;
> +               }
> +
> +               ret = mtk_mac_tx_complete_one(priv);
> +               if (ret) {
> +                       mtk_mac_unlock(priv);
> +                       break;
> +               }
> +
> +               if (netif_queue_stopped(ndev))
> +                       netif_wake_queue(ndev);
> +
> +               mtk_mac_unlock(priv);
> +       }
> +}

It looks like most of the stuff inside of the loop can be pulled out
and only done once here.

> +static int mtk_mac_poll(struct napi_struct *napi, int budget)
> +{
> +       struct mtk_mac_priv *priv;
> +       unsigned int status;
> +       int received = 0;
> +
> +       priv = container_of(napi, struct mtk_mac_priv, napi);
> +
> +       status = mtk_mac_intr_read_and_clear(priv);
> +
> +       /* Clean up TX */
> +       if (status & MTK_MAC_BIT_INT_STS_TNTC)
> +               mtk_mac_tx_complete_all(priv);
> +
> +       /* Receive up to $budget packets */
> +       if (status & MTK_MAC_BIT_INT_STS_FNRC)
> +               received = mtk_mac_process_rx(priv, budget);
> +
> +       /* One of the counter reached 0x8000000 - update stats and reset all
> +        * counters.
> +        */
> +       if (status & MTK_MAC_REG_INT_STS_MIB_CNT_TH) {
> +               mtk_mac_update_stats(priv);
> +               mtk_mac_reset_counters(priv);
> +       }
> +
> +       if (received < budget)
> +               napi_complete_done(napi, received);
> +
> +       mtk_mac_intr_unmask_all(priv);
> +
> +       return received;
> +}

I think you want to leave (at least some of) the interrupts masked
if your budget is exhausted, to avoid generating unnecessary
irqs.

It may also be faster to not mask/unmask at all but just
clear the interrupts that you have finished processing

      Arnd

Powered by blists - more mailing lists