Skip to contentSkip to conversation

qla3xxx checksum related patches

PATCH Applied Stephen Hemminger 3 participants 9 messages May 30, 2007 – Jun 13, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover qla3xxx checksum related patches

Found while looking at drivers that are doing NETIF_F_HW_CSUM incorrectly.

The first is a bug fix. Second is cleanup, Third is an enhancement.

Signature · 1 line
Stephen Hemminger <[email protected]>
#1 lore raw

PATCH 1/3 qla3xxx: device doesnt do hardware checksumming.

Reading the code for ql_hw_csum_setup(), it is obvious that this driver is broken for IPV6. The driver sets the NETIF_F_HW_SUM flag, but the code for checksum setup only deals with IPV4.

Compile tested only, no hardware available.

---
 drivers/net/qla3xxx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/qla3xxx.c +1 -1
@@ -4044,7 +4044,7 @@ static int __devinit ql3xxx_probe(struct
 	if (pci_using_dac)
 		ndev->features |= NETIF_F_HIGHDMA;
 	if (qdev->device_id == QL3032_DEVICE_ID)
-		ndev->features |= (NETIF_F_HW_CSUM | NETIF_F_SG);
+		ndev->features |= NETIF_F_IP_CSUM | NETIF_F_SG;
 
 	qdev->mem_map_registers =
 	    ioremap_nocache(pci_resource_start(pdev, 1),
Signature · 1 line
Stephen Hemminger <[email protected]>
#3 lore raw

PATCH 2/3 qla3xxx: cleanup checksum offload code

In reply to Stephen Hemminger

The code for checksum is more complex than needed when dealing with VLAN's; the higher layers already pass down the location of the IP header.

Compile tested only, no hardware available.

---
 drivers/net/qla3xxx.c |   37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)
drivers/net/qla3xxx.c +11 -26
@@ -2433,37 +2433,22 @@ static int ql_get_seg_count(struct ql3_a
 	return -1;
 }
 
-static void ql_hw_csum_setup(struct sk_buff *skb,
+static void ql_hw_csum_setup(const struct sk_buff *skb,
 			     struct ob_mac_iocb_req *mac_iocb_ptr)
 {
-	struct ethhdr *eth;
-	struct iphdr *ip = NULL;
-	u8 offset = ETH_HLEN;
-
-	eth = (struct ethhdr *)(skb->data);
-
-	if (eth->h_proto == __constant_htons(ETH_P_IP)) {
-		ip = (struct iphdr *)&skb->data[ETH_HLEN];
-	} else if (eth->h_proto == htons(ETH_P_8021Q) &&
-		   ((struct vlan_ethhdr *)skb->data)->
-		   h_vlan_encapsulated_proto == __constant_htons(ETH_P_IP)) {
-		ip = (struct iphdr *)&skb->data[VLAN_ETH_HLEN];
-		offset = VLAN_ETH_HLEN;
-	}
-
-	if (ip) {
-		if (ip->protocol == IPPROTO_TCP) {
-			mac_iocb_ptr->flags1 |= OB_3032MAC_IOCB_REQ_TC | 
+	const struct iphdr *ip = ip_hdr(skb);
+
+	mac_iocb_ptr->ip_hdr_off = skb_network_offset(skb);
+	mac_iocb_ptr->ip_hdr_len = ip->ihl;
+
+	if (ip->protocol == IPPROTO_TCP) {
+		mac_iocb_ptr->flags1 |= OB_3032MAC_IOCB_REQ_TC |
 			OB_3032MAC_IOCB_REQ_IC;
-			mac_iocb_ptr->ip_hdr_off = offset;
-			mac_iocb_ptr->ip_hdr_len = ip->ihl;
-		} else if (ip->protocol == IPPROTO_UDP) {
-			mac_iocb_ptr->flags1 |= OB_3032MAC_IOCB_REQ_UC | 
+	} else {
+		mac_iocb_ptr->flags1 |= OB_3032MAC_IOCB_REQ_UC |
 			OB_3032MAC_IOCB_REQ_IC;
-			mac_iocb_ptr->ip_hdr_off = offset;
-			mac_iocb_ptr->ip_hdr_len = ip->ihl;
-		}
 	}
+
 }
 
 /*
Signature · 1 line
Stephen Hemminger <[email protected]>
#8 lore raw

PATCH 3/3 qla3xxx: ethtool ops for controlling checksum

In reply to Stephen Hemminger

Add the ability to control scatter/gather and checksumming via ethtool.

Compile tested only, no hardware available.

---
 drivers/net/qla3xxx.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
drivers/net/qla3xxx.c +33 -0
@@ -1901,6 +1901,35 @@ static void ql_get_pauseparam(struct net
 	pause->tx_pause = (reg & MAC_CONFIG_REG_TF) >> 1;
 }
 
+static int ql_set_tx_csum(struct net_device *dev, u32 data)
+{
+	if (data) {
+		struct ql3_adapter *qdev = netdev_priv(ndev);
+
+		if (qdev->device_id != QL3032_DEVICE_ID)
+			return -EINVAL;
+
+		dev->features |= NETIF_F_IP_CSUM;
+	} else
+		dev->features &= ~NETIF_F_IP_CSUM;
+
+	return 0;
+}
+
+static int ql_set_sg(struct net_device *dev, u32 data)
+{
+	if (data) {
+		struct ql3_adapter *qdev = netdev_priv(ndev);
+
+		if (qdev->device_id != QL3032_DEVICE_ID)
+			return -EINVAL;
+		dev->features |= NETIF_F_SG;
+	} else
+		dev->features &= ~NETIF_F_SG;
+
+	return 0;
+}
+
 static const struct ethtool_ops ql3xxx_ethtool_ops = {
 	.get_settings = ql_get_settings,
 	.get_drvinfo = ql_get_drvinfo,
@@ -1909,6 +1938,10 @@ static const struct ethtool_ops ql3xxx_e
 	.get_msglevel = ql_get_msglevel,
 	.set_msglevel = ql_set_msglevel,
 	.get_pauseparam = ql_get_pauseparam,
+	.get_tx_csum = ql_op_get_tx_csum,
+	.set_tx_csum = ethtool_op_set_tx_csum,
+	.get_sg = ethtool_op_get_sg,
+	.set_sg = ql_get_sg,
 };
 
 static int ql_populate_free_queue(struct ql3_adapter *qdev)
Signature · 1 line
Stephen Hemminger <[email protected]>
Applied
#2 lore raw

Re: [PATCH 1/3] qla3xxx: device doesnt do hardware checksumming.

In reply to Stephen Hemminger
Quoted 7 lines · Stephen Hemminger

Reading the code for ql_hw_csum_setup(), it is obvious that this driver is broken for IPV6. The driver sets the NETIF_F_HW_SUM flag, but the code for checksum setup only deals with IPV4.

Compile tested only, no hardware available.

applied to #upstream-fixes

#4 lore raw

RE: [PATCH 2/3] qla3xxx: cleanup checksum offload code

In reply to Stephen Hemminger

Jeff,
You've already appled patch 1/3 in Stephen's series. I tested this one (2/3) and it looks good. I can resubmit this if you want. Regards, Ron Mercer

Quoted 78 lines

-----Original Message-----
From: Stephen Hemminger [mailto:[email protected]]
Sent: Wednesday, May 30, 2007 2:23 PM
To: Jeff Garzik; Linux Driver
Cc: [email protected]
Subject: [PATCH 2/3] qla3xxx: cleanup checksum offload code

The code for checksum is more complex than needed when dealing with VLAN's;
the higher layers already pass down the location of the IP header.

Compile tested only, no hardware available.

---
 drivers/net/qla3xxx.c |   37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)
drivers/net/qla3xxx.c +1 -19
@@ -2433,37 +2433,22 @@ static int ql_get_seg_count(struct ql3_a
 	return -1;
 }
 
-static void ql_hw_csum_setup(struct sk_buff *skb,
+static void ql_hw_csum_setup(const struct sk_buff *skb,
 			     struct ob_mac_iocb_req *mac_iocb_ptr)
 {
-	struct ethhdr *eth;
-	struct iphdr *ip = NULL;
-	u8 offset = ETH_HLEN;
-
-	eth = (struct ethhdr *)(skb->data);
-
-	if (eth->h_proto == __constant_htons(ETH_P_IP)) {
-		ip = (struct iphdr *)&skb->data[ETH_HLEN];
-	} else if (eth->h_proto == htons(ETH_P_8021Q) &&
-		   ((struct vlan_ethhdr *)skb->data)->
-		   h_vlan_encapsulated_proto == 
 __constant_htons(ETH_P_IP)) {
-		ip = (struct iphdr *)&skb->data[VLAN_ETH_HLEN];
-		offset = VLAN_ETH_HLEN;
-	}
-
-	if (ip) {
-		if (ip->protocol == IPPROTO_TCP) {
-			mac_iocb_ptr->flags1 |= 

OB_3032MAC_IOCB_REQ_TC |

drivers/net/qla3xxx.c +7 -4
+	const struct iphdr *ip = ip_hdr(skb);
+
+	mac_iocb_ptr->ip_hdr_off = skb_network_offset(skb);
+	mac_iocb_ptr->ip_hdr_len = ip->ihl;
+
+	if (ip->protocol == IPPROTO_TCP) {
+		mac_iocb_ptr->flags1 |= OB_3032MAC_IOCB_REQ_TC |
 			OB_3032MAC_IOCB_REQ_IC;
-			mac_iocb_ptr->ip_hdr_off = offset;
-			mac_iocb_ptr->ip_hdr_len = ip->ihl;
-		} else if (ip->protocol == IPPROTO_UDP) {
-			mac_iocb_ptr->flags1 |= 

OB_3032MAC_IOCB_REQ_UC |

drivers/net/qla3xxx.c +3 -3
+	} else {
+		mac_iocb_ptr->flags1 |= OB_3032MAC_IOCB_REQ_UC |
 			OB_3032MAC_IOCB_REQ_IC;
-			mac_iocb_ptr->ip_hdr_off = offset;
-			mac_iocb_ptr->ip_hdr_len = ip->ihl;
-		}
 	}
+
 }
 
 /*
Signature · 2 lines
-- 
Stephen Hemminger <[email protected]>
#5 lore raw

Re: [PATCH 2/3] qla3xxx: cleanup checksum offload code

In reply to Stephen Hemminger
Quoted 6 lines · Stephen Hemminger

The code for checksum is more complex than needed when dealing with VLAN's; the higher layers already pass down the location of the IP header.

Compile tested only, no hardware available.

Ron, do you ACK patch #2 and patch #3?

#6 lore raw

RE: [PATCH 2/3] qla3xxx: cleanup checksum offload code

ACK patch #2, not patch 3. I will add patch 3 at a later date.

Quoted 20 lines

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Wednesday, June 13, 2007 12:44 PM
To: Stephen Hemminger
Cc: Linux Driver; [email protected]
Subject: Re: [PATCH 2/3] qla3xxx: cleanup checksum offload code

Quoted 1 line · Stephen Hemminger

The code for checksum is more complex than needed when

dealing with VLAN's;

Quoted 5 lines

the higher layers already pass down the location of the IP header.

Compile tested only, no hardware available.

Ron, do you ACK patch #2 and patch #3?

Applied
#7 lore raw

Re: [PATCH 2/3] qla3xxx: cleanup checksum offload code

In reply to Stephen Hemminger
Quoted 6 lines · Stephen Hemminger

The code for checksum is more complex than needed when dealing with VLAN's; the higher layers already pass down the location of the IP header.

Compile tested only, no hardware available.

applied to #upstream (2.6.23)