| Message ID | f29965fee23dc0de99b388373017efeffa1aac10.1405689937.git.cyrille.pitchen@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote: > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > --- > drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++- > drivers/net/ethernet/cadence/macb.h | 2 ++ > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > index 9bdcd1b..6acd6e2 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget) > > skb->protocol = eth_type_trans(skb, bp->dev); > skb_checksum_none_assert(skb); > + if (bp->dev->features & NETIF_F_RXCSUM) > + skb->ip_summed = CHECKSUM_UNNECESSARY; > Really ? If this is what you meant, this deserves a big and fat comment.
Le 18/07/2014 17:36, Eric Dumazet a écrit : > On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote: >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> >> --- >> drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++- >> drivers/net/ethernet/cadence/macb.h | 2 ++ >> 2 files changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c >> index 9bdcd1b..6acd6e2 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget) >> >> skb->protocol = eth_type_trans(skb, bp->dev); >> skb_checksum_none_assert(skb); >> + if (bp->dev->features & NETIF_F_RXCSUM) >> + skb->ip_summed = CHECKSUM_UNNECESSARY; >> > > > Really ? > > If this is what you meant, this deserves a big and fat comment. > > > Hi Eric, Isn't it the proper way to do? According to Cadence documentation about RX checksum offload: "If any of the checksums (IP, TCP or UDP) are verified incorrect by the GEM, the packet is discarded." If I understand, when RX checksum offload is enabled setting bit 24 of the Network Configuration Register, the driver only receives RX frames with correct checksum. Then it tells the kernel that there's no need to verify the checksum again in software. This is done setting skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the same in e1000_rx_checksum() function. Also bit 24 of the Network Configuration Register is updated by macb_open() and macb_set_features() so it always matches the state of NETIF_F_RXCSUM flag in dev->features, once the network interface is up. That's why I'd rather read from dev->features than read from register. Did I make a mistake? Is it the kind of comment you expect to be added? Regards, Cyrille
2014-07-22 9:27 GMT-07:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>: > Le 18/07/2014 17:36, Eric Dumazet a écrit : >> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote: >>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> >>> --- >>> drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++- >>> drivers/net/ethernet/cadence/macb.h | 2 ++ >>> 2 files changed, 30 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c >>> index 9bdcd1b..6acd6e2 100644 >>> --- a/drivers/net/ethernet/cadence/macb.c >>> +++ b/drivers/net/ethernet/cadence/macb.c >>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget) >>> >>> skb->protocol = eth_type_trans(skb, bp->dev); >>> skb_checksum_none_assert(skb); >>> + if (bp->dev->features & NETIF_F_RXCSUM) >>> + skb->ip_summed = CHECKSUM_UNNECESSARY; >>> >> >> >> Really ? >> >> If this is what you meant, this deserves a big and fat comment. >> >> >> > Hi Eric, > > Isn't it the proper way to do? According to Cadence documentation about RX checksum offload: > "If any of the checksums (IP, TCP or UDP) are verified incorrect by the GEM, the packet is discarded." > > If I understand, when RX checksum offload is enabled setting bit 24 of the Network Configuration Register, the driver only receives RX frames with correct checksum. Then it tells the kernel that there's no need to verify the checksum again in software. This is done setting skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the same in e1000_rx_checksum() function. This is not the same thing as what the e1000 driver is doing, the e1000 driver is checking a per-packet status mask which tells it whether a given packet had its protocol level checksum computed by the HW. I think there might be some slight confusion here between the Ethernet frame Frame Control Checksum which is at the Ethernet level, and will (with most adapters not supporting RX_FCS_ERR) cause the Ethernet controller to drop frames at the MAC level, and the higher level protocol checksums such as the TCP or UDP checksum. > > Also bit 24 of the Network Configuration Register is updated by macb_open() and macb_set_features() so it always matches the state of NETIF_F_RXCSUM flag in dev->features, once the network interface is up. That's why I'd rather read from dev->features than read from register. I don't think the Cadence GEM Ethernet controller is any different than other hardware out there, there must be a per-packet status bit which tells whether the TCP/UDP or other protocol checksum was successfully validated by the hardware. One of the reason for that, is that depending on the type of traffic your receive (e.g: ARP), there might be no protocol-level checksum at all in the packet, and the hardware should know about that. > > Did I make a mistake? Is it the kind of comment you expect to be added? > > Regards, > > Cyrille > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, 2014-07-22 at 18:27 +0200, Cyrille Pitchen wrote: > Le 18/07/2014 17:36, Eric Dumazet a écrit : > > On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote: > >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > >> --- > >> drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++- > >> drivers/net/ethernet/cadence/macb.h | 2 ++ > >> 2 files changed, 30 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > >> index 9bdcd1b..6acd6e2 100644 > >> --- a/drivers/net/ethernet/cadence/macb.c > >> +++ b/drivers/net/ethernet/cadence/macb.c > >> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget) > >> > >> skb->protocol = eth_type_trans(skb, bp->dev); > >> skb_checksum_none_assert(skb); > >> + if (bp->dev->features & NETIF_F_RXCSUM) > >> + skb->ip_summed = CHECKSUM_UNNECESSARY; > >> > > > > > > Really ? > > > > If this is what you meant, this deserves a big and fat comment. > > > > > > > Hi Eric, > > Isn't it the proper way to do? According to Cadence documentation > about RX checksum offload: > "If any of the checksums (IP, TCP or UDP) are verified incorrect by > the GEM, the packet is discarded." > > If I understand, when RX checksum offload is enabled setting bit 24 of > the Network Configuration Register, the driver only receives RX frames > with correct checksum. Then it tells the kernel that there's no need > to verify the checksum again in software. This is done setting > skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the > same in e1000_rx_checksum() function. > > Also bit 24 of the Network Configuration Register is updated by > macb_open() and macb_set_features() so it always matches the state of > NETIF_F_RXCSUM flag in dev->features, once the network interface is > up. That's why I'd rather read from dev->features than read from > register. > > Did I make a mistake? Is it the kind of comment you expect to be > added? > > Regards, Usually, frames are not discarded, and a bit is present in rx descriptor to tell us if (TCP) checksum was validated by the NIC We want tcpdump to get a copy of corrupted TCP frames, even if TCP stack drops them later. If this NIC drops incorrect frames, I am afraid we cant use this RX offload at all.
Le 23/07/2014 00:58, Eric Dumazet a écrit : > On Tue, 2014-07-22 at 18:27 +0200, Cyrille Pitchen wrote: >> Le 18/07/2014 17:36, Eric Dumazet a écrit : >>> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote: >>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> >>>> --- >>>> drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++- >>>> drivers/net/ethernet/cadence/macb.h | 2 ++ >>>> 2 files changed, 30 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c >>>> index 9bdcd1b..6acd6e2 100644 >>>> --- a/drivers/net/ethernet/cadence/macb.c >>>> +++ b/drivers/net/ethernet/cadence/macb.c >>>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget) >>>> >>>> skb->protocol = eth_type_trans(skb, bp->dev); >>>> skb_checksum_none_assert(skb); >>>> + if (bp->dev->features & NETIF_F_RXCSUM) >>>> + skb->ip_summed = CHECKSUM_UNNECESSARY; >>>> >>> >>> >>> Really ? >>> >>> If this is what you meant, this deserves a big and fat comment. >>> >>> >>> >> Hi Eric, >> >> Isn't it the proper way to do? According to Cadence documentation >> about RX checksum offload: >> "If any of the checksums (IP, TCP or UDP) are verified incorrect by >> the GEM, the packet is discarded." >> >> If I understand, when RX checksum offload is enabled setting bit 24 of >> the Network Configuration Register, the driver only receives RX frames >> with correct checksum. Then it tells the kernel that there's no need >> to verify the checksum again in software. This is done setting >> skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the >> same in e1000_rx_checksum() function. >> >> Also bit 24 of the Network Configuration Register is updated by >> macb_open() and macb_set_features() so it always matches the state of >> NETIF_F_RXCSUM flag in dev->features, once the network interface is >> up. That's why I'd rather read from dev->features than read from >> register. >> >> Did I make a mistake? Is it the kind of comment you expect to be >> added? >> >> Regards, > > Usually, frames are not discarded, and a bit is present in rx descriptor > to tell us if (TCP) checksum was validated by the NIC > > We want tcpdump to get a copy of corrupted TCP frames, even if TCP stack > drops them later. > > If this NIC drops incorrect frames, I am afraid we cant use this RX > offload at all. > > > OK thanks for your comments. In the next series of patches I'll allow RX checksum offload only when promiscuous mode is disabled. Also I'll check the status returned by the GEM through the buffer descriptor. Regards, Cyrille
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 9bdcd1b..6acd6e2 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget) skb->protocol = eth_type_trans(skb, bp->dev); skb_checksum_none_assert(skb); + if (bp->dev->features & NETIF_F_RXCSUM) + skb->ip_summed = CHECKSUM_UNNECESSARY; bp->stats.rx_packets++; bp->stats.rx_bytes += skb->len; @@ -1506,6 +1508,18 @@ static u32 macb_dbw(struct macb *bp) } /* + * Configure the RX checksum offload + */ +static u32 macb_rxcsum(struct macb *bp) +{ + if (macb_is_gem(bp) && + (bp->dev->features & NETIF_F_RXCSUM)) + return GEM_BIT(RXCOEN); + + return 0; +} + +/* * Configure the receive DMA engine * - use the correct receive buffer size * - set best burst length for DMA operations @@ -1550,6 +1564,7 @@ static void macb_init_hw(struct macb *bp) if (!(bp->dev->flags & IFF_BROADCAST)) config |= MACB_BIT(NBC); /* No BroadCast */ config |= macb_dbw(bp); + config |= macb_rxcsum(bp); macb_writel(bp, NCFGR, config); bp->speed = SPEED_10; bp->duplex = DUPLEX_HALF; @@ -1945,6 +1960,18 @@ static int macb_set_features(struct net_device *netdev, gem_writel(bp, DMACFG, dmacfg); } + /* RX checksum offload */ + if ((changed & NETIF_F_RXCSUM) && macb_is_gem(bp)) { + u32 netcfg; + + netcfg = gem_readl(bp, NCFGR); + if (features & NETIF_F_RXCSUM) + netcfg |= GEM_BIT(RXCOEN); + else + netcfg &= ~GEM_BIT(RXCOEN); + gem_writel(bp, NCFGR, netcfg); + } + return 0; } @@ -2146,7 +2173,7 @@ static int __init macb_probe(struct platform_device *pdev) dev->hw_features = NETIF_F_SG; if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) { /* Checksum offload is only available on packet buffer design */ - dev->hw_features |= NETIF_F_HW_CSUM; + dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM; } if (bp->caps & MACB_CAPS_SG_DISABLED) dev->hw_features &= ~NETIF_F_SG; diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 7bf8285..3678b83 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -164,6 +164,8 @@ #define GEM_CLK_SIZE 3 #define GEM_DBW_OFFSET 21 #define GEM_DBW_SIZE 2 +#define GEM_RXCOEN_OFFSET 24 +#define GEM_RXCOEN_SIZE 1 /* Constants for data bus width. */ #define GEM_DBW32 0
Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> --- drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++- drivers/net/ethernet/cadence/macb.h | 2 ++ 2 files changed, 30 insertions(+), 1 deletion(-)