Patchwork [net-next,1/5] sky2: force receive checksum when using RSS on some hardware (v2)

login
register
mail settings
Submitter stephen hemminger
Date July 7, 2011, 11:40 p.m.
Message ID <20110707164000.6f3f58f4@nehalam.ftrdhcpuser.net>
Download mbox | patch
Permalink /patch/103744/
State Accepted
Delegated to: David Miller
Headers show

Comments

stephen hemminger - July 7, 2011, 11:40 p.m.
Found when reviewing the vendor driver. Apparently some chip versions
require receive checksumming to be enabled in order for RSS to work.

Also, if fix_features has to change some settings; put in message
in log in similar manner to netdev_fix_features.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
v2 - enforce the requirement in fix_features rather than set features


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - July 8, 2011, 8:45 a.m.
2011/7/8 Stephen Hemminger <shemminger@vyatta.com>:
> Found when reviewing the vendor driver. Apparently some chip versions
> require receive checksumming to be enabled in order for RSS to work.
>
> Also, if fix_features has to change some settings; put in message
> in log in similar manner to netdev_fix_features.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> ---
> v2 - enforce the requirement in fix_features rather than set features
>
> --- a/drivers/net/sky2.c        2011-07-07 13:56:03.575420273 -0700
> +++ b/drivers/net/sky2.c        2011-07-07 14:03:17.775403938 -0700
[...]
> @@ -4176,8 +4180,18 @@ static u32 sky2_fix_features(struct net_
>        /* In order to do Jumbo packets on these chips, need to turn off the
>         * transmit store/forward. Therefore checksum offload won't work.
>         */
> -       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U)
> +       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U) {
> +               netdev_warn(dev, "checksum offload not possible with jumbo frames\n");
>                features &= ~(NETIF_F_TSO|NETIF_F_SG|NETIF_F_ALL_CSUM);
> +       }
> +
> +       /* Some hardware requires receive checksum for RSS to work. */
> +       if ( (features & NETIF_F_RXHASH) &&
> +            !(features & NETIF_F_RXCSUM) &&
> +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM)) {
> +               netdev_warn(dev, "receive hashing forces receive checksum\n");
> +               features |= NETIF_F_RXCSUM;
> +       }
>
>        return features;
>  }

I suggest using netdev_dbg() instead of netdev_warn() so it won't spam
dmesg on every features update.

Otherwise, looks good.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - July 8, 2011, 3:32 p.m.
On Fri, 8 Jul 2011 10:45:33 +0200
Michał Mirosław <mirqus@gmail.com> wrote:

> 2011/7/8 Stephen Hemminger <shemminger@vyatta.com>:
> > Found when reviewing the vendor driver. Apparently some chip versions
> > require receive checksumming to be enabled in order for RSS to work.
> >
> > Also, if fix_features has to change some settings; put in message
> > in log in similar manner to netdev_fix_features.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> > ---
> > v2 - enforce the requirement in fix_features rather than set features
> >
> > --- a/drivers/net/sky2.c        2011-07-07 13:56:03.575420273 -0700
> > +++ b/drivers/net/sky2.c        2011-07-07 14:03:17.775403938 -0700
> [...]
> > @@ -4176,8 +4180,18 @@ static u32 sky2_fix_features(struct net_
> >        /* In order to do Jumbo packets on these chips, need to turn off the
> >         * transmit store/forward. Therefore checksum offload won't work.
> >         */
> > -       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U)
> > +       if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U) {
> > +               netdev_warn(dev, "checksum offload not possible with jumbo frames\n");
> >                features &= ~(NETIF_F_TSO|NETIF_F_SG|NETIF_F_ALL_CSUM);
> > +       }
> > +
> > +       /* Some hardware requires receive checksum for RSS to work. */
> > +       if ( (features & NETIF_F_RXHASH) &&
> > +            !(features & NETIF_F_RXCSUM) &&
> > +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM)) {
> > +               netdev_warn(dev, "receive hashing forces receive checksum\n");
> > +               features |= NETIF_F_RXCSUM;
> > +       }
> >
> >        return features;
> >  }
> 
> I suggest using netdev_dbg() instead of netdev_warn() so it won't spam
> dmesg on every features update.

