Re: [patch net / RFC] net: fec: increase frame size limitation to actually available buffer

2016-11-30 Thread Zefir Kurtisi 
On 11/29/2016 07:35 PM, Nikita Yushchenko wrote:
> Fec driver uses Rx buffers of 2k, but programs hardware to limit
> incoming frames to 1522 bytes. This raises issues when FEC device
> is used with DSA (since DSA tag can make frame larger), and also
> disallows manual sending and receiving larger frames.
> 
> This patch removes the limitation, allowing Rx size up to entire buffer.
> At the same time possible Tx size is increased as well, because hardware
> uses the same register field to limit Rx and Tx.
> 
> Signed-off-by: Nikita Yushchenko 
> ---

I recently did similar changes to the freescale/gianfar to prevent fragmentation
of (E)DSA frames [1].

With the fragmentation kicking in, I noticed a bug in the scatter-gather logic
which caused the frame-size of fragmented frames to be reported wrong [2]. Not
sure if the fec driver has SG capabilities and if this issue also applies, you
might want to double-check.


Cheers,
Zefir


[1] http://marc.info/?l=linux-netdev&m=147187438313519&w=2
[2] http://marc.info/?l=linux-netdev&m=147187430213484&w=2

RE: [patch net / RFC] net: fec: increase frame size limitation to actually available buffer

2016-11-30 Thread Andy Duan 
From: Nikita Yushchenko  Sent: Wednesday, 
November 30, 2016 2:35 AM
 >To: David S. Miller ; Andy Duan
 >; Troy Kisky ;
 >Andrew Lunn ; Eric Nelson ; Philippe
 >Reynes ; Johannes Berg ;
 >net...@vger.kernel.org
 >Cc: Chris Healy ; Fabio Estevam
 >; linux-kernel@vger.kernel.org; Nikita
 >Yushchenko 
 >Subject: [patch net / RFC] net: fec: increase frame size limitation to 
 >actually
 >available buffer
 >
 >Fec driver uses Rx buffers of 2k, but programs hardware to limit incoming
 >frames to 1522 bytes. This raises issues when FEC device is used with DSA
 >(since DSA tag can make frame larger), and also disallows manual sending and
 >receiving larger frames.
 >
 >This patch removes the limitation, allowing Rx size up to entire buffer.
 >At the same time possible Tx size is increased as well, because hardware uses
 >the same register field to limit Rx and Tx.
 >
 >Signed-off-by: Nikita Yushchenko 
 >---
 > drivers/net/ethernet/freescale/fec_main.c | 33 +++
 >
 > 1 file changed, 12 insertions(+), 21 deletions(-)
 >
 >diff --git a/drivers/net/ethernet/freescale/fec_main.c
 >b/drivers/net/ethernet/freescale/fec_main.c
 >index 73ac35780611..c09789a71024 100644
 >--- a/drivers/net/ethernet/freescale/fec_main.c
 >+++ b/drivers/net/ethernet/freescale/fec_main.c
 >@@ -171,30 +171,12 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet
 >MAC address");  #endif  #endif /* CONFIG_M5272 */
 >
 >-/* The FEC stores dest/src/type/vlan, data, and checksum for receive
 >packets.
 >- */
 >-#define PKT_MAXBUF_SIZE  1522
 >-#define PKT_MINBUF_SIZE  64
 >-#define PKT_MAXBLR_SIZE  1536
 >-
 > /* FEC receive acceleration */
 > #define FEC_RACC_IPDIS   (1 << 1)
 > #define FEC_RACC_PRODIS  (1 << 2)
 > #define FEC_RACC_SHIFT16 BIT(7)
 > #define FEC_RACC_OPTIONS (FEC_RACC_IPDIS | FEC_RACC_PRODIS)
 >
 >-/*
 >- * The 5270/5271/5280/5282/532x RX control register also contains maximum
 >frame
 >- * size bits. Other FEC hardware does not, so we need to take that into
 >- * account when setting it.
 >- */
 >-#if defined(CONFIG_M523x) || defined(CONFIG_M527x) ||
 >defined(CONFIG_M528x) || \
 >-defined(CONFIG_M520x) || defined(CONFIG_M532x) ||
 >defined(CONFIG_ARM)
 >-#define  OPT_FRAME_SIZE  (PKT_MAXBUF_SIZE << 16)
 >-#else
 >-#define  OPT_FRAME_SIZE  0
 >-#endif
 >-
 > /* FEC MII MMFR bits definition */
 > #define FEC_MMFR_ST  (1 << 30)
 > #define FEC_MMFR_OP_READ (2 << 28)
 >@@ -847,7 +829,8 @@ static void fec_enet_enable_ring(struct net_device
 >*ndev)
 >  for (i = 0; i < fep->num_rx_queues; i++) {
 >  rxq = fep->rx_queue[i];
 >  writel(rxq->bd.dma, fep->hwp + FEC_R_DES_START(i));
 >- writel(PKT_MAXBLR_SIZE, fep->hwp + FEC_R_BUFF_SIZE(i));
 >+ writel(FEC_ENET_RX_FRSIZE - fep->rx_align,
 >+fep->hwp + FEC_R_BUFF_SIZE(i));
 >
 >  /* enable DMA1/2 */
 >  if (i)
 >@@ -895,9 +878,17 @@ fec_restart(struct net_device *ndev)
 >  struct fec_enet_private *fep = netdev_priv(ndev);
 >  u32 val;
 >  u32 temp_mac[2];
 >- u32 rcntl = OPT_FRAME_SIZE | 0x04;
 >+ u32 rcntl = 0x04;
 >  u32 ecntl = 0x2; /* ETHEREN */
 >
 >+ /* The 5270/5271/5280/5282/532x RX control register also contains
 >+  * maximum frame * size bits. Other FEC hardware does not.
 >+  */
 >+#if defined(CONFIG_M523x) || defined(CONFIG_M527x) ||
 >defined(CONFIG_M528x) || \
 >+defined(CONFIG_M520x) || defined(CONFIG_M532x) ||
 >defined(CONFIG_ARM)
 >+ rcntl |= (FEC_ENET_RX_FRSIZE - fep->rx_align) << 16; #endif
 >+
 >  /* Whack a reset.  We should wait for this.
 >   * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
 >   * instead of reset MAC itself.
 >@@ -953,7 +944,7 @@ fec_restart(struct net_device *ndev)
 >  else
 >  val &= ~FEC_RACC_OPTIONS;
 >  writel(val, fep->hwp + FEC_RACC);
 >- writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
 >+ writel(FEC_ENET_RX_FRSIZE - fep->rx_align, fep->hwp +
 >FEC_FTRL);

By the patch itself, it seems fine except MRBRn must be evenly divisible by 64.
But I think it is not necessary since the driver don't support jumbo frame.

 >  }
 > #endif
 >
 >--
 >2.1.4

Re: [patch net / RFC] net: fec: increase frame size limitation to actually available buffer

2016-11-30 Thread Toshiaki Makita 
On 2016/11/30 15:36, Nikita Yushchenko wrote:
>> But I think it is not necessary since the driver don't support jumbo frame.
> 
> Hardcoded 1522 raises two separate issues.
> 
> (1) When DSA is in use, frames processed by FEC chip contain DSA tag and
> thus can be larger than hardcoded limit of 1522. This issue is not
> FEC-specific, any driver that hardcodes maximum frame size to 1522 (many
> do) will have this issue if used with DSA.
> 
> Clean solution for this must take into account that difference between
> MTU and max frame size is no longer known at compile time. Actually this
> is the case even without DSA, due to VLANs: max frame size is (MTU + 18)
> without VLANs, but (MTU + 22) with VLANs. However currently drivers tend
> to ignore this and hardcode 22.  With DSA, 22 is not enough, need to add
> switch-specific tag size to that.
> 
> Not yet sure how to handle this. DSA-specific API to find out tag size
> could be added, but generic solution should handle all cases of dynamic
> difference between MTU and max frame size, not only DSA.

BTW I'm trying to introduce envelope frames to solve this kind of problems.
http://marc.info/?t=14749669155&r=1&w=2
http://marc.info/?t=14749669153&r=1&w=2
http://marc.info/?t=14749669152&r=1&w=2
http://marc.info/?t=14749669154&r=1&w=2
http://marc.info/?t=14749669151&r=1&w=2

It needs jumbo frame support of NICs though.

Regards,
Toshiaki Makita

RE: [patch net / RFC] net: fec: increase frame size limitation to actually available buffer

2016-11-29 Thread Andy Duan 
From: Nikita Yushchenko  Sent: Wednesday, 
November 30, 2016 2:36 PM
 >To: Andy Duan ; David S. Miller
 >; Troy Kisky ;
 >Andrew Lunn ; Eric Nelson ; Philippe
 >Reynes ; Johannes Berg ;
 >net...@vger.kernel.org
 >Cc: Chris Healy ; Fabio Estevam
 >; linux-kernel@vger.kernel.org
 >Subject: Re: [patch net / RFC] net: fec: increase frame size limitation to
 >actually available buffer
 >
 >> But I think it is not necessary since the driver don't support jumbo frame.
 >
 >Hardcoded 1522 raises two separate issues.
 >
 >(1) When DSA is in use, frames processed by FEC chip contain DSA tag and
 >thus can be larger than hardcoded limit of 1522. This issue is not 
 >FEC-specific,
 >any driver that hardcodes maximum frame size to 1522 (many
 >do) will have this issue if used with DSA.
 >
 >Clean solution for this must take into account that difference between MTU
 >and max frame size is no longer known at compile time. Actually this is the
 >case even without DSA, due to VLANs: max frame size is (MTU + 18) without
 >VLANs, but (MTU + 22) with VLANs. However currently drivers tend to ignore
 >this and hardcode 22.  With DSA, 22 is not enough, need to add switch-
 >specific tag size to that.
 >
 >Not yet sure how to handle this. DSA-specific API to find out tag size could 
 >be
 >added, but generic solution should handle all cases of dynamic difference
 >between MTU and max frame size, not only DSA.
 >
 >
 >(2) There is some demand to use larger frames for optimization purposes.
 >
 >FEC register fields that limit frame size are 14-bit, thus allowing frames up 
 >to
 >(4k-1). I'm about to prepare a larger patch:
 >- add ndo_change_mtu handler, allowing MTU up to (4k - overhead),
 >- set MAX_FL / TRUNC_FL based on configured MTU,
 >- if necessary, do buffer reallocation with larger buffers.
 >
 >Is this suitable for upstreaming?
 >Is there any policy related to handling larger frames?

Of course, welcome to upstream the jumbo frame patches, but hope to also add 
the transmit jumbo frame, not only receive path, which is helpful for testing 
with two boards connection.
And, some notes need you to care:
- the maximum jumbo frame should consider the fifo size. Different chip has 
different fifo size. Like i.MX53 tx and rx share one fifo, i.mx6q/dl/sl have 
separate 4k fifo for tx and rx, i.mx6sx/i.mx7x have separate 8k fifo for tx and 
rx.
- rx fifo watermark to generate pause frame in busy loading system to avoid 
fifo overrun. In general,  little pause frames bring better performance, mass 
of pause frames cause worse performance.


Regards,
Andy

Re: [patch net / RFC] net: fec: increase frame size limitation to actually available buffer

2016-11-29 Thread Nikita Yushchenko 
> But I think it is not necessary since the driver don't support jumbo frame.

Hardcoded 1522 raises two separate issues.

(1) When DSA is in use, frames processed by FEC chip contain DSA tag and
thus can be larger than hardcoded limit of 1522. This issue is not
FEC-specific, any driver that hardcodes maximum frame size to 1522 (many
do) will have this issue if used with DSA.

Clean solution for this must take into account that difference between
MTU and max frame size is no longer known at compile time. Actually this
is the case even without DSA, due to VLANs: max frame size is (MTU + 18)
without VLANs, but (MTU + 22) with VLANs. However currently drivers tend
to ignore this and hardcode 22.  With DSA, 22 is not enough, need to add
switch-specific tag size to that.

Not yet sure how to handle this. DSA-specific API to find out tag size
could be added, but generic solution should handle all cases of dynamic
difference between MTU and max frame size, not only DSA.


(2) There is some demand to use larger frames for optimization purposes.

FEC register fields that limit frame size are 14-bit, thus allowing
frames up to (4k-1). I'm about to prepare a larger patch:
- add ndo_change_mtu handler, allowing MTU up to (4k - overhead),
- set MAX_FL / TRUNC_FL based on configured MTU,
- if necessary, do buffer reallocation with larger buffers.

Is this suitable for upstreaming?
Is there any policy related to handling larger frames?

[patch net / RFC] net: fec: increase frame size limitation to actually available buffer

2016-11-29 Thread Nikita Yushchenko 
Fec driver uses Rx buffers of 2k, but programs hardware to limit
incoming frames to 1522 bytes. This raises issues when FEC device
is used with DSA (since DSA tag can make frame larger), and also
disallows manual sending and receiving larger frames.

This patch removes the limitation, allowing Rx size up to entire buffer.
At the same time possible Tx size is increased as well, because hardware
uses the same register field to limit Rx and Tx.

Signed-off-by: Nikita Yushchenko 
---
 drivers/net/ethernet/freescale/fec_main.c | 33 +++
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 73ac35780611..c09789a71024 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -171,30 +171,12 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #endif
 #endif /* CONFIG_M5272 */
 
-/* The FEC stores dest/src/type/vlan, data, and checksum for receive packets.
- */
-#define PKT_MAXBUF_SIZE1522
-#define PKT_MINBUF_SIZE64
-#define PKT_MAXBLR_SIZE1536
-
 /* FEC receive acceleration */
 #define FEC_RACC_IPDIS (1 << 1)
 #define FEC_RACC_PRODIS(1 << 2)
 #define FEC_RACC_SHIFT16   BIT(7)
 #define FEC_RACC_OPTIONS   (FEC_RACC_IPDIS | FEC_RACC_PRODIS)
 
-/*
- * The 5270/5271/5280/5282/532x RX control register also contains maximum frame
- * size bits. Other FEC hardware does not, so we need to take that into
- * account when setting it.
- */
-#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || 
\
-defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM)
-#defineOPT_FRAME_SIZE  (PKT_MAXBUF_SIZE << 16)
-#else
-#defineOPT_FRAME_SIZE  0
-#endif
-
 /* FEC MII MMFR bits definition */
 #define FEC_MMFR_ST(1 << 30)
 #define FEC_MMFR_OP_READ   (2 << 28)
