2013-05-08 09:33:48

by wilocity.git

[permalink] [raw]
Subject: [PATCH 1/3] wil6210: TCP/UDP checksum offloading support

From: Erez Kirshenbaum <[email protected]>

Add TCP/UDP checksum hardware offloading, configure hw and set the corresponding offloading bits in the
DMA descriptors.

Signed-off-by: Erez Kirshenbaum <[email protected]>
Signed-off-by: Wilocity Git <[email protected]>
---
drivers/net/wireless/ath/wil6210/netdev.c | 24 ++++++++++
drivers/net/wireless/ath/wil6210/txrx.c | 77 +++++++++++++++++++++++++++++--
drivers/net/wireless/ath/wil6210/txrx.h | 28 +++++++++--
drivers/net/wireless/ath/wil6210/wmi.c | 8 ++++
4 files changed, 130 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
index 098a8ec..4f2a61f 100644
--- a/drivers/net/wireless/ath/wil6210/netdev.c
+++ b/drivers/net/wireless/ath/wil6210/netdev.c
@@ -32,12 +32,34 @@ static int wil_stop(struct net_device *ndev)
return wil_down(wil);
}

+static netdev_features_t wil_fix_features(struct net_device *netdev,
+ netdev_features_t features)
+{
+ return features;
+}
+
+static int wil_set_features(struct net_device *netdev,
+ netdev_features_t features)
+{
+ struct wil6210_priv *wil = ndev_to_wil(netdev);
+ netdev_features_t changed = features ^ netdev->features;
+
+ wil_info(wil, "wil_set_features: %x\n", (unsigned int)features);
+ if (changed & NETIF_F_HW_CSUM)
+ wil_info(wil, "Tx checksum offloading changed\n");
+ else if (changed & NETIF_F_RXCSUM)
+ wil_info(wil, "Rx checksum offloading changed\n");
+ return 0;
+}
+
static const struct net_device_ops wil_netdev_ops = {
.ndo_open = wil_open,
.ndo_stop = wil_stop,
.ndo_start_xmit = wil_start_xmit,
.ndo_set_mac_address = eth_mac_addr,
.ndo_validate_addr = eth_validate_addr,
+ .ndo_fix_features = wil_fix_features,
+ .ndo_set_features = wil_set_features,
};

void *wil_if_alloc(struct device *dev, void __iomem *csr)
@@ -78,6 +100,8 @@ void *wil_if_alloc(struct device *dev, void __iomem *csr)

ndev->netdev_ops = &wil_netdev_ops;
ndev->ieee80211_ptr = wdev;
+ ndev->hw_features = NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+ ndev->features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
SET_NETDEV_DEV(ndev, wiphy_dev(wdev->wiphy));
wdev->netdev = ndev;

diff --git a/drivers/net/wireless/ath/wil6210/txrx.c b/drivers/net/wireless/ath/wil6210/txrx.c
index 7970245..3f44a6c 100644
--- a/drivers/net/wireless/ath/wil6210/txrx.c
+++ b/drivers/net/wireless/ath/wil6210/txrx.c
@@ -18,6 +18,9 @@
#include <net/ieee80211_radiotap.h>
#include <linux/if_arp.h>
#include <linux/moduleparam.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <net/ipv6.h>

#include "wil6210.h"
#include "wmi.h"
@@ -391,7 +394,16 @@ static struct sk_buff *wil_vring_reap_rx(struct wil6210_priv *wil,
*/
wil_swap_ethaddr(skb->data);
}
-
+ /* Check checksum-offload status */
+ if (ndev->features & NETIF_F_RXCSUM) {
+ if (d->dma.status & RX_DMA_STATUS_L4_IDENT) {
+ /* L4 protocol identified, csum calculated */
+ if ((d->dma.error & RX_DMA_ERROR_L4_ERR) == 0)
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ else
+ wil_err(wil, "Incorrect checksum reported\n");
+ }
+ }
return skb;
}

