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

[Florian Fainelli <f.fainelli@gmail.com>]

On 5/11/2020 8:07 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.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@baylibre.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