Re: [PATCH 1/2] net: macb: fix dma_alloc for rx_buffer
On 2023-11-28 at 17:56 +01, Lucas Stach 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 --- 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 inttx_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-|
Re: [PATCH 1/2] net: macb: fix dma_alloc for rx_buffer
Hello Steffen, On 28.11.23 17:29, Steffen Trumtrar wrote: > 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 > --- > 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 inttx_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; For GEM-type NICs, rx_buffer_size is PKTSIZE (1518 bytes currently), which is not a multiple of the cache line size of the 64 bytes cache line on the ZynqMP's Cortex-A53 the driver is supposed to support. > dma_sync_single_for_cpu(macb->dev, (unsigned long)buffer, > length, > DMA_FROM_DEVICE); This means this could potentially invalidate adjacent buffer contents. > } > @@ -891,8 +908,8 @@ static int macb_probe(struct device *dev) > > macb_init_rx_buffer_size(macb, PKTSIZE); ^ Here's where PKTSIZE comes from. I'd be in favor of changing the global PKTSIZE definition to be a multiple of 64 bytes (or use DMA_ALIGNMENT, but this isn't correctly set for ARM yet. I just sent out a patch for that). Cheers, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/2] net: macb: fix dma_alloc for rx_buffer
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 > --- > 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 inttx_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
[PATCH 1/2] net: macb: fix dma_alloc for rx_buffer
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 --- 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 inttx_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, 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); + 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)); +