netdev - Re: [PATCH net-next 1/5] sky2: force receive checksum when using RSS on some hardware

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 7 Jul 2011 10:13:22 -0700
From:	Stephen Hemminger <shemminger@...tta.com>
To:	Michał Mirosław <mirqus@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/5] sky2: force receive checksum when using
 RSS on some hardware

On Thu, 7 Jul 2011 19:04:46 +0200
Michał Mirosław <mirqus@...il.com> wrote:

> 2011/7/7 Stephen Hemminger <shemminger@...tta.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@...tta.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html