@@ -605,9 +617,7 @@ static int wil_tx_desc_map(volatile struct vring_tx_desc *d,
{
d->dma.addr_low = lower_32_bits(pa);
d->dma.addr_high = (u16)upper_32_bits(pa);
- d->dma.ip_length = 0;
- /* 0..6: mac_length; 7:ip_version 0-IP6 1-IP4*/
- d->dma.b11 = 0/*14 | BIT(7)*/;
+ d->dma.offload_cfg = 0;
d->dma.error = 0;
d->dma.status = 0; /* BIT(0) should be 0 for HW_OWNED */
d->dma.length = len;
@@ -630,6 +640,7 @@ static int wil_tx_vring(struct wil6210_priv *wil, struct vring *vring,
struct sk_buff *skb)
{
struct device *dev = wil_to_dev(wil);
+ struct net_device *ndev = wil_to_ndev(wil);
volatile struct vring_tx_desc *d;
u32 swhead = vring->swhead;
int avail = wil_vring_avail_tx(vring);
@@ -638,6 +649,8 @@ static int wil_tx_vring(struct wil6210_priv *wil, struct vring *vring,
int vring_index = vring - wil->vring_tx;
uint i = swhead;
dma_addr_t pa;
+ int is_ip4 = 0, is_ip6 = 0, is_tcp = 0, is_udp = 0;
+

wil_dbg_txrx(wil, "%s()\n", __func__);

@@ -665,6 +678,62 @@ static int wil_tx_vring(struct wil6210_priv *wil, struct vring *vring,
return -EINVAL;
/* 1-st segment */
wil_tx_desc_map(d, pa, skb_headlen(skb));
+ /*
+ * Process offloading
+ */
+ if ((skb->ip_summed == CHECKSUM_PARTIAL) &&
+ (ndev->features & NETIF_F_HW_CSUM)) {
+ if (skb->protocol == htons(ETH_P_IP)) {
+ is_ip4 = 1;
+ if (ip_hdr(skb)->protocol == IPPROTO_TCP)
+ is_tcp = 1;
+ else if (ip_hdr(skb)->protocol == IPPROTO_UDP)
+ is_udp = 1;
+ } else if (skb->protocol == htons(ETH_P_IPV6)) {
+ unsigned int offset = 0;
+ int ipv6hdr = ipv6_find_hdr(skb,
+ &offset, -1, NULL, NULL);
+ is_ip6 = 1;
+ if (ipv6hdr == NEXTHDR_TCP)
+ is_tcp = 1;
+ else if (ipv6hdr == NEXTHDR_UDP)
+ is_udp = 1;
+ }
+ }
+
+
+ if (is_ip4 || is_ip6) {
+ if (is_ip4)
+ d->dma.offload_cfg |=
+ BIT(DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_POS);
+ d->dma.offload_cfg |=
+ (skb_network_header_len(skb) &
+ DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_MSK);
+ d->dma.offload_cfg |=
+ (0x0e << DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_POS);
+ if (is_tcp || is_udp) {
+ /* Enable TCP/UDP checksum */
+ d->dma.d0 |=
+ BIT(DMA_CFG_DESC_TX_0_TCP_UDP_CHECKSUM_EN_POS);
+ /* Calculate pseudo-header */
+ d->dma.d0 |=
+ BIT(DMA_CFG_DESC_TX_0_PSEUDO_HEADER_CALC_EN_POS);
+ if (is_tcp) {
+ d->dma.d0 |=
+ (2 << DMA_CFG_DESC_TX_0_L4_TYPE_POS);
+ /* L4 header len: TCP header length */
+ d->dma.d0 |=
+ (tcp_hdrlen(skb) &
+ DMA_CFG_DESC_TX_0_L4_LENGTH_MSK);
+ } else {
+ /* L4 header len: UDP header length */
+ d->dma.d0 |=
+ (sizeof(struct udphdr) &
+ DMA_CFG_DESC_TX_0_L4_LENGTH_MSK);
+ }
+ }
+ }
+
d->mac.d[2] |= ((nr_frags + 1) <<
MAC_CFG_DESC_TX_2_NUM_OF_DESCRIPTORS_POS);
/* middle segments */
diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h
index adef12f..da22a93 100644
--- a/drivers/net/wireless/ath/wil6210/txrx.h
+++ b/drivers/net/wireless/ath/wil6210/txrx.h
@@ -209,7 +209,21 @@ struct vring_tx_mac {

#define DMA_CFG_DESC_TX_0_L4_TYPE_POS 30
#define DMA_CFG_DESC_TX_0_L4_TYPE_LEN 2
-#define DMA_CFG_DESC_TX_0_L4_TYPE_MSK 0xC0000000
+#define DMA_CFG_DESC_TX_0_L4_TYPE_MSK 0xC0000000 /* L4 type: 0-UDP, 2-TCP */
+
+/* TX DMA Dword 2 */
+/* Offload bits in offload_cfg field */
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_POS 0
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_LEN 8
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_MSK 0x00FF /* IP hdr len */
+
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_POS 8
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_LEN 7
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_MSK 0x7F00 /* MAC hdr len */
+
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_POS 15
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_LEN 1
+#define DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_MSK 0x8000 /* 1-IPv4, 0-IPv6 */


#define TX_DMA_STATUS_DU BIT(0)
@@ -218,8 +232,7 @@ struct vring_tx_dma {
u32 d0;
u32 addr_low;
u16 addr_high;
- u8 ip_length;
- u8 b11; /* 0..6: mac_length; 7:ip_version */
+ u16 offload_cfg;
u8 error; /* 0..2: err; 3..7: reserved; */
u8 status; /* 0: used; 1..7; reserved */
u16 length;
@@ -309,8 +322,17 @@ struct vring_rx_mac {

#define RX_DMA_D0_CMD_DMA_IT BIT(10)

+/* Error field, offload bits */
+#define RX_DMA_ERROR_L3_ERR BIT(4)
+#define RX_DMA_ERROR_L4_ERR BIT(5)
+
+
+/* Status field */
#define RX_DMA_STATUS_DU BIT(0)
#define RX_DMA_STATUS_ERROR BIT(2)
+
+#define RX_DMA_STATUS_L3_IDENT BIT(4)
+#define RX_DMA_STATUS_L4_IDENT BIT(5)
#define RX_DMA_STATUS_PHY_INFO BIT(6)

struct vring_rx_dma {
diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index 45b04e3..dc0aa8a 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -932,6 +932,14 @@ int wmi_rx_chain_add(struct wil6210_priv *wil, struct vring *vring)
struct wmi_cfg_rx_chain_done_event evt;
} __packed evt;
int rc;
+ u8 csum_enabled = 0;
+
+ /* Initialize offload. Linux always calculates IP checksum */
+ cmd.l3_l4_ctrl |= (0 << L3_L4_CTRL_IPV4_CHECKSUM_EN_POS);
+ /* Enable/disable TCP/UDP checksum */
+ if (ndev->features & NETIF_F_RXCSUM)
+ csum_enabled = 1;
+ cmd.l3_l4_ctrl |= (csum_enabled << L3_L4_CTRL_TCPIP_CHECKSUM_EN_POS);

if (wdev->iftype == NL80211_IFTYPE_MONITOR) {
struct ieee80211_channel *ch = wdev->preset_chandef.chan;
--
1.7.11.7



2013-05-08 12:06:28

by Vladimir Kondratiev

[permalink] [raw]
Subject: Re: [PATCH 1/3] wil6210: TCP/UDP checksum offloading support

On Wednesday, May 08, 2013 11:32:51 AM [email protected] wrote:
> From: Erez Kirshenbaum <[email protected]>
>
> Add TCP/UDP checksum hardware offloading, configure hw and set the corresponding offloading bits in the
> DMA descriptors.
>
> Signed-off-by: Erez Kirshenbaum <[email protected]>
> Signed-off-by: Wilocity Git <[email protected]>

Please run your patches through 'checkpatch.pl --strict' and fix reported problems

> ---
> drivers/net/wireless/ath/wil6210/netdev.c | 24 ++++++++++
> drivers/net/wireless/ath/wil6210/txrx.c | 77 +++++++++++++++++++++++++++++--
> drivers/net/wireless/ath/wil6210/txrx.h | 28 +++++++++--
> drivers/net/wireless/ath/wil6210/wmi.c | 8 ++++
> 4 files changed, 130 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
> index 098a8ec..4f2a61f 100644
> --- a/drivers/net/wireless/ath/wil6210/netdev.c
> +++ b/drivers/net/wireless/ath/wil6210/netdev.c
> @@ -32,12 +32,34 @@ static int wil_stop(struct net_device *ndev)
> return wil_down(wil);
> }
>
> +static netdev_features_t wil_fix_features(struct net_device *netdev,
> + netdev_features_t features)
> +{
> + return features;
> +}
> +
> +static int wil_set_features(struct net_device *netdev,
> + netdev_features_t features)
> +{
> + struct wil6210_priv *wil = ndev_to_wil(netdev);
> + netdev_features_t changed = features ^ netdev->features;
> +
> + wil_info(wil, "wil_set_features: %x\n", (unsigned int)features);
> + if (changed & NETIF_F_HW_CSUM)
> + wil_info(wil, "Tx checksum offloading changed\n");
> + else if (changed & NETIF_F_RXCSUM)
> + wil_info(wil, "Rx checksum offloading changed\n");
> + return 0;
> +}
> +
> static const struct net_device_ops wil_netdev_ops = {
> .ndo_open = wil_open,
> .ndo_stop = wil_stop,
> .ndo_start_xmit = wil_start_xmit,
> .ndo_set_mac_address = eth_mac_addr,
> .ndo_validate_addr = eth_validate_addr,
> + .ndo_fix_features = wil_fix_features,
> + .ndo_set_features = wil_set_features,
> };

2 methods above are not needed, remove

>
> void *wil_if_alloc(struct device *dev, void __iomem *csr)
> @@ -78,6 +100,8 @@ void *wil_if_alloc(struct device *dev, void __iomem *csr)
>
> ndev->netdev_ops = &wil_netdev_ops;
> ndev->ieee80211_ptr = wdev;
> + ndev->hw_features = NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
> + ndev->features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
> SET_NETDEV_DEV(ndev, wiphy_dev(wdev->wiphy));
> wdev->netdev = ndev;
>
> diff --git a/drivers/net/wireless/ath/wil6210/txrx.c b/drivers/net/wireless/ath/wil6210/txrx.c
> index 7970245..3f44a6c 100644
> --- a/drivers/net/wireless/ath/wil6210/txrx.c
> +++ b/drivers/net/wireless/ath/wil6210/txrx.c
> @@ -18,6 +18,9 @@
> #include <net/ieee80211_radiotap.h>
> #include <linux/if_arp.h>
> #include <linux/moduleparam.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <net/ipv6.h>
>
> #include "wil6210.h"
> #include "wmi.h"
> @@ -391,7 +394,16 @@ static struct sk_buff *wil_vring_reap_rx(struct wil6210_priv *wil,
> */
> wil_swap_ethaddr(skb->data);
> }
> -
> + /* Check checksum-offload status */
> + if (ndev->features & NETIF_F_RXCSUM) {
> + if (d->dma.status & RX_DMA_STATUS_L4_IDENT) {

This code access Rx descriptor in the non-cached memory, this is time consuming.
Need to access cached copy through 'd1'
Also, to reduce identation, combine 2 'if' statement into one.

> + /* L4 protocol identified, csum calculated */
> + if ((d->dma.error & RX_DMA_ERROR_L4_ERR) == 0)

Same note for non-cached memory as above

> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + else
> + wil_err(wil, "Incorrect checksum reported\n");

If this error detected, does it mean packet still usable?

What is exact meaning of this error:
- HW did not calculated checksum for some reason, or
- checksum calculated but do not match

> + }
> + }
> return skb;
> }
>
> @@ -605,9 +617,7 @@ static int wil_tx_desc_map(volatile struct vring_tx_desc *d,
> {
> d->dma.addr_low = lower_32_bits(pa);
> d->dma.addr_high = (u16)upper_32_bits(pa);
> - d->dma.ip_length = 0;
> - /* 0..6: mac_length; 7:ip_version 0-IP6 1-IP4*/
> - d->dma.b11 = 0/*14 | BIT(7)*/;
> + d->dma.offload_cfg = 0;
> d->dma.error = 0;
> d->dma.status = 0; /* BIT(0) should be 0 for HW_OWNED */
> d->dma.length = len;
> @@ -630,6 +640,7 @@ static int wil_tx_vring(struct wil6210_priv *wil, struct vring *vring,
> struct sk_buff *skb)
> {
> struct device *dev = wil_to_dev(wil);
> + struct net_device *ndev = wil_to_ndev(wil);
> volatile struct vring_tx_desc *d;
> u32 swhead = vring->swhead;
> int avail = wil_vring_avail_tx(vring);
> @@ -638,6 +649,8 @@ static int wil_tx_vring(struct wil6210_priv *wil, struct vring *vring,
> int vring_index = vring - wil->vring_tx;
> uint i = swhead;
> dma_addr_t pa;
> + int is_ip4 = 0, is_ip6 = 0, is_tcp = 0, is_udp = 0;
> +
>
> wil_dbg_txrx(wil, "%s()\n", __func__);
>
> @@ -665,6 +678,62 @@ static int wil_tx_vring(struct wil6210_priv *wil, struct vring *vring,
> return -EINVAL;
> /* 1-st segment */
> wil_tx_desc_map(d, pa, skb_headlen(skb));

Please make separate function like wil_tx_csum from code below

Also, consider optimising calculations and return ASAP like:

bool is_ip6 = false;
switch(skb->protocol) {
case htons(ETH_P_IP):
break;
case htons(ETH_P_IPV6):
is_ip6 = true;
break;
default:
return;
}
> + /*
> + * Process offloading
> + */
> + if ((skb->ip_summed == CHECKSUM_PARTIAL) &&
> + (ndev->features & NETIF_F_HW_CSUM)) {
> + if (skb->protocol == htons(ETH_P_IP)) {
> + is_ip4 = 1;
> + if (ip_hdr(skb)->protocol == IPPROTO_TCP)
> + is_tcp = 1;
> + else if (ip_hdr(skb)->protocol == IPPROTO_UDP)
> + is_udp = 1;
> + } else if (skb->protocol == htons(ETH_P_IPV6)) {
> + unsigned int offset = 0;
> + int ipv6hdr = ipv6_find_hdr(skb,
> + &offset, -1, NULL, NULL);
> + is_ip6 = 1;
> + if (ipv6hdr == NEXTHDR_TCP)
> + is_tcp = 1;
> + else if (ipv6hdr == NEXTHDR_UDP)
> + is_udp = 1;
> + }
> + }
> +
> +
> + if (is_ip4 || is_ip6) {
> + if (is_ip4)
> + d->dma.offload_cfg |=
> + BIT(DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_POS);
> + d->dma.offload_cfg |=
> + (skb_network_header_len(skb) &
> + DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_MSK);
> + d->dma.offload_cfg |=
> + (0x0e << DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_POS);
> + if (is_tcp || is_udp) {
> + /* Enable TCP/UDP checksum */

Do one need to do anything if it is neither TCP nor UDP?

> + d->dma.d0 |=
> + BIT(DMA_CFG_DESC_TX_0_TCP_UDP_CHECKSUM_EN_POS);
> + /* Calculate pseudo-header */
> + d->dma.d0 |=
> + BIT(DMA_CFG_DESC_TX_0_PSEUDO_HEADER_CALC_EN_POS);
> + if (is_tcp) {
> + d->dma.d0 |=
> + (2 << DMA_CFG_DESC_TX_0_L4_TYPE_POS);
> + /* L4 header len: TCP header length */
> + d->dma.d0 |=
> + (tcp_hdrlen(skb) &
> + DMA_CFG_DESC_TX_0_L4_LENGTH_MSK);
> + } else {
> + /* L4 header len: UDP header length */
> + d->dma.d0 |=
> + (sizeof(struct udphdr) &
> + DMA_CFG_DESC_TX_0_L4_LENGTH_MSK);
> + }
> + }
> + }
> +
> d->mac.d[2] |= ((nr_frags + 1) <<
> MAC_CFG_DESC_TX_2_NUM_OF_DESCRIPTORS_POS);
> /* middle segments */
> diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h
> index adef12f..da22a93 100644
> --- a/drivers/net/wireless/ath/wil6210/txrx.h
> +++ b/drivers/net/wireless/ath/wil6210/txrx.h
> @@ -209,7 +209,21 @@ struct vring_tx_mac {
>
> #define DMA_CFG_DESC_TX_0_L4_TYPE_POS 30
> #define DMA_CFG_DESC_TX_0_L4_TYPE_LEN 2
> -#define DMA_CFG_DESC_TX_0_L4_TYPE_MSK 0xC0000000
> +#define DMA_CFG_DESC_TX_0_L4_TYPE_MSK 0xC0000000 /* L4 type: 0-UDP, 2-TCP */
> +
> +/* TX DMA Dword 2 */
> +/* Offload bits in offload_cfg field */
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_POS 0
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_LEN 8
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_IP_LEN_MSK 0x00FF /* IP hdr len */
> +
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_POS 8
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_LEN 7
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_MAC_LEN_MSK 0x7F00 /* MAC hdr len */
> +
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_POS 15
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_LEN 1
> +#define DMA_CFG_DESC_TX_OFFLOAD_CFG_L3T_IPV4_MSK 0x8000 /* 1-IPv4, 0-IPv6 */
>
>
> #define TX_DMA_STATUS_DU BIT(0)
> @@ -218,8 +232,7 @@ struct vring_tx_dma {
> u32 d0;
> u32 addr_low;
> u16 addr_high;
> - u8 ip_length;
> - u8 b11; /* 0..6: mac_length; 7:ip_version */
> + u16 offload_cfg;

If 2 bytes above combined to u16, proper endian conversions need to be used.
Alternatively, you may keep 2 u8 fields, renaming 2-nd one to offload_cfg,
and adjust bit numbers.

> u8 error; /* 0..2: err; 3..7: reserved; */
> u8 status; /* 0: used; 1..7; reserved */
> u16 length;
> @@ -309,8 +322,17 @@ struct vring_rx_mac {
>
> #define RX_DMA_D0_CMD_DMA_IT BIT(10)
>
> +/* Error field, offload bits */
> +#define RX_DMA_ERROR_L3_ERR BIT(4)
> +#define RX_DMA_ERROR_L4_ERR BIT(5)
> +
> +
> +/* Status field */
> #define RX_DMA_STATUS_DU BIT(0)
> #define RX_DMA_STATUS_ERROR BIT(2)
> +
> +#define RX_DMA_STATUS_L3_IDENT BIT(4)
> +#define RX_DMA_STATUS_L4_IDENT BIT(5)

These 2 bits are not used. Shouldn't it be checked on Rx?

> #define RX_DMA_STATUS_PHY_INFO BIT(6)
>
> struct vring_rx_dma {
> diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
> index 45b04e3..dc0aa8a 100644
> --- a/drivers/net/wireless/ath/wil6210/wmi.c
> +++ b/drivers/net/wireless/ath/wil6210/wmi.c
> @@ -932,6 +932,14 @@ int wmi_rx_chain_add(struct wil6210_priv *wil, struct vring *vring)
> struct wmi_cfg_rx_chain_done_event evt;
> } __packed evt;
> int rc;
> + u8 csum_enabled = 0;
> +
> + /* Initialize offload. Linux always calculates IP checksum */
> + cmd.l3_l4_ctrl |= (0 << L3_L4_CTRL_IPV4_CHECKSUM_EN_POS);

Why do one need IP checksum enable if NETIF_F_RXCSUM disabled?

> + /* Enable/disable TCP/UDP checksum */
> + if (ndev->features & NETIF_F_RXCSUM)
> + csum_enabled = 1;
> + cmd.l3_l4_ctrl |= (csum_enabled << L3_L4_CTRL_TCPIP_CHECKSUM_EN_POS);

Fragment above will be better like (no need to introduce csum_enabled):

if (ndev->features & NETIF_F_RXCSUM)
cmd.l3_l4_ctrl |= BIT(L3_L4_CTRL_TCPIP_CHECKSUM_EN_POS);


>
> if (wdev->iftype == NL80211_IFTYPE_MONITOR) {
> struct ieee80211_channel *ch = wdev->preset_chandef.chan;
>

Thanks, Vladimir

2013-05-09 01:37:50

by Julian Calaby

[permalink] [raw]
Subject: Re: [PATCH 1/3] wil6210: TCP/UDP checksum offloading support

Hi Erez,

Just a couple of notes on patch authorship and email addresses.

On Wed, May 8, 2013 at 6:32 PM, <[email protected]> wrote:
> From: Erez Kirshenbaum <[email protected]>

In the other two patches, your email address is "erezk@wilocitycom" -
note the missing "."

> Add TCP/UDP checksum hardware offloading, configure hw and set the corresponding offloading bits in the
> DMA descriptors.
>
> Signed-off-by: Erez Kirshenbaum <[email protected]>
> Signed-off-by: Wilocity Git <[email protected]>

I assume you've read Documentation/SubmittingPatches, particularly the
section about signing your changes:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n297

The names in Signed-off-bys are expected to be real people or
permanent email addresses for teams. Given the name of the gmail
account, this appears to be just a email address you're sending
patches through. Is it monitored? Can you not send patches directly
from your "real" account?

Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/