[PATCH 0/7] non-coherent Rx buffers in FEC and some small fixes

[PATCH 0/7] non-coherent Rx buffers in FEC and some small fixes

[Andrey Smirnov]

Everyone:

This series was created while working on converting FEC driver to use
non-coherent memory for Rx buffers as a part of addressing feedback
for [1]. Patch 7/7 implements said change while the rest of the
patches are just small improvements (IMHO, of course) that I made
while looking at FEC's source code.

Tested by using TFTP on following boards:

  - NXP i.MX8MQ EVK
  - ZII i.MX6Q RDU2
  - ZII VF610 CFU1

Feedback is welcome!

Thanks,
Andrey Smirnov

[1] http://lists.infradead.org/pipermail/barebox/2018-August/034216.html

Andrey Smirnov (7):
  net: fec_imx: Drop extra indentation level by exiting early
  net: fec_imx: Don't check bd_status & FEC_RBD_ERR twice
  net: fec_imx: Read data_length only once
  net: fec_imx: Drop struct fec_frame
  net: fec_imx: Drop frame_length
  net: fec_imx: Make use of readx_poll_timeout() macros
  net: fec_imx: Do not use DMA coherent memory for Rx buffers

 drivers/net/fec_imx.c | 141 +++++++++++++++++++++---------------------
 1 file changed, 69 insertions(+), 72 deletions(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/7] net: fec_imx: Drop extra indentation level by exiting early

[Andrey Smirnov]

Drop extra indentation level by exiting early which also allows us to
check for bd_status & FEC_RBD_EMPTY instead of !(bd_status &
FEC_RBD_EMPTY).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 53 ++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 94a159e2b..904eba30a 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -568,35 +568,36 @@ static int fec_recv(struct eth_device *dev)
 	 */
 	bd_status = readw(&rbd->status);
 
-	if (!(bd_status & FEC_RBD_EMPTY)) {
-		if ((bd_status & FEC_RBD_LAST) && !(bd_status & FEC_RBD_ERR) &&
-			((readw(&rbd->data_length) - 4) > 14)) {
-
-			if (fec_is_imx28(fec))
-				imx28_fix_endianess_rd(
-					phys_to_virt(readl(&rbd->data_pointer)),
-					(readw(&rbd->data_length) + 3) >> 2);
-
-			/*
-			 * Get buffer address and size
-			 */
-			frame = phys_to_virt(readl(&rbd->data_pointer));
-			frame_length = readw(&rbd->data_length) - 4;
-			net_receive(dev, frame->data, frame_length);
-			len = frame_length;
-		} else {
-			if (bd_status & FEC_RBD_ERR) {
-				dev_warn(&dev->dev, "error frame: 0x%p 0x%08x\n", rbd, bd_status);
-			}
-		}
+	if (bd_status & FEC_RBD_EMPTY)
+		return 0;
+
+	if ((bd_status & FEC_RBD_LAST) && !(bd_status & FEC_RBD_ERR) &&
+	    ((readw(&rbd->data_length) - 4) > 14)) {
+
+		if (fec_is_imx28(fec))
+			imx28_fix_endianess_rd(
+				phys_to_virt(readl(&rbd->data_pointer)),
+				(readw(&rbd->data_length) + 3) >> 2);
+
 		/*
-		 * free the current buffer, restart the engine
-		 * and move forward to the next buffer
+		 * Get buffer address and size
 		 */
-		fec_rbd_clean(fec->rbd_index == (FEC_RBD_NUM - 1) ? 1 : 0, rbd);
-		fec_rx_task_enable(fec);
-		fec->rbd_index = (fec->rbd_index + 1) % FEC_RBD_NUM;
+		frame = phys_to_virt(readl(&rbd->data_pointer));
+		frame_length = readw(&rbd->data_length) - 4;
+		net_receive(dev, frame->data, frame_length);
+		len = frame_length;
+	} else {
+		if (bd_status & FEC_RBD_ERR) {
+			dev_warn(&dev->dev, "error frame: 0x%p 0x%08x\n", rbd, bd_status);
+		}
 	}
+	/*
+	 * free the current buffer, restart the engine
+	 * and move forward to the next buffer
+	 */
+	fec_rbd_clean(fec->rbd_index == (FEC_RBD_NUM - 1) ? 1 : 0, rbd);
+	fec_rx_task_enable(fec);
+	fec->rbd_index = (fec->rbd_index + 1) % FEC_RBD_NUM;
 
 	return len;
 }
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/7] net: fec_imx: Don't check bd_status & FEC_RBD_ERR twice

[Andrey Smirnov]

Re-shuffle comparisons in order to avoid having to check for
FEC_RBD_ERR in bd_status more than once.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 904eba30a..25e2425d6 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -571,9 +571,11 @@ static int fec_recv(struct eth_device *dev)
 	if (bd_status & FEC_RBD_EMPTY)
 		return 0;
 
-	if ((bd_status & FEC_RBD_LAST) && !(bd_status & FEC_RBD_ERR) &&
-	    ((readw(&rbd->data_length) - 4) > 14)) {
-
+	if (bd_status & FEC_RBD_ERR) {
+		dev_warn(&dev->dev, "error frame: 0x%p 0x%08x\n",
+			 rbd, bd_status);
+	} else if ((bd_status & FEC_RBD_LAST) &&
+		   ((readw(&rbd->data_length) - 4) > 14)) {
 		if (fec_is_imx28(fec))
 			imx28_fix_endianess_rd(
 				phys_to_virt(readl(&rbd->data_pointer)),
@@ -586,10 +588,6 @@ static int fec_recv(struct eth_device *dev)
 		frame_length = readw(&rbd->data_length) - 4;
 		net_receive(dev, frame->data, frame_length);
 		len = frame_length;
-	} else {
-		if (bd_status & FEC_RBD_ERR) {
-			dev_warn(&dev->dev, "error frame: 0x%p 0x%08x\n", rbd, bd_status);
-		}
 	}
 	/*
 	 * free the current buffer, restart the engine
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/7] net: fec_imx: Read data_length only once

[Andrey Smirnov]

Data length of a given Rx data descriptor isn't going to change until
that descriptor is processed and given back to HW. Re-work the code to
save and re-use that value instead.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 25e2425d6..36218f3c9 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -574,20 +574,23 @@ static int fec_recv(struct eth_device *dev)
 	if (bd_status & FEC_RBD_ERR) {
 		dev_warn(&dev->dev, "error frame: 0x%p 0x%08x\n",
 			 rbd, bd_status);
-	} else if ((bd_status & FEC_RBD_LAST) &&
-		   ((readw(&rbd->data_length) - 4) > 14)) {
-		if (fec_is_imx28(fec))
-			imx28_fix_endianess_rd(
-				phys_to_virt(readl(&rbd->data_pointer)),
-				(readw(&rbd->data_length) + 3) >> 2);
-
-		/*
-		 * Get buffer address and size
-		 */
-		frame = phys_to_virt(readl(&rbd->data_pointer));
-		frame_length = readw(&rbd->data_length) - 4;
-		net_receive(dev, frame->data, frame_length);
-		len = frame_length;
+	} else if (bd_status & FEC_RBD_LAST) {
+		const uint16_t data_length = readw(&rbd->data_length);
+
+		if (data_length - 4 > 14) {
+			if (fec_is_imx28(fec))
+				imx28_fix_endianess_rd(
+					phys_to_virt(readl(&rbd->data_pointer)),
+					(data_length + 3) >> 2);
+
+			/*
+			 * Get buffer address and size
+			 */
+			frame = phys_to_virt(readl(&rbd->data_pointer));
+			frame_length = data_length - 4;
+			net_receive(dev, frame->data, frame_length);
+			len = frame_length;
+		}
 	}
 	/*
 	 * free the current buffer, restart the engine
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/7] net: fec_imx: Drop struct fec_frame

[Andrey Smirnov]

Drop struct fec_frame since it doesn't have any real users in the
code. It is only referenced in fec_recv() and is used by that function
as a generic pointer.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 36218f3c9..ff5a64174 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -33,12 +33,6 @@
 
 #include "fec_imx.h"
 
-struct fec_frame {
-	uint8_t data[1500];	/* actual data */
-	int length;		/* actual length */
-	int used;		/* buffer in use or not */
-	uint8_t head[16];	/* MAC header(6 + 6 + 2) + 2(aligned) */
-};
 
 /*
  * MII-interface related functions
@@ -530,7 +524,6 @@ static int fec_recv(struct eth_device *dev)
 	struct buffer_descriptor __iomem *rbd = &fec->rbd_base[fec->rbd_index];
 	uint32_t ievent;
 	int frame_length, len = 0;
-	struct fec_frame *frame;
 	uint16_t bd_status;
 
 	/*
@@ -578,17 +571,16 @@ static int fec_recv(struct eth_device *dev)
 		const uint16_t data_length = readw(&rbd->data_length);
 
 		if (data_length - 4 > 14) {
+			void *frame = phys_to_virt(readl(&rbd->data_pointer));
 			if (fec_is_imx28(fec))
-				imx28_fix_endianess_rd(
-					phys_to_virt(readl(&rbd->data_pointer)),
-					(data_length + 3) >> 2);
+				imx28_fix_endianess_rd(frame,
+						       (data_length + 3) >> 2);
 
 			/*
 			 * Get buffer address and size
 			 */
