| 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
| ||
|
Date: Mon, 25 Jul 2011 15:42:09 -0700
From: "Skidmore, Donald C" <donald.c.skidmore@...el.com>
To: Michal Miroslaw <mirq-linux@...e.qmqm.pl>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"gospo@...hat.com" <gospo@...hat.com>,
"sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [net-next 10/10] ixgbe: convert to ndo_fix_features
>-----Original Message-----
>From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
>On Behalf Of Michal Miroslaw
>Sent: Friday, July 22, 2011 1:03 AM
>To: Kirsher, Jeffrey T
>Cc: davem@...emloft.net; Skidmore, Donald C; netdev@...r.kernel.org;
>gospo@...hat.com; sassmann@...hat.com
>Subject: Re: [net-next 10/10] ixgbe: convert to ndo_fix_features
>
>On Thu, Jul 21, 2011 at 11:09:11PM -0700, Jeff Kirsher wrote:
>> From: Don Skidmore <donald.c.skidmore@...el.com>
>>
>> Private rx_csum flags are now duplicate of netdev->features &
>> NETIF_F_RXCSUM. We remove those duplicates and now use the
>net_device_ops
>> ndo_set_features. This was based on the original patch submitted by
>> Michal Miroslaw <mirq-linux@...e.qmqm.pl>. I also removed the special
>> case not requiring a reset for X540 hardware. It is needed just as it
>is
>> in 82599 hardware.
>
>Looks mostly good now. Minor hints below.
>
>[...]
>> +static u32 ixgbe_fix_features(struct net_device *netdev, u32 data)
>> +{
>[...]
>> + /* Turn off LRO if not RSC capable or invalid ITR settings */
>> + if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)) {
>> + data &= ~NETIF_F_LRO;
>> + } else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
>> + (adapter->rx_itr_setting != 1 &&
>> + adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE)) {
>> + data &= ~NETIF_F_LRO;
>> + e_info(probe, "rx-usecs set too low, not enabling RSC\n");
>> + }
>
>Better:
>
>... else if (data & NETIF_F_LRO && adapter->rx_itr_setting != 1 &&
>adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
> e_info(...)
> data &= ~NETIF_F_LRO;
>}
>
I see your point here, the added complexity (checking IXGBE_FLAG2_RSC_ENABLED) is there to cover cases that come up in our out of tree driver's kcompat code. This is why we mirror these feature flags to begin with. I would like to modify the driver to use only the feature flags but that will require some interesting kcompat code as well as touch large parts of the driver so I was thinking would probably be best as another patch.
My concern in this conditional is when RSC is not just possible but actually turned on. Currently the NETIF_F_LRO flag is enabled in probe if we are RSC capable. But it is possible to be RSC capable but not have it enabled due to setting interrupts throttling rates too high.
>> +
>> + return data;
>> +}
>> +
>> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
>> +{
>> + struct ixgbe_adapter *adapter = netdev_priv(netdev);
>> + bool need_reset = false;
>> +
>> + /* If Rx checksum is disabled, then RSC/LRO should also be
>disabled */
>> + if (!(data & NETIF_F_RXCSUM))
>> + adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
>> + else
>> + adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
>
>This exactly mirrors NETIF_F_RXCSUM. Waiting for later cleanup?
That's pretty much it, I would like to clean up all of these after figuring out the best way to create the kcompat code for the out of tree driver that has to support old kernel versions. Seems like that would be worthy of its own patch.
>
>[...]
>> + /*
>> + * Check if Flow Director n-tuple support was enabled or disabled.
>If
>> + * the state changed, we need to reset.
>> + */
>> + if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
>> + /* turn off ATR, enable perfect filters and reset */
>> + if (data & NETIF_F_NTUPLE) {
>> + adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
>> + adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
>> + need_reset = true;
>> + }
>> + } else if (!(data & NETIF_F_NTUPLE)) {
>> + /* turn off Flow Director, set ATR and reset */
>> + adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
>> + if ((adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
>> + !(adapter->flags & IXGBE_FLAG_DCB_ENABLED))
>> + adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
>> + need_reset = true;
>> + }
>
>You could make this more readable:
>
>old = adapter->flags;
>if (data & NETIF_F_NTUPLE) {
> adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
>} else {
> adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> if ((adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
> !(adapter->flags & IXGBE_FLAG_DCB_ENABLED))
> adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
>}
>if (old != adapter->flags)
> need_reset = true;
>
I agree this is a lot more readable. Wish I would have thought of it. :)
I'll send out a new patch with at least this modification.
>> +
>> + if (need_reset)
>> + ixgbe_do_reset(netdev);
>> +
>> + return 0;
>> +
>> +}
>> +
>> static const struct net_device_ops ixgbe_netdev_ops = {
>> .ndo_open = ixgbe_open,
>> .ndo_stop = ixgbe_close,
>> @@ -7153,6 +7228,8 @@ static const struct net_device_ops
>ixgbe_netdev_ops = {
>> .ndo_fcoe_disable = ixgbe_fcoe_disable,
>> .ndo_fcoe_get_wwn = ixgbe_fcoe_get_wwn,
>> #endif /* IXGBE_FCOE */
>> + .ndo_set_features = ixgbe_set_features,
>> + .ndo_fix_features = ixgbe_fix_features,
>> };
>>
>> static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
>> @@ -7420,20 +7497,24 @@ static int __devinit ixgbe_probe(struct
>pci_dev *pdev,
>>
>> netdev->features = NETIF_F_SG |
>> NETIF_F_IP_CSUM |
>> + NETIF_F_IPV6_CSUM |
>> NETIF_F_HW_VLAN_TX |
>> NETIF_F_HW_VLAN_RX |
>> - NETIF_F_HW_VLAN_FILTER;
>> + NETIF_F_HW_VLAN_FILTER |
>> + NETIF_F_TSO |
>> + NETIF_F_TSO6 |
>> + NETIF_F_GRO |
>> + NETIF_F_RXHASH |
>> + NETIF_F_RXCSUM;
>
>Drop NETIF_F_GRO here, as its always set by network core now.
>
Will do.
>> - netdev->features |= NETIF_F_IPV6_CSUM;
>> - netdev->features |= NETIF_F_TSO;
>> - netdev->features |= NETIF_F_TSO6;
>> - netdev->features |= NETIF_F_GRO;
>> - netdev->features |= NETIF_F_RXHASH;
>> + netdev->hw_features = netdev->features;
>>
>> switch (adapter->hw.mac.type) {
>> case ixgbe_mac_82599EB:
>> case ixgbe_mac_X540:
>> netdev->features |= NETIF_F_SCTP_CSUM;
>> + netdev->hw_features |= NETIF_F_SCTP_CSUM |
>> + NETIF_F_NTUPLE;
>
>NTUPLE disabled by default. That's the idea?
We default with ATR on which disables perfect filters (NETIF_F_NTUPLE).
>
>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
--
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
Powered by blists - more mailing lists