Re: [PATCH 1/2] net: macb: fix dma_alloc for rx_buffer

[Lucas Stach <l.stach@pengutronix.de>]

Am Dienstag, dem 28.11.2023 um 17:29 +0100 schrieb Steffen Trumtrar:
> rx_buffer gets dma_alloc'ed but is never dma_map'ed and therefor not
> flushed before it is initially used.
> 
> Map the rx_buffer when the macb is initialized and unmap it on ether_halt.
> 
> While at it, cleanup the dma_alloc_coherent rx_ring/tx_ring, too.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/net/macb.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 260c1e806a..92f78f7253 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -63,10 +63,13 @@ struct macb_device {
>  	unsigned int		tx_head;
>  
>  	void			*rx_buffer;
> +	dma_addr_t		rx_buffer_phys;
>  	void			*tx_buffer;
>  	void			*rx_packet_buf;
>  	struct macb_dma_desc	*rx_ring;
> +	dma_addr_t		rx_ring_phys;
>  	struct macb_dma_desc	*tx_ring;
> +	dma_addr_t		tx_ring_phys;
>  	struct macb_dma_desc	*gem_q1_descs;
>  
>  	int			rx_buffer_size;
> @@ -181,7 +184,7 @@ static int gem_recv(struct eth_device *edev)
>  		barrier();
>  		status = macb->rx_ring[macb->rx_tail].ctrl;
>  		length = MACB_BFEXT(RX_FRMLEN, status);
> -		buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
> +		buffer = (void *)macb->rx_buffer_phys + macb->rx_buffer_size * macb->rx_tail;
>  		dma_sync_single_for_cpu(macb->dev, (unsigned long)buffer, length,
>  					DMA_FROM_DEVICE);
>  		net_receive(edev, buffer, length);
> @@ -221,7 +224,7 @@ static int macb_recv(struct eth_device *edev)
>  		}
>  
>  		if (status & MACB_BIT(RX_EOF)) {
> -			buffer = macb->rx_buffer + macb->rx_buffer_size * macb->rx_tail;
> +			buffer = (void *)macb->rx_buffer_phys + macb->rx_buffer_size * macb->rx_tail;
>  			length = MACB_BFEXT(RX_FRMLEN, status);
>  			if (wrapped) {
>  				unsigned int headlen, taillen;
> @@ -232,12 +235,12 @@ static int macb_recv(struct eth_device *edev)
>  				dma_sync_single_for_cpu(macb->dev, (unsigned long)buffer,
>  							headlen, DMA_FROM_DEVICE);
>  				memcpy(macb->rx_packet_buf, buffer, headlen);
> -				dma_sync_single_for_cpu(macb->dev, (unsigned long)macb->rx_buffer,
> +				dma_sync_single_for_cpu(macb->dev, (unsigned long)macb->rx_buffer_phys,

You can drop all those (unsigned long) casts in calls to
dma_sync_single, now that you are passing a argument of the proper
dma_addr_t type.

>  							taillen, DMA_FROM_DEVICE);
>  				memcpy(macb->rx_packet_buf + headlen, macb->rx_buffer, taillen);
>  				dma_sync_single_for_device(macb->dev, (unsigned long)buffer,
>  							headlen, DMA_FROM_DEVICE);
> -				dma_sync_single_for_device(macb->dev, (unsigned long)macb->rx_buffer,
> +				dma_sync_single_for_device(macb->dev, (unsigned long)macb->rx_buffer_phys,
>  							taillen, DMA_FROM_DEVICE);
>  				net_receive(edev, macb->rx_packet_buf, length);
>  			} else {
> @@ -377,7 +380,7 @@ static int gmac_init_dummy_tx_queues(struct macb_device *macb)
>  	return 0;
>  }
>  
> -static void macb_init(struct macb_device *macb)
> +static int macb_init(struct macb_device *macb)
>  {
>  	unsigned long paddr, val = 0;
>  	int i;
> @@ -386,6 +389,11 @@ static void macb_init(struct macb_device *macb)
>  	 * macb_halt should have been called at some point before now,
>  	 * so we'll assume the controller is idle.
>  	 */
> +	macb->rx_buffer_phys = dma_map_single(macb->dev, macb->rx_buffer,
> +					      macb->rx_buffer_size * macb->rx_ring_size,
> +					      DMA_TO_DEVICE);

The RX buffer is used to hold data written by the device, so it must be
mapped with DMA_FROM_DEVICE.

Regards,
Lucas

> +	if (dma_mapping_error(macb->dev, macb->rx_buffer_phys))
> +		return -EFAULT;
>  
>  	/* initialize DMA descriptors */
>  	paddr = (ulong)macb->rx_buffer;
> @@ -442,6 +450,7 @@ static void macb_init(struct macb_device *macb)
>  
>  	macb_or_gem_writel(macb, USRIO, val);
>  
> +	return 0;
>  }
>  
>  static void macb_halt(struct eth_device *edev)
> @@ -460,6 +469,13 @@ static void macb_halt(struct eth_device *edev)
>  
>  	/* Disable TX and RX, and clear statistics */
>  	macb_writel(macb, NCR, MACB_BIT(CLRSTAT));
> +
> +	dma_unmap_single(macb->dev, macb->rx_buffer_phys,
> +			 macb->rx_buffer_size * macb->rx_ring_size,
> +			 DMA_TO_DEVICE);
> +	free(macb->rx_buffer);
> +	dma_free_coherent((void *)macb->rx_ring, macb->rx_ring_phys, RX_RING_BYTES(macb));
> +	dma_free_coherent((void *)macb->tx_ring, macb->tx_ring_phys, TX_RING_BYTES);
>  }
>  
>  static int macb_phy_read(struct mii_bus *bus, int addr, int reg)
> @@ -780,6 +796,7 @@ static int macb_probe(struct device *dev)
>  	const char *pclk_name, *hclk_name;
>  	const struct macb_config *config = NULL;
>  	u32 ncfgr;
> +	int ret;
>  
>  	macb = xzalloc(sizeof(*macb));
>  	edev = &macb->netdev;
> @@ -877,7 +894,7 @@ static int macb_probe(struct device *dev)
>  		clk_enable(macb->rxclk);
>  
>  	if (config) {
> -		int ret = config->txclk_init(dev, &macb->txclk);
> +		ret = config->txclk_init(dev, &macb->txclk);
>  		if (ret)
>  			return ret;
>  	}
> @@ -891,8 +908,8 @@ static int macb_probe(struct device *dev)
>  
>  	macb_init_rx_buffer_size(macb, PKTSIZE);
>  	macb->rx_buffer = dma_alloc(macb->rx_buffer_size * macb->rx_ring_size);
> -	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb), DMA_ADDRESS_BROKEN);
> -	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES, DMA_ADDRESS_BROKEN);
> +	macb->rx_ring = dma_alloc_coherent(RX_RING_BYTES(macb), &macb->rx_ring_phys);
> +	macb->tx_ring = dma_alloc_coherent(TX_RING_BYTES, &macb->tx_ring_phys);
>  
>  	if (macb->is_gem)
>  		macb->gem_q1_descs = dma_alloc_coherent(GEM_Q1_DESC_BYTES,
> @@ -907,7 +924,9 @@ static int macb_probe(struct device *dev)
>  	ncfgr |= macb_dbw(macb);
>  	macb_writel(macb, NCFGR, ncfgr);
>  
> -	macb_init(macb);
> +	ret = macb_init(macb);
> +	if (ret)
> +		return ret;
>  
>  	mdiobus_register(&macb->miibus);
>  	eth_register(edev);
>