netdev - Re: [PATCH v2 09/14] net: ethernet: mtk-eth-mac: new driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <dab80587-a196-e0ab-ae97-f8e5cc4a71d4@gmail.com>
Date:   Mon, 11 May 2020 12:24:20 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>,
        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>
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, 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



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?

[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.

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.

[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?

[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.
-- 
Florian