-			frame = phys_to_virt(readl(&rbd->data_pointer));
 			frame_length = data_length - 4;
-			net_receive(dev, frame->data, frame_length);
+			net_receive(dev, frame, frame_length);
 			len = frame_length;
 		}
 	}
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/7] net: fec_imx: Drop frame_length

[Andrey Smirnov]

Drop frame_length in favour of just using len

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index ff5a64174..843eba71f 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -523,7 +523,7 @@ static int fec_recv(struct eth_device *dev)
 	struct fec_priv *fec = (struct fec_priv *)dev->priv;
 	struct buffer_descriptor __iomem *rbd = &fec->rbd_base[fec->rbd_index];
 	uint32_t ievent;
-	int frame_length, len = 0;
+	int len = 0;
 	uint16_t bd_status;
 
 	/*
@@ -579,9 +579,8 @@ static int fec_recv(struct eth_device *dev)
 			/*
 			 * Get buffer address and size
 			 */
-			frame_length = data_length - 4;
-			net_receive(dev, frame, frame_length);
-			len = frame_length;
+			len = data_length - 4;
+			net_receive(dev, frame, len);
 		}
 	}
 	/*
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/7] net: fec_imx: Make use of readx_poll_timeout() macros

[Andrey Smirnov]

Condense a bit of code by using vairous readx_poll_timeout() macros.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 61 ++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 843eba71f..33f2da84a 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -30,6 +30,7 @@
 #include <of_net.h>
 #include <of_gpio.h>
 #include <gpio.h>
+#include <linux/iopoll.h>
 
 #include "fec_imx.h"
 
@@ -43,7 +44,6 @@ static int fec_miibus_read(struct mii_bus *bus, int phyAddr, int regAddr)
 
 	uint32_t reg;		/* convenient holder for the PHY register */
 	uint32_t phy;		/* convenient holder for the PHY */
-	uint64_t start;
 
 	writel(((clk_get_rate(fec->clk[FEC_CLK_IPG]) >> 20) / 5) << 1,
 			fec->regs + FEC_MII_SPEED);
@@ -60,12 +60,10 @@ static int fec_miibus_read(struct mii_bus *bus, int phyAddr, int regAddr)
 	/*
 	 * wait for the related interrupt
 	 */
-	start = get_time_ns();
-	while (!(readl(fec->regs + FEC_IEVENT) & FEC_IEVENT_MII)) {
-		if (is_timeout(start, MSECOND)) {
-			dev_err(&fec->edev.dev, "Read MDIO failed...\n");
-			return -1;
-		}
+	if (readl_poll_timeout(fec->regs + FEC_IEVENT, reg,
+			       reg & FEC_IEVENT_MII, MSECOND)) {
+		dev_err(&fec->edev.dev, "Read MDIO failed...\n");
+		return -1;
 	}
 
 	/*
@@ -86,7 +84,6 @@ static int fec_miibus_write(struct mii_bus *bus, int phyAddr,
 
 	uint32_t reg;		/* convenient holder for the PHY register */
 	uint32_t phy;		/* convenient holder for the PHY */
-	uint64_t start;
 
 	writel(((clk_get_rate(fec->clk[FEC_CLK_IPG]) >> 20) / 5) << 1,
 			fec->regs + FEC_MII_SPEED);
@@ -100,12 +97,10 @@ static int fec_miibus_write(struct mii_bus *bus, int phyAddr,
 	/*
 	 * wait for the MII interrupt
 	 */
-	start = get_time_ns();
-	while (!(readl(fec->regs + FEC_IEVENT) & FEC_IEVENT_MII)) {
-		if (is_timeout(start, MSECOND)) {
-			dev_err(&fec->edev.dev, "Write MDIO failed...\n");
-			return -1;
-		}
+	if (readl_poll_timeout(fec->regs + FEC_IEVENT, reg,
+			       reg & FEC_IEVENT_MII, MSECOND)) {
+		dev_err(&fec->edev.dev, "Write MDIO failed...\n");
+		return -1;
 	}
 
 	/*
@@ -408,20 +403,16 @@ static int fec_open(struct eth_device *edev)
 static void fec_halt(struct eth_device *dev)
 {
 	struct fec_priv *fec = (struct fec_priv *)dev->priv;
-	uint64_t tmo;
+	uint32_t reg;
 
 	/* issue graceful stop command to the FEC transmitter if necessary */
 	writel(readl(fec->regs + FEC_X_CNTRL) | FEC_ECNTRL_RESET,
 			fec->regs + FEC_X_CNTRL);
 
 	/* wait for graceful stop to register */
-	tmo = get_time_ns();
-	while (!(readl(fec->regs + FEC_IEVENT) & FEC_IEVENT_GRA)) {
-		if (is_timeout(tmo, 1 * SECOND)) {
-			dev_err(&dev->dev, "graceful stop timeout\n");
-			break;
-		}
-	}
+	if (readl_poll_timeout(fec->regs + FEC_IEVENT, reg,
+			       reg & FEC_IEVENT_GRA, SECOND))
+		dev_err(&dev->dev, "graceful stop timeout\n");
 
 	/* Disable SmartDMA tasks */
 	fec_tx_task_disable(fec);
@@ -446,7 +437,6 @@ static void fec_halt(struct eth_device *dev)
 static int fec_send(struct eth_device *dev, void *eth_data, int data_length)
 {
 	unsigned int status;
-	uint64_t tmo;
 	dma_addr_t dma;
 
 	/*
@@ -494,14 +484,10 @@ static int fec_send(struct eth_device *dev, void *eth_data, int data_length)
 	/* Enable SmartDMA transmit task */
 	fec_tx_task_enable(fec);
 
-	/* wait until frame is sent */
-	tmo = get_time_ns();
-	while (readw(&fec->tbd_base[fec->tbd_index].status) & FEC_TBD_READY) {
-		if (is_timeout(tmo, 1 * SECOND)) {
-			dev_err(&dev->dev, "transmission timeout\n");
-			break;
-		}
-	}
+	if (readw_poll_timeout(&fec->tbd_base[fec->tbd_index].status,
+			       status, !(status & FEC_TBD_READY), SECOND))
+		dev_err(&dev->dev, "transmission timeout\n");
+
 	dma_unmap_single(fec->dev, dma, data_length, DMA_TO_DEVICE);
 
 	/* for next transmission use the other buffer */
@@ -735,7 +721,7 @@ static int fec_probe(struct device_d *dev)
 	enum fec_type type;
 	int phy_reset;
 	u32 msec = 1, phy_post_delay = 0;
-	u64 start;
+	u32 reg;
 
 	ret = dev_get_drvdata(dev, (const void **)&type);
 	if (ret)
@@ -797,14 +783,11 @@ static int fec_probe(struct device_d *dev)
 	}
 
 	/* Reset chip. */
-	start = get_time_ns();
 	writel(FEC_ECNTRL_RESET, fec->regs + FEC_ECNTRL);
-	while(readl(fec->regs + FEC_ECNTRL) & FEC_ECNTRL_RESET) {
-		if (is_timeout(start, SECOND)) {
-			ret = -ETIMEDOUT;
-			goto free_gpio;
-		}
-	}
+	ret = readl_poll_timeout(fec->regs + FEC_ECNTRL, reg,
+				 !(reg & FEC_ECNTRL_RESET), SECOND);
+	if (ret)
+		goto free_gpio;
 
 	/*
 	 * reserve memory for both buffer descriptor chains at once
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/7] net: fec_imx: Do not use DMA coherent memory for Rx buffers

[Andrey Smirnov]

Trying to do unaligned access of coherent memory on AArch64 will lead
to an abort and some parts of our IP stack will do exactly that with
received packet buffer by using memcpy() to extracts parts of it.

Convert the driver to use regular memory for received data buffers to
prevent the issue from happening.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/fec_imx.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 33f2da84a..d304b94c5 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -558,6 +558,14 @@ static int fec_recv(struct eth_device *dev)
 
 		if (data_length - 4 > 14) {
 			void *frame = phys_to_virt(readl(&rbd->data_pointer));
+			/*
+			 * Sync the data for CPU so that endianness
+			 * fixup and net_receive below would get
+			 * proper data
+			 */
+			dma_sync_single_for_cpu((unsigned long)frame,
+						data_length,
+						DMA_FROM_DEVICE);
 			if (fec_is_imx28(fec))
 				imx28_fix_endianess_rd(frame,
 						       (data_length + 3) >> 2);
@@ -567,6 +575,9 @@ static int fec_recv(struct eth_device *dev)
 			 */
 			len = data_length - 4;
 			net_receive(dev, frame, len);
+			dma_sync_single_for_device((unsigned long)frame,
+						   data_length,
+						   DMA_FROM_DEVICE);
 		}
 	}
 	/*
@@ -585,13 +596,23 @@ static int fec_alloc_receive_packets(struct fec_priv *fec, int count, int size)
 	void *p;
 	int i;
 
-	/* reserve data memory and consider alignment */
-	p = dma_alloc_coherent(size * count, DMA_ADDRESS_BROKEN);
+
+	p = dma_alloc(size * count);
 	if (!p)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		writel(virt_to_phys(p), &fec->rbd_base[i].data_pointer);
+		dma_addr_t dma;
+		/*
+		 * Make sure there are no outstanding writes to the
+		 * region of memory we are going to use as receive
+		 * buffers as well as check that DMA mapping is valid
+		 */
+		dma = dma_map_single(fec->dev, p, size, DMA_FROM_DEVICE);
+		if (dma_mapping_error(fec->dev, dma))
+			return -EFAULT;
+
+		writel(dma, &fec->rbd_base[i].data_pointer);
 		p += size;
 	}
 
