Date:   Tue, 12 May 2020 08:35:21 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     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>,
        Arnd Bergmann <arnd@...db.de>,
        Fabien Parent <fparent@...libre.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Edwin Peer <edwin.peer@...adcom.com>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        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 v2 09/14] net: ethernet: mtk-eth-mac: new driver

pon., 11 maj 2020 o 21:24 Florian Fainelli <f.fainelli@...il.com> napisał(a):
>
>
>
> On 5/11/2020 8:07 AM, Bartosz Golaszewski 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>
> > ---
>
> [snip]
>
> > +static int mtk_mac_ring_pop_tail(struct mtk_mac_ring *ring,
> > +                              struct mtk_mac_ring_desc_data *desc_data)
> > +{
> > +     struct mtk_mac_ring_desc *desc = &ring->descs[ring->tail];
> > +     unsigned int status;
> > +
> > +     /* Let the device release the descriptor. */
> > +     dma_rmb();
> > +     status = desc->status;
> > +
> > +     if (!(status & MTK_MAC_DESC_BIT_COWN))
> > +             return -1;
> > +
> > +     desc_data->len = status & MTK_MAC_DESC_MSK_LEN;
> > +     desc_data->flags = status & ~MTK_MAC_DESC_MSK_LEN;
> > +     desc_data->dma_addr = desc->data_ptr;
> > +     desc_data->skb = ring->skbs[ring->tail];
> > +
> > +     desc->data_ptr = 0;
> > +     desc->status = MTK_MAC_DESC_BIT_COWN;
> > +     if (status & MTK_MAC_DESC_BIT_EOR)
> > +             desc->status |= MTK_MAC_DESC_BIT_EOR;
>
> Don't you need a dma_wmb() for the device to observe the new descriptor
> here?
>

HW has released the descriptor (set the COWN bit) and I just clear all
other bits here really. Yeah, I guess it won't hurt to make sure.

> [snip]
>
> > +static void mtk_mac_dma_unmap_tx(struct mtk_mac_priv *priv,
> > +                              struct mtk_mac_ring_desc_data *desc_data)
> > +{
> > +     struct device *dev = mtk_mac_get_dev(priv);
> > +
> > +     return dma_unmap_single(dev, desc_data->dma_addr,
> > +                             desc_data->len, DMA_TO_DEVICE);
>
> If you stored a pointer to the sk_buff you transmitted, then you would
> need an expensive read to the descriptor to determine the address and
> length, and you would also not be at the mercy of the HW putting
> incorrect values there.
>

You mean store the mapped addresses? Yeah I can do that but I'll still
need to read the descriptor memory to verify it was released by HW.

> sp
> > +static void mtk_mac_dma_init(struct mtk_mac_priv *priv)
> > +{
> > +     struct mtk_mac_ring_desc *desc;
> > +     unsigned int val;
> > +     int i;
> > +
> > +     priv->descs_base = (struct mtk_mac_ring_desc *)priv->ring_base;
> > +
> > +     for (i = 0; i < MTK_MAC_NUM_DESCS_TOTAL; i++) {
> > +             desc = &priv->descs_base[i];
> > +
> > +             memset(desc, 0, sizeof(*desc));
> > +             desc->status = MTK_MAC_DESC_BIT_COWN;
> > +             if ((i == MTK_MAC_NUM_TX_DESCS - 1) ||
> > +                 (i == MTK_MAC_NUM_DESCS_TOTAL - 1))
> > +                     desc->status |= MTK_MAC_DESC_BIT_EOR;
> > +     }
> > +
> > +     mtk_mac_ring_init(&priv->tx_ring, priv->descs_base, 0);
> > +     mtk_mac_ring_init(&priv->rx_ring,
> > +                       priv->descs_base + MTK_MAC_NUM_TX_DESCS,
> > +                       MTK_MAC_NUM_RX_DESCS);
> > +
> > +     /* Set DMA pointers. */
> > +     val = (unsigned int)priv->dma_addr;
>
> You would probably add a WARN_ON() or something that catches the upper
> 32-bits of the dma_addr being set, see my comment about the DMA mask
> setting.
>

Can it still happen if I check the return value of dma_set_mask_and_coherent()?

> [snip]
>
> > +static void mtk_mac_tx_complete_all(struct mtk_mac_priv *priv)
> > +{
> > +     struct net_device *ndev = priv_to_netdev(priv);
> > +     struct mtk_mac_ring *ring = &priv->tx_ring;
> > +     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);
> > +     }
>
> Where do you increment the net_device statistics to indicate the bytes
> and packets transmitted?
>

I don't. I use the counters provided by HW for that.

> [snip]
>
> > +     mtk_mac_set_mode_rmii(priv);
> > +
> > +     dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>
> Your code assumes that DMA addresses are not going to be >= 4GB so you
> should be checking this function's return code and abort here otherwise
> your driver will fail in surprisingly difficult ways to debug.

Sure, thanks.

Bart

Powered by blists - more mailing lists