I was just copying existing policy in netdev_fix_features.
netdev_dbg() is useless because most user turn off debug messages.
Maybe notice or info is more appropriate level here.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - July 8, 2011, 3:54 p.m.
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 7 Jul 2011 16:40:00 -0700

> Found when reviewing the vendor driver. Apparently some chip versions
> require receive checksumming to be enabled in order for RSS to work.
> 
> Also, if fix_features has to change some settings; put in message
> in log in similar manner to netdev_fix_features.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> v2 - enforce the requirement in fix_features rather than set features

All 5 patches applied, I changed this patch #1 to use netdev_info()
as per the discussion.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/net/sky2.c	2011-07-07 13:56:03.575420273 -0700
+++ b/drivers/net/sky2.c	2011-07-07 14:03:17.775403938 -0700
@@ -2996,7 +2996,8 @@  static int __devinit sky2_init(struct sk
 		hw->flags = SKY2_HW_GIGABIT
 			| SKY2_HW_NEWER_PHY
 			| SKY2_HW_NEW_LE
-			| SKY2_HW_ADV_POWER_CTL;
+			| SKY2_HW_ADV_POWER_CTL
+			| SKY2_HW_RSS_CHKSUM;
 
 		/* New transmit checksum */
 		if (hw->chip_rev != CHIP_REV_YU_EX_B0)
@@ -3024,7 +3025,7 @@  static int __devinit sky2_init(struct sk
 
 		/* The workaround for status conflicts VLAN tag detection. */
 		if (hw->chip_rev == CHIP_REV_YU_FE2_A0)
-			hw->flags |= SKY2_HW_VLAN_BROKEN;
+			hw->flags |= SKY2_HW_VLAN_BROKEN | SKY2_HW_RSS_CHKSUM;
 		break;
 
 	case CHIP_ID_YUKON_SUPR:
@@ -3033,6 +3034,9 @@  static int __devinit sky2_init(struct sk
 			| SKY2_HW_NEW_LE
 			| SKY2_HW_AUTO_TX_SUM
 			| SKY2_HW_ADV_POWER_CTL;
+
+		if (hw->chip_rev == CHIP_REV_YU_SU_A0)
+			hw->flags |= SKY2_HW_RSS_CHKSUM;
 		break;
 
 	case CHIP_ID_YUKON_UL_2:
@@ -4176,8 +4180,18 @@  static u32 sky2_fix_features(struct net_
 	/* In order to do Jumbo packets on these chips, need to turn off the
 	 * transmit store/forward. Therefore checksum offload won't work.
 	 */
-	if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U)
+	if (dev->mtu > ETH_DATA_LEN && hw->chip_id == CHIP_ID_YUKON_EC_U) {
+		netdev_warn(dev, "checksum offload not possible with jumbo frames\n");
 		features &= ~(NETIF_F_TSO|NETIF_F_SG|NETIF_F_ALL_CSUM);
+	}
+
+	/* Some hardware requires receive checksum for RSS to work. */
+	if ( (features & NETIF_F_RXHASH) &&
+	     !(features & NETIF_F_RXCSUM) &&
+	     (sky2->hw->flags & SKY2_HW_RSS_CHKSUM)) {
+		netdev_warn(dev, "receive hashing forces receive checksum\n");
+		features |= NETIF_F_RXCSUM;
+	}
 
 	return features;
 }
--- a/drivers/net/sky2.h	2011-07-07 13:56:03.563420272 -0700
+++ b/drivers/net/sky2.h	2011-07-07 13:56:07.719420117 -0700
@@ -2281,6 +2281,7 @@  struct sky2_hw {
 #define SKY2_HW_ADV_POWER_CTL	0x00000080	/* additional PHY power regs */
 #define SKY2_HW_RSS_BROKEN	0x00000100
 #define SKY2_HW_VLAN_BROKEN     0x00000200
+#define SKY2_HW_RSS_CHKSUM	0x00000400	/* RSS requires chksum */
 
 	u8	     	     chip_id;
 	u8		     chip_rev;