[PATCH] net: ksz884x: convert to hw_features

[PATCH] net: ksz884x: convert to hw_features

[Michał Mirosław]

This also fixes possible race when changing receive checksum state
and removes IPV6_CSUM_GEN_HACK as it's always set.

BTW, The claim about fake IPV6 checksum looks dubious. If that were true,
then there's a problem in networking core and should be fixed there and not
in random drivers.

BTW#2, there's no MAINTAINERS entry for this driver. I assume this driver
is supported by Micrel?

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/ksz884x.c |   73 ++++++++++++++-----------------------------------
 1 files changed, 21 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ksz884x.c b/drivers/net/ksz884x.c
index 7f7d570..2c37a38 100644
--- a/drivers/net/ksz884x.c
+++ b/drivers/net/ksz884x.c
@@ -1221,7 +1221,6 @@ struct ksz_port_info {
 #define LINK_INT_WORKING		(1 << 0)
 #define SMALL_PACKET_TX_BUG		(1 << 1)
 #define HALF_DUPLEX_SIGNAL_BUG		(1 << 2)
-#define IPV6_CSUM_GEN_HACK		(1 << 3)
 #define RX_HUGE_FRAME			(1 << 4)
 #define STP_SUPPORT			(1 << 8)
 
@@ -3748,7 +3747,6 @@ static int hw_init(struct ksz_hw *hw)
 		if (1 == rc)
 			hw->features |= HALF_DUPLEX_SIGNAL_BUG;
 	}
-	hw->features |= IPV6_CSUM_GEN_HACK;
 	return rc;
 }
 