@@ -847,7 +829,8 @@ static void fec_enet_enable_ring(struct net_device *ndev)
for (i = 0; i < fep->num_rx_queues; i++) {
rxq = fep->rx_queue[i];
writel(rxq->bd.dma, fep->hwp + FEC_R_DES_START(i));
-   writel(PKT_MAXBLR_SIZE, fep->hwp + FEC_R_BUFF_SIZE(i));
+   writel(FEC_ENET_RX_FRSIZE - fep->rx_align,
+  fep->hwp + FEC_R_BUFF_SIZE(i));
 
/* enable DMA1/2 */
if (i)
@@ -895,9 +878,17 @@ fec_restart(struct net_device *ndev)
struct fec_enet_private *fep = netdev_priv(ndev);
u32 val;
u32 temp_mac[2];
-   u32 rcntl = OPT_FRAME_SIZE | 0x04;
+   u32 rcntl = 0x04;
u32 ecntl = 0x2; /* ETHEREN */
 
+   /* The 5270/5271/5280/5282/532x RX control register also contains
+* maximum frame * size bits. Other FEC hardware does not.
+*/
+#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || 
\
+defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARM)
+   rcntl |= (FEC_ENET_RX_FRSIZE - fep->rx_align) << 16;
+#endif
+
/* Whack a reset.  We should wait for this.
 * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
 * instead of reset MAC itself.
@@ -953,7 +944,7 @@ fec_restart(struct net_device *ndev)
else
val &= ~FEC_RACC_OPTIONS;
writel(val, fep->hwp + FEC_RACC);
-   writel(PKT_MAXBUF_SIZE, fep->hwp + FEC_FTRL);
+   writel(FEC_ENET_RX_FRSIZE - fep->rx_align, fep->hwp + FEC_FTRL);
}
 #endif
 
-- 
2.1.4