[PATCH] net: fec: Fixed panic problem with non-tso

[PATCH] net: fec: Fixed panic problem with non-tso

[Yuusuke Ashiduka]

If highmem and 2GB or more of memory are valid,
"this_frag-> page.p" indicates the highmem area,
so the result of page_address() is NULL and panic occurs.

This commit fixes this by using the skb_frag_dma_map() helper,
which takes care of mapping the skb fragment properly. Additionally,
the type of mapping is now tracked, so it can be unmapped using
dma_unmap_page or dma_unmap_single when appropriate.
---
 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 48 +++++++++++++++++++++++--------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 5ea740b4cf14..5b187e8aacf0 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -463,6 +463,7 @@ struct bufdesc_prop {
 struct fec_enet_priv_tx_q {
 	struct bufdesc_prop bd;
 	unsigned char *tx_bounce[TX_RING_SIZE];
+	int tx_page_mapping[TX_RING_SIZE];
 	struct  sk_buff *tx_skbuff[TX_RING_SIZE];
 
 	unsigned short tx_stop_threshold;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 38160c2bebcb..b1562107e337 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -60,6 +60,7 @@
 #include <linux/if_vlan.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/prefetch.h>
+#include <linux/highmem.h>
 #include <soc/imx/cpuidle.h>
 
 #include <asm/cacheflush.h>
@@ -377,20 +378,28 @@ fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
 			ebdp->cbd_esc = cpu_to_fec32(estatus);
 		}
 
-		bufaddr = page_address(this_frag->page.p) + this_frag->page_offset;
-
 		index = fec_enet_get_bd_index(bdp, &txq->bd);
-		if (((unsigned long) bufaddr) & fep->tx_align ||
+		txq->tx_page_mapping[index] = 0;
+		if (this_frag->page_offset & fep->tx_align ||
 			fep->quirks & FEC_QUIRK_SWAP_FRAME) {
+			bufaddr = kmap_atomic(this_frag->page.p) +
+						this_frag->page_offset;
 			memcpy(txq->tx_bounce[index], bufaddr, frag_len);
+			kunmap_atomic(bufaddr);
 			bufaddr = txq->tx_bounce[index];
 
 			if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
 				swap_buffer(bufaddr, frag_len);
+			addr = dma_map_single(&fep->pdev->dev,
+					      bufaddr,
+					      frag_len,
+					      DMA_TO_DEVICE);
+		} else {
+			txq->tx_page_mapping[index] = 1;
+			addr = skb_frag_dma_map(&fep->pdev->dev, this_frag, 0,
+						frag_len, DMA_TO_DEVICE);
 		}
 
-		addr = dma_map_single(&fep->pdev->dev, bufaddr, frag_len,
-				      DMA_TO_DEVICE);
 		if (dma_mapping_error(&fep->pdev->dev, addr)) {
 			if (net_ratelimit())
 				netdev_err(ndev, "Tx DMA memory map failed\n");
@@ -411,8 +420,16 @@ fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
 	bdp = txq->bd.cur;
 	for (i = 0; i < frag; i++) {
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
-		dma_unmap_single(&fep->pdev->dev, fec32_to_cpu(bdp->cbd_bufaddr),
-				 fec16_to_cpu(bdp->cbd_datlen), DMA_TO_DEVICE);
+		if (txq->tx_page_mapping[index])
+			dma_unmap_page(&fep->pdev->dev,
+				       fec32_to_cpu(bdp->cbd_bufaddr),
+				       fec16_to_cpu(bdp->cbd_datlen),
+				       DMA_TO_DEVICE);
+		else
+			dma_unmap_single(&fep->pdev->dev,
+					 fec32_to_cpu(bdp->cbd_bufaddr),
+					 fec16_to_cpu(bdp->cbd_datlen),
+					 DMA_TO_DEVICE);
 	}
 	return ERR_PTR(-ENOMEM);
 }
@@ -1201,11 +1218,18 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
 
 		skb = txq->tx_skbuff[index];
 		txq->tx_skbuff[index] = NULL;
-		if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
-			dma_unmap_single(&fep->pdev->dev,
-					 fec32_to_cpu(bdp->cbd_bufaddr),
-					 fec16_to_cpu(bdp->cbd_datlen),
-					 DMA_TO_DEVICE);
+		if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr))) {
+			if (txq->tx_page_mapping[index])
+				dma_unmap_page(&fep->pdev->dev,
+					       fec32_to_cpu(bdp->cbd_bufaddr),
+					       fec16_to_cpu(bdp->cbd_datlen),
+					       DMA_TO_DEVICE);
+			else
+				dma_unmap_single(&fep->pdev->dev,
+						 fec32_to_cpu(bdp->cbd_bufaddr),
+						 fec16_to_cpu(bdp->cbd_datlen),
+						 DMA_TO_DEVICE);
+		}
 		bdp->cbd_bufaddr = cpu_to_fec32(0);
 		if (!skb)
 			goto skb_done;
-- 
2.11.0

RE: [PATCH] net: fec: Fixed panic problem with non-tso

[Andy Duan]

From: Yuusuke Ashiduka <ashiduka@jp.fujitsu.com> Sent: Tuesday, January 17, 2017 3:48 PM
>To: Andy Duan <fugang.duan@nxp.com>
>Cc: netdev@vger.kernel.org; Yuusuke Ashiduka <ashiduka@jp.fujitsu.com>
>Subject: [PATCH] net: fec: Fixed panic problem with non-tso
>
>If highmem and 2GB or more of memory are valid, "this_frag-> page.p"
>indicates the highmem area, so the result of page_address() is NULL and panic
>occurs.
>
>This commit fixes this by using the skb_frag_dma_map() helper, which takes
>care of mapping the skb fragment properly. Additionally, the type of mapping
>is now tracked, so it can be unmapped using dma_unmap_page or
>dma_unmap_single when appropriate.
>---
> drivers/net/ethernet/freescale/fec.h      |  1 +
> drivers/net/ethernet/freescale/fec_main.c | 48
>+++++++++++++++++++++++--------
> 2 files changed, 37 insertions(+), 12 deletions(-)
>
The patch itself seems fine.
The driver doesn't support skb from highmem, if to support highmem, it should add frag_skb (highmem) support for tso and non-tso.
In driver net/core/tso.c, it also add highmem support, right ? 

Thanks.

>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index 5ea740b4cf14..5b187e8aacf0 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -463,6 +463,7 @@ struct bufdesc_prop {  struct fec_enet_priv_tx_q {
> 	struct bufdesc_prop bd;
> 	unsigned char *tx_bounce[TX_RING_SIZE];
>+	int tx_page_mapping[TX_RING_SIZE];
> 	struct  sk_buff *tx_skbuff[TX_RING_SIZE];
>
> 	unsigned short tx_stop_threshold;
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 38160c2bebcb..b1562107e337 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -60,6 +60,7 @@
> #include <linux/if_vlan.h>
> #include <linux/pinctrl/consumer.h>
> #include <linux/prefetch.h>
>+#include <linux/highmem.h>
> #include <soc/imx/cpuidle.h>
>
> #include <asm/cacheflush.h>
>@@ -377,20 +378,28 @@ fec_enet_txq_submit_frag_skb(struct
>fec_enet_priv_tx_q *txq,
> 			ebdp->cbd_esc = cpu_to_fec32(estatus);
> 		}
>
>-		bufaddr = page_address(this_frag->page.p) + this_frag-
>>page_offset;
>-
> 		index = fec_enet_get_bd_index(bdp, &txq->bd);
>-		if (((unsigned long) bufaddr) & fep->tx_align ||
>+		txq->tx_page_mapping[index] = 0;
>+		if (this_frag->page_offset & fep->tx_align ||
> 			fep->quirks & FEC_QUIRK_SWAP_FRAME) {
>+			bufaddr = kmap_atomic(this_frag->page.p) +
>+						this_frag->page_offset;
> 			memcpy(txq->tx_bounce[index], bufaddr, frag_len);
>+			kunmap_atomic(bufaddr);
> 			bufaddr = txq->tx_bounce[index];
>
> 			if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> 				swap_buffer(bufaddr, frag_len);
>+			addr = dma_map_single(&fep->pdev->dev,
>+					      bufaddr,
>+					      frag_len,
>+					      DMA_TO_DEVICE);
>+		} else {
>+			txq->tx_page_mapping[index] = 1;
>+			addr = skb_frag_dma_map(&fep->pdev->dev,
>this_frag, 0,
>+						frag_len, DMA_TO_DEVICE);
> 		}
>
>-		addr = dma_map_single(&fep->pdev->dev, bufaddr, frag_len,
>-				      DMA_TO_DEVICE);
> 		if (dma_mapping_error(&fep->pdev->dev, addr)) {
> 			if (net_ratelimit())
> 				netdev_err(ndev, "Tx DMA memory map
>failed\n"); @@ -411,8 +420,16 @@ fec_enet_txq_submit_frag_skb(struct
>fec_enet_priv_tx_q *txq,
> 	bdp = txq->bd.cur;
> 	for (i = 0; i < frag; i++) {
> 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
>-		dma_unmap_single(&fep->pdev->dev, fec32_to_cpu(bdp-
>>cbd_bufaddr),
>-				 fec16_to_cpu(bdp->cbd_datlen),
>DMA_TO_DEVICE);
>+		if (txq->tx_page_mapping[index])
>+			dma_unmap_page(&fep->pdev->dev,
>+				       fec32_to_cpu(bdp->cbd_bufaddr),
>+				       fec16_to_cpu(bdp->cbd_datlen),
>+				       DMA_TO_DEVICE);
>+		else
>+			dma_unmap_single(&fep->pdev->dev,
>+					 fec32_to_cpu(bdp->cbd_bufaddr),
>+					 fec16_to_cpu(bdp->cbd_datlen),
>+					 DMA_TO_DEVICE);
> 	}
> 	return ERR_PTR(-ENOMEM);
> }
>@@ -1201,11 +1218,18 @@ fec_enet_tx_queue(struct net_device *ndev, u16
>queue_id)
>
> 		skb = txq->tx_skbuff[index];
> 		txq->tx_skbuff[index] = NULL;
>-		if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
>-			dma_unmap_single(&fep->pdev->dev,
>-					 fec32_to_cpu(bdp->cbd_bufaddr),
>-					 fec16_to_cpu(bdp->cbd_datlen),
>-					 DMA_TO_DEVICE);
>+		if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
>{
>+			if (txq->tx_page_mapping[index])
>+				dma_unmap_page(&fep->pdev->dev,
>+					       fec32_to_cpu(bdp->cbd_bufaddr),
>+					       fec16_to_cpu(bdp->cbd_datlen),
>+					       DMA_TO_DEVICE);
>+			else
>+				dma_unmap_single(&fep->pdev->dev,
>+						 fec32_to_cpu(bdp-
>>cbd_bufaddr),
>+						 fec16_to_cpu(bdp-
>>cbd_datlen),
>+						 DMA_TO_DEVICE);
>+		}
> 		bdp->cbd_bufaddr = cpu_to_fec32(0);
> 		if (!skb)
> 			goto skb_done;
>--
>2.11.0

Re: [PATCH] net: fec: Fixed panic problem with non-tso

[David Miller]

From: Yuusuke Ashiduka <ashiduka@jp.fujitsu.com>
Date: Tue, 17 Jan 2017 16:48:20 +0900

> If highmem and 2GB or more of memory are valid,
> "this_frag-> page.p" indicates the highmem area,
> so the result of page_address() is NULL and panic occurs.
> 
> This commit fixes this by using the skb_frag_dma_map() helper,
> which takes care of mapping the skb fragment properly. Additionally,
> the type of mapping is now tracked, so it can be unmapped using
> dma_unmap_page or dma_unmap_single when appropriate.

This patch submission is lacking a proper signoff.

RE: [PATCH] net: fec: Fixed panic problem with non-tso

[Ashizuka, Yuusuke]

> -----Original Message-----
> From: Andy Duan [mailto:fugang.duan@nxp.com]
> Sent: Tuesday, January 17, 2017 8:02 PM
> To: Ashizuka, Yuusuke/芦塚 雄介
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH] net: fec: Fixed panic problem with non-tso
> 
> From: Yuusuke Ashiduka <ashiduka@jp.fujitsu.com> Sent: Tuesday, January
> 17, 2017 3:48 PM
> >To: Andy Duan <fugang.duan@nxp.com>
> >Cc: netdev@vger.kernel.org; Yuusuke Ashiduka <ashiduka@jp.fujitsu.com>
> >Subject: [PATCH] net: fec: Fixed panic problem with non-tso
> >
> >If highmem and 2GB or more of memory are valid, "this_frag-> page.p"
> >indicates the highmem area, so the result of page_address() is NULL and
> >panic occurs.
> >
> >This commit fixes this by using the skb_frag_dma_map() helper, which
> >takes care of mapping the skb fragment properly. Additionally, the type
> >of mapping is now tracked, so it can be unmapped using dma_unmap_page
> >or dma_unmap_single when appropriate.
> >---
> > drivers/net/ethernet/freescale/fec.h      |  1 +
> > drivers/net/ethernet/freescale/fec_main.c | 48
> >+++++++++++++++++++++++--------
> > 2 files changed, 37 insertions(+), 12 deletions(-)
> >
> The patch itself seems fine.
> The driver doesn't support skb from highmem, if to support highmem, it should
> add frag_skb (highmem) support for tso and non-tso.
> In driver net/core/tso.c, it also add highmem support, right ?

indeed.

In the case of TSO with i.MX6 system (highmem enabled) with 2GB memory,
"this_frag->page.p" did not become highmem area.
(We confirmed by transferring about 100MB of files)

However, in the case of non-tso on an i.MX6 system with 2GB of memory,
"this_frag->page.p" may become a highmem area.
(Occurred with approximately 2MB of file transfer)

For non-tso only, I do not know the reason why "this_frag-> page.p" 
in this driver shows highmem area.

Thanks.

> 
> Thanks.
> 
> >diff --git a/drivers/net/ethernet/freescale/fec.h
> >b/drivers/net/ethernet/freescale/fec.h
> >index 5ea740b4cf14..5b187e8aacf0 100644
> >--- a/drivers/net/ethernet/freescale/fec.h
> >+++ b/drivers/net/ethernet/freescale/fec.h
> >@@ -463,6 +463,7 @@ struct bufdesc_prop {  struct fec_enet_priv_tx_q {
> > 	struct bufdesc_prop bd;
> > 	unsigned char *tx_bounce[TX_RING_SIZE];
> >+	int tx_page_mapping[TX_RING_SIZE];
> > 	struct  sk_buff *tx_skbuff[TX_RING_SIZE];
> >
> > 	unsigned short tx_stop_threshold;
> >diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >b/drivers/net/ethernet/freescale/fec_main.c
> >index 38160c2bebcb..b1562107e337 100644
> >--- a/drivers/net/ethernet/freescale/fec_main.c
> >+++ b/drivers/net/ethernet/freescale/fec_main.c
> >@@ -60,6 +60,7 @@
> > #include <linux/if_vlan.h>
> > #include <linux/pinctrl/consumer.h>
> > #include <linux/prefetch.h>
> >+#include <linux/highmem.h>
> > #include <soc/imx/cpuidle.h>
> >
> > #include <asm/cacheflush.h>
> >@@ -377,20 +378,28 @@ fec_enet_txq_submit_frag_skb(struct
> >fec_enet_priv_tx_q *txq,
> > 			ebdp->cbd_esc = cpu_to_fec32(estatus);
> > 		}
> >
> >-		bufaddr = page_address(this_frag->page.p) + this_frag-
> >>page_offset;
> >-
> > 		index = fec_enet_get_bd_index(bdp, &txq->bd);
> >-		if (((unsigned long) bufaddr) & fep->tx_align ||
> >+		txq->tx_page_mapping[index] = 0;
> >+		if (this_frag->page_offset & fep->tx_align ||
> > 			fep->quirks & FEC_QUIRK_SWAP_FRAME) {
> >+			bufaddr = kmap_atomic(this_frag->page.p) +
> >+
> 	this_frag->page_offset;
> > 			memcpy(txq->tx_bounce[index], bufaddr,
> frag_len);
> >+			kunmap_atomic(bufaddr);
> > 			bufaddr = txq->tx_bounce[index];
> >
> > 			if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> > 				swap_buffer(bufaddr, frag_len);
> >+			addr = dma_map_single(&fep->pdev->dev,
> >+					      bufaddr,
> >+					      frag_len,
> >+					      DMA_TO_DEVICE);
> >+		} else {
> >+			txq->tx_page_mapping[index] = 1;
> >+			addr = skb_frag_dma_map(&fep->pdev->dev,
> >this_frag, 0,
> >+						frag_len,
> DMA_TO_DEVICE);
> > 		}
> >
> >-		addr = dma_map_single(&fep->pdev->dev, bufaddr,
> frag_len,
> >-				      DMA_TO_DEVICE);
> > 		if (dma_mapping_error(&fep->pdev->dev, addr)) {
> > 			if (net_ratelimit())
> > 				netdev_err(ndev, "Tx DMA memory map
> failed\n"); @@ -411,8 +420,16
> >@@ fec_enet_txq_submit_frag_skb(struct
> >fec_enet_priv_tx_q *txq,
> > 	bdp = txq->bd.cur;
> > 	for (i = 0; i < frag; i++) {
> > 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
> >-		dma_unmap_single(&fep->pdev->dev, fec32_to_cpu(bdp-
> >>cbd_bufaddr),
> >-				 fec16_to_cpu(bdp->cbd_datlen),
> >DMA_TO_DEVICE);
> >+		if (txq->tx_page_mapping[index])
> >+			dma_unmap_page(&fep->pdev->dev,
> >+				       fec32_to_cpu(bdp->cbd_bufaddr),
> >+				       fec16_to_cpu(bdp->cbd_datlen),
> >+				       DMA_TO_DEVICE);
> >+		else
> >+			dma_unmap_single(&fep->pdev->dev,
> >+
> fec32_to_cpu(bdp->cbd_bufaddr),
> >+
> fec16_to_cpu(bdp->cbd_datlen),
> >+					 DMA_TO_DEVICE);
> > 	}
> > 	return ERR_PTR(-ENOMEM);
> > }
> >@@ -1201,11 +1218,18 @@ fec_enet_tx_queue(struct net_device *ndev, u16
> >queue_id)
> >
> > 		skb = txq->tx_skbuff[index];
> > 		txq->tx_skbuff[index] = NULL;
> >-		if (!IS_TSO_HEADER(txq,
> fec32_to_cpu(bdp->cbd_bufaddr)))
> >-			dma_unmap_single(&fep->pdev->dev,
> >-
> fec32_to_cpu(bdp->cbd_bufaddr),
> >-
> fec16_to_cpu(bdp->cbd_datlen),
> >-					 DMA_TO_DEVICE);
> >+		if (!IS_TSO_HEADER(txq,
> fec32_to_cpu(bdp->cbd_bufaddr)))
> >{
> >+			if (txq->tx_page_mapping[index])
> >+				dma_unmap_page(&fep->pdev->dev,
> >+
> fec32_to_cpu(bdp->cbd_bufaddr),
> >+
> fec16_to_cpu(bdp->cbd_datlen),
> >+					       DMA_TO_DEVICE);
> >+			else
> >+				dma_unmap_single(&fep->pdev->dev,
> >+						 fec32_to_cpu(bdp-
> >>cbd_bufaddr),
> >+						 fec16_to_cpu(bdp-
> >>cbd_datlen),
> >+						 DMA_TO_DEVICE);
> >+		}
> > 		bdp->cbd_bufaddr = cpu_to_fec32(0);
> > 		if (!skb)
> > 			goto skb_done;
> >--
> >2.11.0

RE: [PATCH] net: fec: Fixed panic problem with non-tso

[Ashizuka, Yuusuke]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, January 18, 2017 5:45 AM
> To: Ashizuka, Yuusuke/芦塚 雄介
> Cc: fugang.duan@nxp.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: fec: Fixed panic problem with non-tso
> 
> From: Yuusuke Ashiduka <ashiduka@jp.fujitsu.com>
> Date: Tue, 17 Jan 2017 16:48:20 +0900
> 
> > If highmem and 2GB or more of memory are valid, "this_frag-> page.p"
> > indicates the highmem area, so the result of page_address() is NULL
> > and panic occurs.
> >
> > This commit fixes this by using the skb_frag_dma_map() helper, which
> > takes care of mapping the skb fragment properly. Additionally, the
> > type of mapping is now tracked, so it can be unmapped using
> > dma_unmap_page or dma_unmap_single when appropriate.
> 
> This patch submission is lacking a proper signoff.

Thank you for pointing out my mistake.
I will submit the patch again.

[PATCH v2] net: fec: Fixed panic problem with non-tso

[Yuusuke Ashiduka]

If highmem and 2GB or more of memory are valid,
"this_frag-> page.p" indicates the highmem area,
so the result of page_address() is NULL and panic occurs.

This commit fixes this by using the skb_frag_dma_map() helper,
which takes care of mapping the skb fragment properly. Additionally,
the type of mapping is now tracked, so it can be unmapped using
dma_unmap_page or dma_unmap_single when appropriate.

Signed-off-by: Yuusuke Ashiduka <ashiduka@jp.fujitsu.com>
---

Changes for v2:
 - Added signed-off
---
 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 48 +++++++++++++++++++++++--------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 5ea740b4cf14..5b187e8aacf0 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -463,6 +463,7 @@ struct bufdesc_prop {
 struct fec_enet_priv_tx_q {
 	struct bufdesc_prop bd;
 	unsigned char *tx_bounce[TX_RING_SIZE];
+	int tx_page_mapping[TX_RING_SIZE];
 	struct  sk_buff *tx_skbuff[TX_RING_SIZE];
 
 	unsigned short tx_stop_threshold;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 38160c2bebcb..b1562107e337 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -60,6 +60,7 @@
 #include <linux/if_vlan.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/prefetch.h>
+#include <linux/highmem.h>
 #include <soc/imx/cpuidle.h>
 
 #include <asm/cacheflush.h>
@@ -377,20 +378,28 @@ fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
 			ebdp->cbd_esc = cpu_to_fec32(estatus);
 		}
 
-		bufaddr = page_address(this_frag->page.p) + this_frag->page_offset;
-
 		index = fec_enet_get_bd_index(bdp, &txq->bd);
-		if (((unsigned long) bufaddr) & fep->tx_align ||
+		txq->tx_page_mapping[index] = 0;
+		if (this_frag->page_offset & fep->tx_align ||
 			fep->quirks & FEC_QUIRK_SWAP_FRAME) {
+			bufaddr = kmap_atomic(this_frag->page.p) +
+						this_frag->page_offset;
 			memcpy(txq->tx_bounce[index], bufaddr, frag_len);
+			kunmap_atomic(bufaddr);
 			bufaddr = txq->tx_bounce[index];
 
 			if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
 				swap_buffer(bufaddr, frag_len);
+			addr = dma_map_single(&fep->pdev->dev,
+					      bufaddr,
+					      frag_len,
+					      DMA_TO_DEVICE);
+		} else {
+			txq->tx_page_mapping[index] = 1;
+			addr = skb_frag_dma_map(&fep->pdev->dev, this_frag, 0,
+						frag_len, DMA_TO_DEVICE);
 		}
 
-		addr = dma_map_single(&fep->pdev->dev, bufaddr, frag_len,
-				      DMA_TO_DEVICE);
 		if (dma_mapping_error(&fep->pdev->dev, addr)) {
 			if (net_ratelimit())
 				netdev_err(ndev, "Tx DMA memory map failed\n");
@@ -411,8 +420,16 @@ fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
 	bdp = txq->bd.cur;
 	for (i = 0; i < frag; i++) {
 		bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
-		dma_unmap_single(&fep->pdev->dev, fec32_to_cpu(bdp->cbd_bufaddr),
-				 fec16_to_cpu(bdp->cbd_datlen), DMA_TO_DEVICE);
+		if (txq->tx_page_mapping[index])
+			dma_unmap_page(&fep->pdev->dev,
+				       fec32_to_cpu(bdp->cbd_bufaddr),
+				       fec16_to_cpu(bdp->cbd_datlen),
+				       DMA_TO_DEVICE);
+		else
+			dma_unmap_single(&fep->pdev->dev,
+					 fec32_to_cpu(bdp->cbd_bufaddr),
+					 fec16_to_cpu(bdp->cbd_datlen),
+					 DMA_TO_DEVICE);
 	}
 	return ERR_PTR(-ENOMEM);
 }
@@ -1201,11 +1218,18 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
 
 		skb = txq->tx_skbuff[index];
 		txq->tx_skbuff[index] = NULL;
-		if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
-			dma_unmap_single(&fep->pdev->dev,
-					 fec32_to_cpu(bdp->cbd_bufaddr),
-					 fec16_to_cpu(bdp->cbd_datlen),
-					 DMA_TO_DEVICE);
+		if (!IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr))) {
+			if (txq->tx_page_mapping[index])
+				dma_unmap_page(&fep->pdev->dev,
+					       fec32_to_cpu(bdp->cbd_bufaddr),
+					       fec16_to_cpu(bdp->cbd_datlen),
+					       DMA_TO_DEVICE);
+			else
+				dma_unmap_single(&fep->pdev->dev,
+						 fec32_to_cpu(bdp->cbd_bufaddr),
+						 fec16_to_cpu(bdp->cbd_datlen),
+						 DMA_TO_DEVICE);
+		}
 		bdp->cbd_bufaddr = cpu_to_fec32(0);
 		if (!skb)
 			goto skb_done;
-- 
2.11.0

Re: [PATCH] net: fec: Fixed panic problem with non-tso

[Eric Dumazet]

On Wed, 2017-01-18 at 03:12 +0000, Ashizuka, Yuusuke wrote:

> indeed.
> 
> In the case of TSO with i.MX6 system (highmem enabled) with 2GB memory,
> "this_frag->page.p" did not become highmem area.
> (We confirmed by transferring about 100MB of files)
> 
> However, in the case of non-tso on an i.MX6 system with 2GB of memory,
> "this_frag->page.p" may become a highmem area.
> (Occurred with approximately 2MB of file transfer)
> 
> For non-tso only, I do not know the reason why "this_frag-> page.p" 
> in this driver shows highmem area.

This worries me, since this driver does not set NETIF_F_HIGHDMA in its
features.

No packet should be given to this driver with a highmem fragment

Check is done in illegal_highdma() in net/core/dev.c

Re: [PATCH] net: fec: Fixed panic problem with non-tso

[Eric Dumazet]

On Tue, 2017-01-17 at 20:21 -0800, Eric Dumazet wrote:
> On Wed, 2017-01-18 at 03:12 +0000, Ashizuka, Yuusuke wrote:
> 
> > indeed.
> > 
> > In the case of TSO with i.MX6 system (highmem enabled) with 2GB memory,
> > "this_frag->page.p" did not become highmem area.
> > (We confirmed by transferring about 100MB of files)
> > 
> > However, in the case of non-tso on an i.MX6 system with 2GB of memory,
> > "this_frag->page.p" may become a highmem area.
> > (Occurred with approximately 2MB of file transfer)
> > 
> > For non-tso only, I do not know the reason why "this_frag-> page.p" 
> > in this driver shows highmem area.
> 
> This worries me, since this driver does not set NETIF_F_HIGHDMA in its
> features.
> 
> No packet should be given to this driver with a highmem fragment
> 
> Check is done in illegal_highdma() in net/core/dev.c

This used to work.

I suspect commit ec5f061564238892005257c83565a0b58ec79295
("net: Kill link between CSUM and SG features.")

added this bug.

Can you try this hot fix :

diff --git a/net/core/dev.c b/net/core/dev.c
index ad5959e561166f445bdd9d7260652a338f74cfea..073b832b945257dba9ed47f4bf875605225effc9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2773,9 +2773,9 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
 	if (skb->ip_summed != CHECKSUM_NONE &&
 	    !can_checksum_protocol(features, type)) {
 		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
-	} else if (illegal_highdma(skb->dev, skb)) {
-		features &= ~NETIF_F_SG;
 	}
+	if (illegal_highdma(skb->dev, skb))
+		features &= ~NETIF_F_SG;
 
 	return features;
 }

Re: [PATCH v2] net: fec: Fixed panic problem with non-tso

[Eric Dumazet]

On Wed, 2017-01-18 at 13:11 +0900, Yuusuke Ashiduka wrote:
> If highmem and 2GB or more of memory are valid,
> "this_frag-> page.p" indicates the highmem area,
> so the result of page_address() is NULL and panic occurs.
> 
> This commit fixes this by using the skb_frag_dma_map() helper,
> which takes care of mapping the skb fragment properly. Additionally,
> the type of mapping is now tracked, so it can be unmapped using
> dma_unmap_page or dma_unmap_single when appropriate.


I would prefer we fix the root cause, instead of tweaking all legacy
drivers out there :/

RE: [PATCH v2] net: fec: Fixed panic problem with non-tso

[Andy Duan]

From: Eric Dumazet <eric.dumazet@gmail.com> Sent: Wednesday, January 18, 2017 1:02 PM
>To: Yuusuke Ashiduka <ashiduka@jp.fujitsu.com>
>Cc: Andy Duan <fugang.duan@nxp.com>; netdev@vger.kernel.org
>Subject: Re: [PATCH v2] net: fec: Fixed panic problem with non-tso
>
>On Wed, 2017-01-18 at 13:11 +0900, Yuusuke Ashiduka wrote:
>> If highmem and 2GB or more of memory are valid, "this_frag-> page.p"
>> indicates the highmem area, so the result of page_address() is NULL
>> and panic occurs.
>>
>> This commit fixes this by using the skb_frag_dma_map() helper, which
>> takes care of mapping the skb fragment properly. Additionally, the
>> type of mapping is now tracked, so it can be unmapped using
>> dma_unmap_page or dma_unmap_single when appropriate.
>
>
>I would prefer we fix the root cause, instead of tweaking all legacy drivers out
>there :/
>
>
I agree with you.

The driver always doesn't support highmem. The fragment shouldn't  allocate from highmem except the common code bug.
If request the driver to support NETIF_F_HIGHDMA feature, we also add highmem support for tso driver.

Andy

Re: [PATCH] net: fec: Fixed panic problem with non-tso

[Pravin Shelar]

On Tue, Jan 17, 2017 at 8:35 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-01-17 at 20:21 -0800, Eric Dumazet wrote:
>> On Wed, 2017-01-18 at 03:12 +0000, Ashizuka, Yuusuke wrote:
>>
>> > indeed.
>> >
>> > In the case of TSO with i.MX6 system (highmem enabled) with 2GB memory,
>> > "this_frag->page.p" did not become highmem area.
>> > (We confirmed by transferring about 100MB of files)
>> >
>> > However, in the case of non-tso on an i.MX6 system with 2GB of memory,
>> > "this_frag->page.p" may become a highmem area.
>> > (Occurred with approximately 2MB of file transfer)
>> >
>> > For non-tso only, I do not know the reason why "this_frag-> page.p"
>> > in this driver shows highmem area.
>>
>> This worries me, since this driver does not set NETIF_F_HIGHDMA in its
>> features.
>>
>> No packet should be given to this driver with a highmem fragment
>>
>> Check is done in illegal_highdma() in net/core/dev.c
>
> This used to work.
>
> I suspect commit ec5f061564238892005257c83565a0b58ec79295
> ("net: Kill link between CSUM and SG features.")
>
> added this bug.
>
> Can you try this hot fix :
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ad5959e561166f445bdd9d7260652a338f74cfea..073b832b945257dba9ed47f4bf875605225effc9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2773,9 +2773,9 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
>         if (skb->ip_summed != CHECKSUM_NONE &&
>             !can_checksum_protocol(features, type)) {
>                 features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
> -       } else if (illegal_highdma(skb->dev, skb)) {
> -               features &= ~NETIF_F_SG;
>         }
> +       if (illegal_highdma(skb->dev, skb))
> +               features &= ~NETIF_F_SG;
>
>         return features;
>  }

Right, this high mem check should be decoupled from csum check.

Thanks,
Pravin.

Re: [PATCH] net: fec: Fixed panic problem with non-tso

[Eric Dumazet]

On Wed, 2017-01-18 at 10:18 -0800, Pravin Shelar wrote:
\
> Right, this high mem check should be decoupled from csum check.

I must say I am surprised nobody hit this problem before today.

linux-3.10 is more than 3 years old.

[PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Ashizuka reported a highmem oddity and sent a patch for freescale
fec driver.

But the problem root cause is that core networking stack
must ensure no skb with highmem fragment is ever sent through
a device that does not assert NETIF_F_HIGHDMA in its features.

We need to call illegal_highdma() from harmonize_features()
regardless of CSUM checks.

Fixes: ec5f06156423 ("net: Kill link between CSUM and SG features.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Pravin Shelar <pshelar@ovn.org>
Reported-by: "Ashizuka, Yuusuke" <ashiduka@jp.fujitsu.com>
---
 net/core/dev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 07b307b0b414730688b64fdb2295b0fa1b721e51..7f218e095361520d11c243d650e053321ea7274f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2795,9 +2795,9 @@ static netdev_features_t harmonize_features(struct sk_buff *skb,
 	if (skb->ip_summed != CHECKSUM_NONE &&
 	    !can_checksum_protocol(features, type)) {
 		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
-	} else if (illegal_highdma(skb->dev, skb)) {
-		features &= ~NETIF_F_SG;
 	}
+	if (illegal_highdma(skb->dev, skb))
+		features &= ~NETIF_F_SG;
 
 	return features;
 }

Re: [PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 18 Jan 2017 12:12:17 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> Ashizuka reported a highmem oddity and sent a patch for freescale
> fec driver.
> 
> But the problem root cause is that core networking stack
> must ensure no skb with highmem fragment is ever sent through
> a device that does not assert NETIF_F_HIGHDMA in its features.
> 
> We need to call illegal_highdma() from harmonize_features()
> regardless of CSUM checks.
> 
> Fixes: ec5f06156423 ("net: Kill link between CSUM and SG features.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Pravin Shelar <pshelar@ovn.org>
> Reported-by: "Ashizuka, Yuusuke" <ashiduka@jp.fujitsu.com>

Applied, thanks Eric.

I guess few devices support SG and lack highmem support.

RE: [PATCH] net: fec: Fixed panic problem with non-tso

[Andy Duan]

From: Eric Dumazet <eric.dumazet@gmail.com> Sent: Wednesday, January 18, 2017 12:36 PM
>To: Ashizuka, Yuusuke <ashiduka@jp.fujitsu.com>
>Cc: Andy Duan <fugang.duan@nxp.com>; netdev@vger.kernel.org; Pravin B
>Shelar <pshelar@nicira.com>
>Subject: Re: [PATCH] net: fec: Fixed panic problem with non-tso
>
>On Tue, 2017-01-17 at 20:21 -0800, Eric Dumazet wrote:
>> On Wed, 2017-01-18 at 03:12 +0000, Ashizuka, Yuusuke wrote:
>>
>> > indeed.
>> >
>> > In the case of TSO with i.MX6 system (highmem enabled) with 2GB
>> > memory, "this_frag->page.p" did not become highmem area.
>> > (We confirmed by transferring about 100MB of files)
>> >
>> > However, in the case of non-tso on an i.MX6 system with 2GB of
>> > memory, "this_frag->page.p" may become a highmem area.
>> > (Occurred with approximately 2MB of file transfer)
>> >
>> > For non-tso only, I do not know the reason why "this_frag-> page.p"
>> > in this driver shows highmem area.
>>
>> This worries me, since this driver does not set NETIF_F_HIGHDMA in its
>> features.
>>
>> No packet should be given to this driver with a highmem fragment
>>
>> Check is done in illegal_highdma() in net/core/dev.c
>
>This used to work.
>
>I suspect commit ec5f061564238892005257c83565a0b58ec79295
>("net: Kill link between CSUM and SG features.")
>
>added this bug.
>
>Can you try this hot fix :
>
>diff --git a/net/core/dev.c b/net/core/dev.c index
>ad5959e561166f445bdd9d7260652a338f74cfea..073b832b945257dba9ed47f4bf
>875605225effc9 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -2773,9 +2773,9 @@ static netdev_features_t harmonize_features(struct
>sk_buff *skb,
> 	if (skb->ip_summed != CHECKSUM_NONE &&
> 	    !can_checksum_protocol(features, type)) {
> 		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>-	} else if (illegal_highdma(skb->dev, skb)) {
>-		features &= ~NETIF_F_SG;
> 	}
>+	if (illegal_highdma(skb->dev, skb))
>+		features &= ~NETIF_F_SG;
>
> 	return features;
> }
>
>

I will double check your fix. Thanks.

And, if driver is to support highmem,  then we should add tso highmem support in net/core/tso.c,  do you think it is necessary ?

Re: [PATCH] net: fec: Fixed panic problem with non-tso

[Eric Dumazet]

On Thu, 2017-01-19 at 08:18 +0000, Andy Duan wrote:

> I will double check your fix. Thanks.
> 
> And, if driver is to support highmem,  then we should add tso highmem
> support in net/core/tso.c,  do you think it is necessary ?


Adding TSO highmem support would mean changing net/core/tso.c ABI and
thus changing all net/core/tso.c users.

Looks a lot of work to me.