* [PATCH] net: fec: Fixed panic problem with non-tso @ 2017-01-17 7:48 Yuusuke Ashiduka 2017-01-17 11:02 ` Andy Duan 2017-01-17 20:45 ` David Miller 0 siblings, 2 replies; 16+ messages in thread From: Yuusuke Ashiduka @ 2017-01-17 7:48 UTC (permalink / raw) To: fugang.duan; +Cc: netdev, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-17 7:48 [PATCH] net: fec: Fixed panic problem with non-tso Yuusuke Ashiduka @ 2017-01-17 11:02 ` Andy Duan 2017-01-18 3:12 ` Ashizuka, Yuusuke 2017-01-17 20:45 ` David Miller 1 sibling, 1 reply; 16+ messages in thread From: Andy Duan @ 2017-01-17 11:02 UTC (permalink / raw) To: Yuusuke Ashiduka; +Cc: netdev@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-17 11:02 ` Andy Duan @ 2017-01-18 3:12 ` Ashizuka, Yuusuke 2017-01-18 4:21 ` Eric Dumazet 0 siblings, 1 reply; 16+ messages in thread From: Ashizuka, Yuusuke @ 2017-01-18 3:12 UTC (permalink / raw) To: Andy Duan; +Cc: netdev@vger.kernel.org > -----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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-18 3:12 ` Ashizuka, Yuusuke @ 2017-01-18 4:21 ` Eric Dumazet 2017-01-18 4:35 ` Eric Dumazet 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2017-01-18 4:21 UTC (permalink / raw) To: Ashizuka, Yuusuke; +Cc: Andy Duan, netdev@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-18 4:21 ` Eric Dumazet @ 2017-01-18 4:35 ` Eric Dumazet 2017-01-18 18:18 ` Pravin Shelar 2017-01-19 8:18 ` [PATCH] net: fec: Fixed panic problem with non-tso Andy Duan 0 siblings, 2 replies; 16+ messages in thread From: Eric Dumazet @ 2017-01-18 4:35 UTC (permalink / raw) To: Ashizuka, Yuusuke; +Cc: Andy Duan, netdev@vger.kernel.org, Pravin B Shelar 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; } ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-18 4:35 ` Eric Dumazet @ 2017-01-18 18:18 ` Pravin Shelar 2017-01-18 19:51 ` Eric Dumazet 2017-01-19 8:18 ` [PATCH] net: fec: Fixed panic problem with non-tso Andy Duan 1 sibling, 1 reply; 16+ messages in thread From: Pravin Shelar @ 2017-01-18 18:18 UTC (permalink / raw) To: Eric Dumazet Cc: Ashizuka, Yuusuke, Andy Duan, netdev@vger.kernel.org, Pravin B 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-18 18:18 ` Pravin Shelar @ 2017-01-18 19:51 ` Eric Dumazet 2017-01-18 20:12 ` [PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA Eric Dumazet 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2017-01-18 19:51 UTC (permalink / raw) To: Pravin Shelar Cc: Ashizuka, Yuusuke, Andy Duan, netdev@vger.kernel.org, Pravin B Shelar 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA 2017-01-18 19:51 ` Eric Dumazet @ 2017-01-18 20:12 ` Eric Dumazet 2017-01-18 20:25 ` David Miller 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2017-01-18 20:12 UTC (permalink / raw) To: Pravin Shelar, David Miller Cc: Ashizuka, Yuusuke, Andy Duan, netdev@vger.kernel.org, Pravin B Shelar 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; } ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA 2017-01-18 20:12 ` [PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA Eric Dumazet @ 2017-01-18 20:25 ` David Miller 0 siblings, 0 replies; 16+ messages in thread From: David Miller @ 2017-01-18 20:25 UTC (permalink / raw) To: eric.dumazet; +Cc: pshelar, ashiduka, fugang.duan, netdev, pshelar 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-18 4:35 ` Eric Dumazet 2017-01-18 18:18 ` Pravin Shelar @ 2017-01-19 8:18 ` Andy Duan 2017-01-19 13:24 ` Eric Dumazet 1 sibling, 1 reply; 16+ messages in thread From: Andy Duan @ 2017-01-19 8:18 UTC (permalink / raw) To: Eric Dumazet, Ashizuka, Yuusuke; +Cc: netdev@vger.kernel.org, Pravin B Shelar 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 ? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-19 8:18 ` [PATCH] net: fec: Fixed panic problem with non-tso Andy Duan @ 2017-01-19 13:24 ` Eric Dumazet 0 siblings, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2017-01-19 13:24 UTC (permalink / raw) To: Andy Duan; +Cc: Ashizuka, Yuusuke, netdev@vger.kernel.org, Pravin B Shelar 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-17 7:48 [PATCH] net: fec: Fixed panic problem with non-tso Yuusuke Ashiduka 2017-01-17 11:02 ` Andy Duan @ 2017-01-17 20:45 ` David Miller 2017-01-18 3:20 ` Ashizuka, Yuusuke 2017-01-18 4:11 ` [PATCH v2] " Yuusuke Ashiduka 1 sibling, 2 replies; 16+ messages in thread From: David Miller @ 2017-01-17 20:45 UTC (permalink / raw) To: ashiduka; +Cc: fugang.duan, netdev 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH] net: fec: Fixed panic problem with non-tso 2017-01-17 20:45 ` David Miller @ 2017-01-18 3:20 ` Ashizuka, Yuusuke 2017-01-18 4:11 ` [PATCH v2] " Yuusuke Ashiduka 1 sibling, 0 replies; 16+ messages in thread From: Ashizuka, Yuusuke @ 2017-01-18 3:20 UTC (permalink / raw) To: David Miller; +Cc: fugang.duan@nxp.com, netdev@vger.kernel.org > -----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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] net: fec: Fixed panic problem with non-tso 2017-01-17 20:45 ` David Miller 2017-01-18 3:20 ` Ashizuka, Yuusuke @ 2017-01-18 4:11 ` Yuusuke Ashiduka 2017-01-18 5:02 ` Eric Dumazet 1 sibling, 1 reply; 16+ messages in thread From: Yuusuke Ashiduka @ 2017-01-18 4:11 UTC (permalink / raw) To: fugang.duan; +Cc: netdev, 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] net: fec: Fixed panic problem with non-tso 2017-01-18 4:11 ` [PATCH v2] " Yuusuke Ashiduka @ 2017-01-18 5:02 ` Eric Dumazet 2017-01-18 5:38 ` Andy Duan 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2017-01-18 5:02 UTC (permalink / raw) To: Yuusuke Ashiduka; +Cc: fugang.duan, netdev 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 :/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2] net: fec: Fixed panic problem with non-tso 2017-01-18 5:02 ` Eric Dumazet @ 2017-01-18 5:38 ` Andy Duan 0 siblings, 0 replies; 16+ messages in thread From: Andy Duan @ 2017-01-18 5:38 UTC (permalink / raw) To: Eric Dumazet, Yuusuke Ashiduka; +Cc: netdev@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-01-19 13:58 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-17 7:48 [PATCH] net: fec: Fixed panic problem with non-tso Yuusuke Ashiduka 2017-01-17 11:02 ` Andy Duan 2017-01-18 3:12 ` Ashizuka, Yuusuke 2017-01-18 4:21 ` Eric Dumazet 2017-01-18 4:35 ` Eric Dumazet 2017-01-18 18:18 ` Pravin Shelar 2017-01-18 19:51 ` Eric Dumazet 2017-01-18 20:12 ` [PATCH net] net: fix harmonize_features() vs NETIF_F_HIGHDMA Eric Dumazet 2017-01-18 20:25 ` David Miller 2017-01-19 8:18 ` [PATCH] net: fec: Fixed panic problem with non-tso Andy Duan 2017-01-19 13:24 ` Eric Dumazet 2017-01-17 20:45 ` David Miller 2017-01-18 3:20 ` Ashizuka, Yuusuke 2017-01-18 4:11 ` [PATCH v2] " Yuusuke Ashiduka 2017-01-18 5:02 ` Eric Dumazet 2017-01-18 5:38 ` Andy Duan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox