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

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On 2023-11-28 at 17:56 +01, Lucas Stach <l.stach@pengutronix.de> wrote:

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

Thanks, will drop.

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

Argh, of course :(


Thanks,
Steffen

--
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |