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

login
register
mail settings
Submitter stephen hemminger
Date July 7, 2011, 3:50 p.m.
Message ID <20110707155212.000920741@vyatta.com>
Download mbox | patch
Permalink /patch/103697/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

stephen hemminger - July 7, 2011, 3:50 p.m.
Found when reviewing the vendor driver. Apparently some chip versions
require receive checksumming to be enabled in order for RSS to work.
Rather than silently enabling checksumming which is what the vendor
driver does, return an error to the user instead.

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




--
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 7, 2011, 5:04 p.m.
2011/7/7 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.
> Rather than silently enabling checksumming which is what the vendor
> driver does, return an error to the user instead.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
>
> --- a/drivers/net/sky2.c        2011-07-07 08:36:10.748003369 -0700
> +++ b/drivers/net/sky2.c        2011-07-07 08:36:37.372004595 -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:
> @@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
>        struct sky2_port *sky2 = netdev_priv(dev);
>        u32 changed = dev->features ^ features;
>
> +       /* Some hardware requires receive checksum for RSS to work. */
> +       if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
> +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
> +               return -EINVAL;
> +
>        if (changed & NETIF_F_RXCSUM) {
>                u32 on = features & NETIF_F_RXCSUM;
>                sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),

Nah. This won't work as the error is not passed to user (except via
dmesg) as there is no way to do it. Failing ndo_set_features() without
updating dev->features will also prevent other unrelated changes to
features set.

This should either force RXCSUM when RXHASH is on or disable RXHASH
when RXCSUM is off (both in sky2_fix_features()). Or maybe enable
RXCSUM with RXHASH but ignore hardware-computed checksum when it's not
requested.

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 7, 2011, 5:13 p.m.
On Thu, 7 Jul 2011 19:04:46 +0200
Michał Mirosław <mirqus@gmail.com> wrote:

> 2011/7/7 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.
> > Rather than silently enabling checksumming which is what the vendor
> > driver does, return an error to the user instead.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> >
> > --- a/drivers/net/sky2.c        2011-07-07 08:36:10.748003369 -0700
> > +++ b/drivers/net/sky2.c        2011-07-07 08:36:37.372004595 -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:
> > @@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
> >        struct sky2_port *sky2 = netdev_priv(dev);
> >        u32 changed = dev->features ^ features;
> >
> > +       /* Some hardware requires receive checksum for RSS to work. */
> > +       if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
> > +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
> > +               return -EINVAL;
> > +
> >        if (changed & NETIF_F_RXCSUM) {
> >                u32 on = features & NETIF_F_RXCSUM;
> >                sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
> 
> Nah. This won't work as the error is not passed to user (except via
> dmesg) as there is no way to do it. Failing ndo_set_features() without
> updating dev->features will also prevent other unrelated changes to
> features set.

I think this needs to be fixed in the netdev core then.
There are bound to be hardware types that can support only some combinations
of features. It should be passed back like all other errors.
--
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 7, 2011, 5:40 p.m.
W dniu 7 lipca 2011 19:13 użytkownik Stephen Hemminger
<shemminger@vyatta.com> napisał:
> On Thu, 7 Jul 2011 19:04:46 +0200
> Michał Mirosław <mirqus@gmail.com> wrote:
>
>> 2011/7/7 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.
>> > Rather than silently enabling checksumming which is what the vendor
>> > driver does, return an error to the user instead.
>> >
>> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>> >
>> >
>> > --- a/drivers/net/sky2.c        2011-07-07 08:36:10.748003369 -0700
>> > +++ b/drivers/net/sky2.c        2011-07-07 08:36:37.372004595 -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:
>> > @@ -4187,6 +4191,11 @@ static int sky2_set_features(struct net_
>> >        struct sky2_port *sky2 = netdev_priv(dev);
>> >        u32 changed = dev->features ^ features;
>> >
>> > +       /* Some hardware requires receive checksum for RSS to work. */
>> > +       if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
>> > +            (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
>> > +               return -EINVAL;
>> > +
>> >        if (changed & NETIF_F_RXCSUM) {
>> >                u32 on = features & NETIF_F_RXCSUM;
>> >                sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
>>
>> Nah. This won't work as the error is not passed to user (except via
>> dmesg) as there is no way to do it. Failing ndo_set_features() without
>> updating dev->features will also prevent other unrelated changes to
>> features set.
>
> I think this needs to be fixed in the netdev core then.
> There are bound to be hardware types that can support only some combinations
> of features. It should be passed back like all other errors.

This is what ndo_fix_features callback is for - it should alter passed
feature set to what combinations are permitted for the device's
current state.

Updating of the features is not always initiated by user (it might be
because, e.g. link speed changed) - in those cases the error has no
place to go.

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

Patch

--- a/drivers/net/sky2.c	2011-07-07 08:36:10.748003369 -0700
+++ b/drivers/net/sky2.c	2011-07-07 08:36:37.372004595 -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:
@@ -4187,6 +4191,11 @@  static int sky2_set_features(struct net_
 	struct sky2_port *sky2 = netdev_priv(dev);
 	u32 changed = dev->features ^ features;
 
+	/* Some hardware requires receive checksum for RSS to work. */
+	if ( (features & (NETIF_F_RXHASH|NETIF_F_RXCSUM)) == NETIF_F_RXHASH &&
+	     (sky2->hw->flags & SKY2_HW_RSS_CHKSUM))
+		return -EINVAL;
+
 	if (changed & NETIF_F_RXCSUM) {
 		u32 on = features & NETIF_F_RXCSUM;
 		sky2_write32(sky2->hw, Q_ADDR(rxqaddr[sky2->port], Q_CSR),
--- a/drivers/net/sky2.h	2011-07-06 21:57:43.155687362 -0700
+++ b/drivers/net/sky2.h	2011-07-07 08:36:37.372004595 -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;