@@ -601,7 +622,7 @@ static int fec_alloc_receive_packets(struct fec_priv *fec, int count, int size)
 static void fec_free_receive_packets(struct fec_priv *fec, int count, int size)
 {
 	void *p = phys_to_virt(fec->rbd_base[0].data_pointer);
-	dma_free_coherent(p, 0, size * count);
+	dma_free(p);
 }
 
 #ifdef CONFIG_OFDEVICE
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/7] non-coherent Rx buffers in FEC and some small fixes

[Sascha Hauer]

On Mon, Sep 17, 2018 at 10:21:15PM -0700, Andrey Smirnov wrote:
> Everyone:
> 
> This series was created while working on converting FEC driver to use
> non-coherent memory for Rx buffers as a part of addressing feedback
> for [1]. Patch 7/7 implements said change while the rest of the
> patches are just small improvements (IMHO, of course) that I made
> while looking at FEC's source code.
> 
> Tested by using TFTP on following boards:
> 
>   - NXP i.MX8MQ EVK
>   - ZII i.MX6Q RDU2
>   - ZII VF610 CFU1
> 
> Feedback is welcome!
> 

Applied, thanks

Sascha

> Thanks,
> Andrey Smirnov
> 
> [1] http://lists.infradead.org/pipermail/barebox/2018-August/034216.html
> 
> Andrey Smirnov (7):
>   net: fec_imx: Drop extra indentation level by exiting early
>   net: fec_imx: Don't check bd_status & FEC_RBD_ERR twice
>   net: fec_imx: Read data_length only once
>   net: fec_imx: Drop struct fec_frame
>   net: fec_imx: Drop frame_length
>   net: fec_imx: Make use of readx_poll_timeout() macros
>   net: fec_imx: Do not use DMA coherent memory for Rx buffers
> 
>  drivers/net/fec_imx.c | 141 +++++++++++++++++++++---------------------
>  1 file changed, 69 insertions(+), 72 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/7] net: fec_imx: Do not use DMA coherent memory for Rx buffers

[Uwe Kleine-König]

Hello,

On Mon, Sep 17, 2018 at 10:21:22PM -0700, Andrey Smirnov wrote:
> Trying to do unaligned access of coherent memory on AArch64 will lead
> to an abort and some parts of our IP stack will do exactly that with
> received packet buffer by using memcpy() to extracts parts of it.
> 
> Convert the driver to use regular memory for received data buffers to
> prevent the issue from happening.

I didn't spend time yet to debug, but this commit is pointed out to
break nfs booting on an mx28evk board.

Maybe someone spots the obvious problem before I come around to fix
this.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/7] net: fec_imx: Do not use DMA coherent memory for Rx buffers

[Uwe Kleine-König]

On Mon, Jan 14, 2019 at 01:21:30PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Sep 17, 2018 at 10:21:22PM -0700, Andrey Smirnov wrote:
> > Trying to do unaligned access of coherent memory on AArch64 will lead
> > to an abort and some parts of our IP stack will do exactly that with
> > received packet buffer by using memcpy() to extracts parts of it.
> > 
> > Convert the driver to use regular memory for received data buffers to
> > prevent the issue from happening.
> 
> I didn't spend time yet to debug, but this commit is pointed out to
> break nfs booting on an mx28evk board.
> 
> Maybe someone spots the obvious problem before I come around to fix
> this.

Sascha found it. The problem is that the nfs code uses the memory
location the packet was placed in only after the net_receive function
returned.

With this patch applied it works for me again:

diff --git a/fs/nfs.c b/fs/nfs.c
index d7f156687fc9..20fdf5ff4dfc 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -440,6 +440,7 @@ again:
 	nfs_timer_start = get_time_ns();
 
 	nfs_state = STATE_START;
+	free(nfs_packet);
 	nfs_packet = NULL;
 
 	while (nfs_state != STATE_DONE) {
@@ -924,7 +925,7 @@ static void nfs_handler(void *ctx, char *packet, unsigned len)
 	char *pkt = net_eth_to_udp_payload(packet);
 
 	nfs_state = STATE_DONE;
-	nfs_packet = pkt;
+	nfs_packet = xmemdup(pkt, len);
 	nfs_len = len;
 }
 

(Not completely nice though, as (at least) the last packet isn't freed.)

Will try to come up with a proper patch later.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox