Re: [PATCH v2] ath6kl: Support for TCP checksum offload to firmware
From: Kalle Valo <hidden>
Date: 2011-12-24 01:22:55
Hi Rishi, On 12/21/2011 02:25 AM, Rishi Panjwani wrote:
The change enables offloading TCP checksum calculation to firmware. The support has been added for QA team's testing purposes and currently there is no plan to offload the feature to firmware by default at load time.
Why? Please document the reason in the commit log. Also does this comment still apply? It wasn't clear for me from looking at the code.
+ ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
No need for parenthesis here.
quoted hunk ↗ jump to hunk
@@ -265,6 +267,14 @@ int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev) } if (test_bit(WMI_ENABLED, &ar->flag)) { + if ((dev->hw_features & NETIF_F_IP_CSUM) && + (csum == CHECKSUM_PARTIAL)) { + csum_start = skb->csum_start - + (skb_network_header(skb) - skb->head) + + sizeof(struct ath6kl_llc_snap_hdr); + csum_dest = skb->csum_offset + csum_start; + }
Shouldn't you test dev->features here, not hw_features?
quoted hunk ↗ jump to hunk
+ if (skb_headroom(skb) < dev->needed_headroom) { struct sk_buff *tmp_skb = skb;@@ -281,11 +291,31 @@ int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev) goto fail_tx; } - if (ath6kl_wmi_data_hdr_add(ar->wmi, skb, DATA_MSGTYPE, - more_data, 0, 0, NULL, - vif->fw_vif_idx)) { - ath6kl_err("wmi_data_hdr_add failed\n"); - goto fail_tx; + if ((dev->hw_features & NETIF_F_IP_CSUM) && + (csum == CHECKSUM_PARTIAL)) {
Ditto.
+ meta_v2.csum_start = csum_start; + meta_v2.csum_dest = csum_dest; + +/*instruct target to calculate checksum*/
Indent similarly as the rest of the code and add spaces like this: /* instruct target to calculate checksum */
+ meta_v2.csum_flags = CSUM_OFFLOAD_FLAG;
+ ret = ath6kl_wmi_data_hdr_add(ar->wmi, skb,
+ DATA_MSGTYPE, more_data, 0,
+ WMI_META_VERSION_2,
+ &meta_v2, vif->fw_vif_idx);
+ if (ret) {
+ ath6kl_warn("failed to add tcp checksum:%d\n"
+ , ret);
+ goto fail_tx;
+ }
+ } else {
+ ret = ath6kl_wmi_data_hdr_add(ar->wmi, skb,
+ DATA_MSGTYPE, more_data, 0, 0,
+ NULL, vif->fw_vif_idx);
+ if (ret) {
+ ath6kl_warn("failed to add wmi data header:%d\n"
+ , ret);
+ goto fail_tx;
+ }
}
Instead of calling the wmi function twice you could add new variables to
make it look simpler:
if (foo) {
meta = &meta_v2;
meta_ver = WMI_META_VERSION_2;
} else {
meta = NULL;
meta_ver = 0;
}
ret = ath6kl_wmi_data_hdr_add(ar->wmi, skb,
DATA_MSGTYPE, more_data, 0,
meta_ver,
&meta, vif->fw_vif_idx);
quoted hunk ↗ jump to hunk
@@ -1259,6 +1289,27 @@ void ath6kl_rx(struct htc_target *target, struct htc_packet *packet) break; } +/*Support for configuring rx checksum offload*/
Indent and add spaces.
+ if ((vif->ndev->features & NETIF_F_RXCSUM) &&
+ (ar->rx_meta_ver != WMI_META_VERSION_2)) {
+ ar->rx_meta_ver = WMI_META_VERSION_2;
+ if (ath6kl_wmi_set_rx_frame_format_cmd(ar->wmi,
+ if_idx, ar->rx_meta_ver, 0, 0)) {
+ ath6kl_err("unable to set the rx frame format\n");
+ status = -EIO;
+ }
+ } else if (!(vif->ndev->features & NETIF_F_RXCSUM) &&
+ (ar->rx_meta_ver == WMI_META_VERSION_2)) {
+ ar->rx_meta_ver = 0;
+ if (ath6kl_wmi_set_rx_frame_format_cmd(ar->wmi,
+ if_idx, ar->rx_meta_ver, 0, 0)) {
+ ath6kl_err("unable to set the rx frame format\n");
+ status = -EIO;
+ }
+
+ }I don't like that this in the rx path, which is supposed to be a hotpath. I think adding ndo_set_features() callback and doing this in that callback is much better.
quoted hunk ↗ jump to hunk
--- a/drivers/net/wireless/ath/ath6kl/wmi.h +++ b/drivers/net/wireless/ath/ath6kl/wmi.h@@ -257,6 +257,9 @@ static inline u8 wmi_data_hdr_get_if_idx(struct wmi_data_hdr *dhdr) #define WMI_META_VERSION_1 0x01 #define WMI_META_VERSION_2 0x02 +/* Flag to signal to FW to calculate TCP checksum */ +#define CSUM_OFFLOAD_FLAG 0x01
This could be named a bit differently, WMI_META_V2_FLAG_CSUM_OFFLOAD or something like that. Kalle