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 19:40:10 +0200
From:	Michał Mirosław <mirqus@...il.com>
To:	Stephen Hemminger <shemminger@...tta.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

W dniu 7 lipca 2011 19:13 użytkownik Stephen Hemminger
<shemminger@...tta.com> napisał:
> 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.

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