| 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, 11 Jul 2011 16:53:57 -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>
Subject: RE: [net-next 5/5] ixgbe: convert to ndo_fix_features
>-----Original Message-----
>From: Michal Miroslaw [mailto:mirq-linux@...e.qmqm.pl]
>Sent: Saturday, July 09, 2011 5:11 AM
>To: Kirsher, Jeffrey T
>Cc: davem@...emloft.net; Skidmore, Donald C; netdev@...r.kernel.org;
>gospo@...hat.com
>Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>
>On Sat, Jul 09, 2011 at 04:50:40AM -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.
>[...]
>> --- a/drivers/net/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ixgbe/ixgbe_main.c
>> @@ -7188,6 +7188,88 @@ int ixgbe_setup_tc(struct net_device *dev, u8
>tc)
>[...]
>> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
>> +{
>> + struct ixgbe_adapter *adapter = netdev_priv(netdev);
>> + bool need_reset = false;
>> +
>> +#ifdef CONFIG_DCB
>> + if ((adapter->flags & IXGBE_FLAG_DCB_ENABLED) &&
>> + !(data & NETIF_F_HW_VLAN_RX))
>> + return -EINVAL;
>> +#endif
>> + /* return error if RXHASH is being enabled when RSS is not
>supported */
>> + if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED) &&
>> + (data & NETIF_F_RXHASH))
>> + return -EINVAL;
>> +
>> + /* If Rx checksum is disabled, then RSC/LRO should also be
>disabled */
>> + if (!(data & NETIF_F_RXCSUM)) {
>> + data &= ~NETIF_F_LRO;
>> + adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
>> + } else {
>> + adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
>> + }
>
>The checks above should be done in ndo_fix_features callback. The check
>for
>RSS_ENABLED for RXHASH is redundant, as the feature is removed in
>probe()
>in this case (see [MARK] below).
Thanks for the comments Michal, it clears up a lot in my mind. :)
So in ndo_fix_features we would just turn off feature flags rather than return an error? For example:
if (!(data & NETIF_F_RXCSUM))
data &= ~NETIF_F_LRO;
As for RSS_ENABLED/RXHASH check it was to cover the cases where RSS_ENABLED might have changed since probe. This could happen with a resume were we don't get enough MSI-X vectors. There are also paths in FCoE and DCB that get into code that could clear IXGBE_FLAG_RSS_ENABLED.
>
>> +
>> + /* if state changes we need to update adapter->flags and reset */
>> + if ((adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) &&
>> + (!!(data & NETIF_F_LRO) !=
>> + !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED))) {
>> + if ((data & NETIF_F_LRO) &&
>> + adapter->rx_itr_setting != 1 &&
>> + adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
>> + e_info(probe, "rx-usecs set too low, "
>> + "not enabling RSC\n");
>> + } else {
>> + adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
>> + switch (adapter->hw.mac.type) {
>> + case ixgbe_mac_X540:
>> + case ixgbe_mac_82599EB:
>> + need_reset = true;
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> + }
>> +
>> + /*
>> + * 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;
>> + }
>
>Part of the checks above should be in ndo_fix_features, too.
>ndo_set_features
>should just enable what it has been passed. What ndo_fix_features
>callback
>returns is further limited by generic checks, and then (if the resulting
>set
>is different than current dev->features) ndo_set_features is called.
I'm a little confused here. From your comments I get the idea that ndo_fix_features() just modifies and error checks our feature set. The result of this would then be just a change to the feature set (data variable in my case above). Is that a correct assumption? If so I'm confused as none of the two checks above change the feature set. They do change driver flags to indicate the new state and mark whether we need a reset. I don't believe we would want to do the reset until ndo_set_feature is called and if we broke up the setting of the driver flags into ndo_fix_features we would lose some state (i.e. if the IXGBE_FLAG2_RSC_ENABLED changed) that we need to decide if a reset is needed in ndo_set_features.
Am I just missing something here?
Thanks
-Don
>
>> +
>> + 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,
>> @@ -7219,6 +7301,7 @@ 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,
>> };
>>
>> static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
>> @@ -7486,20 +7569,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;
>>
>> - 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;
>> break;
>> default:
>> break;
>> @@ -7538,6 +7625,8 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>> netdev->vlan_features |= NETIF_F_HIGHDMA;
>> }
>>
>> + if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)
>> + netdev->hw_features |= NETIF_F_LRO;
>> if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
>> netdev->features |= NETIF_F_LRO;
>>
>> @@ -7574,8 +7663,10 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>> if (err)
>> goto err_sw_init;
>>
>> - if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED))
>> + if (!(adapter->flags & IXGBE_FLAG_RSS_ENABLED)) {
>> + netdev->hw_features &= ~NETIF_F_RXHASH;
>> netdev->features &= ~NETIF_F_RXHASH;
>> + }
>>
>
>[MARK here]
>
>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
Powered by blists - more mailing lists