@@ -4887,8 +4885,7 @@ static netdev_tx_t netdev_tx(struct sk_buff *skb, struct net_device *dev)
 	left = hw_alloc_pkt(hw, skb->len, num);
 	if (left) {
 		if (left < num ||
-				((hw->features & IPV6_CSUM_GEN_HACK) &&
-				(CHECKSUM_PARTIAL == skb->ip_summed) &&
+				((CHECKSUM_PARTIAL == skb->ip_summed) &&
 				(ETH_P_IPV6 == htons(skb->protocol)))) {
 			struct sk_buff *org_skb = skb;
 
@@ -6583,57 +6580,33 @@ static void netdev_get_ethtool_stats(struct net_device *dev,
 }
 
 /**
- * netdev_get_rx_csum - get receive checksum support
+ * netdev_set_features - set receive checksum support
  * @dev:	Network device.
- *
- * This function gets receive checksum support setting.
- *
- * Return true if receive checksum is enabled; false otherwise.
- */
-static u32 netdev_get_rx_csum(struct net_device *dev)
-{
-	struct dev_priv *priv = netdev_priv(dev);
-	struct dev_info *hw_priv = priv->adapter;
-	struct ksz_hw *hw = &hw_priv->hw;
-
-	return hw->rx_cfg &
-		(DMA_RX_CSUM_UDP |
-		DMA_RX_CSUM_TCP |
-		DMA_RX_CSUM_IP);
-}
-
-/**
- * netdev_set_rx_csum - set receive checksum support
- * @dev:	Network device.
- * @data:	Zero to disable receive checksum support.
+ * @features:	New device features (offloads).
  *
  * This function sets receive checksum support setting.
  *
  * Return 0 if successful; otherwise an error code.
  */
-static int netdev_set_rx_csum(struct net_device *dev, u32 data)
+static int netdev_set_features(struct net_device *dev, u32 features)
 {
 	struct dev_priv *priv = netdev_priv(dev);
 	struct dev_info *hw_priv = priv->adapter;
 	struct ksz_hw *hw = &hw_priv->hw;
-	u32 new_setting = hw->rx_cfg;
 
-	if (data)
-		new_setting |=
-			(DMA_RX_CSUM_UDP | DMA_RX_CSUM_TCP |
-			DMA_RX_CSUM_IP);
-	else
-		new_setting &=
-			~(DMA_RX_CSUM_UDP | DMA_RX_CSUM_TCP |
-			DMA_RX_CSUM_IP);
-	new_setting &= ~DMA_RX_CSUM_UDP;
 	mutex_lock(&hw_priv->lock);
-	if (new_setting != hw->rx_cfg) {
-		hw->rx_cfg = new_setting;
-		if (hw->enabled)
-			writel(hw->rx_cfg, hw->io + KS_DMA_RX_CTRL);
-	}
+
+	/* see note in hw_setup() */
+	if (features & NETIF_F_RXCSUM)
+		hw->rx_cfg |= DMA_RX_CSUM_TCP | DMA_RX_CSUM_IP;
+	else
+		hw->rx_cfg &= ~(DMA_RX_CSUM_TCP | DMA_RX_CSUM_IP);
+
+	if (hw->enabled)
+		writel(hw->rx_cfg, hw->io + KS_DMA_RX_CTRL);
+
 	mutex_unlock(&hw_priv->lock);
+
 	return 0;
 }
 
@@ -6658,12 +6631,6 @@ static struct ethtool_ops netdev_ethtool_ops = {
 	.get_strings		= netdev_get_strings,
 	.get_sset_count		= netdev_get_sset_count,
 	.get_ethtool_stats	= netdev_get_ethtool_stats,
-	.get_rx_csum		= netdev_get_rx_csum,
-	.set_rx_csum		= netdev_set_rx_csum,
-	.get_tx_csum		= ethtool_op_get_tx_csum,
-	.set_tx_csum		= ethtool_op_set_tx_csum,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
 };
 
 /*
@@ -6828,14 +6795,15 @@ static int __init netdev_init(struct net_device *dev)
 	/* 500 ms timeout */
 	dev->watchdog_timeo = HZ / 2;
 
-	dev->features |= NETIF_F_IP_CSUM;
+	dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG | NETIF_F_RXCSUM;
 
 	/*
 	 * Hardware does not really support IPv6 checksum generation, but
-	 * driver actually runs faster with this on.  Refer IPV6_CSUM_GEN_HACK.
+	 * driver actually runs faster with this on.
 	 */
-	dev->features |= NETIF_F_IPV6_CSUM;
-	dev->features |= NETIF_F_SG;
+	dev->hw_features |= NETIF_F_IPV6_CSUM;
+
+	dev->features |= dev->hw_features;
 
 	sema_init(&priv->proc_sem, 1);
 
@@ -6860,6 +6828,7 @@ static const struct net_device_ops netdev_ops = {
 	.ndo_start_xmit		= netdev_tx,
 	.ndo_tx_timeout		= netdev_tx_timeout,
 	.ndo_change_mtu		= netdev_change_mtu,
+	.ndo_set_features	= netdev_set_features,
 	.ndo_set_mac_address	= netdev_set_mac_address,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_do_ioctl		= netdev_ioctl,
-- 
1.7.2.5

Re: [PATCH] net: ksz884x: convert to hw_features

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Sun, 10 Apr 2011 15:13:21 +0200 (CEST)

> This also fixes possible race when changing receive checksum state
> and removes IPV6_CSUM_GEN_HACK as it's always set.
> 
> BTW, The claim about fake IPV6 checksum looks dubious. If that were true,
> then there's a problem in networking core and should be fixed there and not
> in random drivers.
> 
> BTW#2, there's no MAINTAINERS entry for this driver. I assume this driver
> is supported by Micrel?
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.

RE: [PATCH] net: ksz884x: convert to hw_features

[Ha, Tristram]

Michał Mirosław wrote:
> This also fixes possible race when changing receive checksum state and removes
> IPV6_CSUM_GEN_HACK as it's always set. 
> 
> BTW, The claim about fake IPV6 checksum looks dubious. If that were true, then there's a problem
> in networking core and should be fixed there and not in random drivers. 
> 
> BTW#2, there's no MAINTAINERS entry for this driver. I assume this driver is supported by Micrel?
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---

Micrel KSZ884X PCI Ethernet Controller does not support generating checksums on IPv6 packets.  But if the driver advertises IPv6 checksum generation support and uses the skb_copy_and_csum_dev function to consolidate the socket buffer, the network performance is actually better than the normal method.  Common sense seems to dictate that using the normal method is faster than the scatter/gather then consolidation method,  but actually this method is faster.  Indeed that is another network driver in the kernel that advertises IPv4 checksum generation support but does not do anything special in hardware and just uses the skb_copy_and_csum_dev function.  If this method is no longer faster or workable, then KSZ884X driver should not advertise IPv6 checksum generation.