Message-ID: <C0716E470783B24A8178C9E38CD2F34505E7682C@orsmsx509.amr.corp.intel.com>
Date:	Tue, 12 Jul 2011 14:28:15 -0700
From:	"Skidmore, Donald C" <donald.c.skidmore@...el.com>
To:	Michal Miroslaw <mirq-linux@...e.qmqm.pl>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"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: Tuesday, July 12, 2011 3:10 AM
>To: Skidmore, Donald C
>Cc: Kirsher, Jeffrey T; davem@...emloft.net; netdev@...r.kernel.org;
>gospo@...hat.com
>Subject: Re: [net-next 5/5] ixgbe: convert to ndo_fix_features
>
>On Mon, Jul 11, 2011 at 04:53:57PM -0700, Skidmore, Donald C wrote:
>> >-----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;
>
>Exactly.
>
>> 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.
>
>That makes sense now. The checks should be in ndo_fix_features and clear
>features whenever they are unavailable.
>

I'm seeing how this would work; most of my earlier checks should fall pretty naturally in to ndo_fix_feature. :)

>> >> +
>> >> +	/* 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))) {
>
>ndo_fix_features() should prevent the change if
>!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) is true.
>

The logic in this conditional is a little complex. I believe it would end up looking something like the following:

in fix_features:

	/* Prevent change if not RSC capable or invalid ITR setting */
	if (data & NETIF_F_LRO)
		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;
		}
	} 

in set_feature:

	/* Make sure RSC matches LRO, reset if change */
	if (!!(data & NETIF_F_LRO) != 
	    !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)) {
		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;	
		}
	}

>> >> +		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");
>
>And should clear NETIF_F_LRO when above condition is met.
>
>> >> +		} 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?
>
>I see that this changes only driver internal state, so your right in
>putting
>this in ndo_set_features callback.
>
>Can you draw a map of the relevant bits and what state should result
>from
>them (a truth table if you will)? I have a hard time understanding the
>idea.
>This changes _CAPABLE bits depending on _ENABLED which adds to
>confusion.
>
>I would expect that:
> _CAPABLE are set whenever a config is possible, disregarding
>dependencies
> _ENABLED are set whenever a config is requested and possible
>But it looks like what I described is reversed or just wrong.
>
>Best Regards,
>Michał Mirosław

Hopefully the code example above answers your questions.  I can see why you were having a hard time understanding it; I was making the FALSE assumption and not clearing the feature flags for the more complex conditionals. Thanks again for the clarifications, the netdev-features.txt helped a lot too. 

Assuming I'm on the right track now I'll code up a new patch and send it out after our validation runs.

Thanks,
-Don Skidmore <donald.c.skidmore@...el.com>
--
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