[Intel-wired-lan] [PATCH net v1] iavf: Fix promiscuous mode configuration flow messages

Nguyen, Anthony L anthony.l.nguyen at intel.com
Fri Sep 3 18:32:34 UTC 2021


On Thu, 2021-09-02 at 14:44 +0200, Karen Sornek wrote:
> Currently when configuring promiscuous mode on the AVF we detect a
> change in the netdev->flags. We use IFF_PROMISC and IFF_ALLMULTI to
> determine whether or not we need to request/release promiscuous mode
> and/or multicast promiscuous mode. The problem is that the AQ calls
> for
> setting/clearing promiscuous/multicast mode are treated separately.
> This
> leads to a case where we can trigger two promiscuous mode AQ calls in
> a row with the incorrect state. To fix this make a few changes.
> 
> Use IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE instead of the previous
> IAVF_FLAG_AQ_[REQUEST|RELEASE]_[PROMISC|ALLMULTI] flags.
> 
> In iavf_set_rx_mode() detect if there is a change in the
> netdev->flags in comparison with adapter->flags and set the
> IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE aq_required bit. Then in
> iavf_process_aq_command() only check for
> IAVF_FLAG_CONFIGURE_PROMISC_MODE
> and call iavf_set_promiscuous() if it's set.
> 
> In iavf_set_promiscuous() check again to see which (if any)
> promiscuous mode bits have changed when comparing the netdev->flags
> with
> the adapter->flags. Use this to set the flags which get sent to the
> PF
> driver.
> 
> Add a spinlock that is used for updating current_netdev_promisc_flags
> and only allows one promiscuous mode AQ at a time.
> 
> [1] Fixes the fact that we will only have one AQ call in the
> aq_required
> queue at any one time.
> 
> [2] Streamlines the change in promiscuous mode to only set one AQ
> required bit.
> 
> [3] This allows us to keep track of the current state of the flags
> and
> also makes it so we can take the most recent netdev->flags
> promiscuous
> mode state.
> 
> [4] This fixes the problem where a change in the netdev->flags can
> cause
> IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE to be set in iavf_set_rx_mode(),
> but cleared in iavf_set_promiscuous() before the change is ever made
> via
> AQ call.
> 
> Also, break up long line print statements in iavf_set_promiscuous().

Strings are not exempt from long line, no need to break them up.

Also, this patch does not apply.

> Fixes: 129cf89e5856 ("iavf: rename functions and structs to new
> name")
> Signed-off-by: Brett Creeley brett.creeley at intel.com
> Signed-off-by: Karen Sornek karen.sornek at intel.com
> ---
>  drivers/net/ethernet/intel/iavf/iavf.h        | 20 +++--
>  drivers/net/ethernet/intel/iavf/iavf_main.c   | 46 +++++------
>  .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 77 ++++++++++++-----
> --
>  3 files changed, 82 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index 21c957755..b3e145ebf 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -267,8 +267,6 @@ struct iavf_adapter {
>  #define IAVF_FLAG_CLIENT_NEEDS_OPEN		BIT(10)
>  #define IAVF_FLAG_CLIENT_NEEDS_CLOSE		BIT(11)
>  #define IAVF_FLAG_CLIENT_NEEDS_L2_PARAMS	BIT(12)
> -#define IAVF_FLAG_PROMISC_ON			BIT(13)
> -#define IAVF_FLAG_ALLMULTI_ON			BIT(14)
>  #define IAVF_FLAG_LEGACY_RX			BIT(15)
>  #define IAVF_FLAG_REINIT_ITR_NEEDED		BIT(16)
>  #define IAVF_FLAG_QUEUES_DISABLED		BIT(17)
> @@ -292,10 +290,7 @@ struct iavf_adapter {
>  #define IAVF_FLAG_AQ_SET_HENA			BIT(12)
>  #define IAVF_FLAG_AQ_SET_RSS_KEY		BIT(13)
>  #define IAVF_FLAG_AQ_SET_RSS_LUT		BIT(14)
> -#define IAVF_FLAG_AQ_REQUEST_PROMISC		BIT(15)
> -#define IAVF_FLAG_AQ_RELEASE_PROMISC		BIT(16)
> -#define IAVF_FLAG_AQ_REQUEST_ALLMULTI		BIT(17)
> -#define IAVF_FLAG_AQ_RELEASE_ALLMULTI		BIT(18)
> +#define IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE	BIT(15)
>  #define IAVF_FLAG_AQ_ENABLE_VLAN_STRIPPING	BIT(19)
>  #define IAVF_FLAG_AQ_DISABLE_VLAN_STRIPPING	BIT(20)
>  #define IAVF_FLAG_AQ_ENABLE_CHANNELS		BIT(21)
> @@ -307,6 +302,16 @@ struct iavf_adapter {
>  #define IAVF_FLAG_AQ_ADD_ADV_RSS_CFG		BIT(27)
>  #define IAVF_FLAG_AQ_DEL_ADV_RSS_CFG		BIT(28)
>  
> +	/* Lock to prevent possible clobbering of
> +	 * current_netdev_promisc_flags
> +	 */
> +	spinlock_t current_netdev_promisc_flags_lock;
> +#ifdef HAVE_RHEL6_NET_DEVICE_OPS_EXT
> +	u32 current_netdev_promisc_flags;
> +#else
> +	netdev_features_t current_netdev_promisc_flags;
> +#endif /* HAVE_RHEL6_NET_DEVICE_OPS_EXT */

What is this HAVE*?

> +
>  	/* OS defined structs */
>  	struct net_device *netdev;
>  	struct pci_dev *pdev;
> @@ -426,7 +431,8 @@ void iavf_add_ether_addrs(struct iavf_adapter
> *adapter);
>  void iavf_del_ether_addrs(struct iavf_adapter *adapter);
>  void iavf_add_vlans(struct iavf_adapter *adapter);
>  void iavf_del_vlans(struct iavf_adapter *adapter);
> -void iavf_set_promiscuous(struct iavf_adapter *adapter, int flags);
> +void iavf_set_promiscuous(struct iavf_adapter *adapter);
> +bool iavf_promiscuous_mode_changed(struct iavf_adapter *adapter);
>  void iavf_request_stats(struct iavf_adapter *adapter);
>  void iavf_request_reset(struct iavf_adapter *adapter);
>  void iavf_get_hena(struct iavf_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 80437ef26..edd448bfb 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -889,6 +889,16 @@ static int iavf_addr_unsync(struct net_device
> *netdev, const u8 *addr)
>  	return 0;
>  }
>  
> +/**
> + * iavf_promiscuous_mode_changed - check if promiscuous mode bits
> changed
> + * @adapter: device specific adapter
> + */
> +bool iavf_promiscuous_mode_changed(struct iavf_adapter *adapter)
> +{
> +        return (adapter->current_netdev_promisc_flags ^ adapter-
> >netdev->flags)
> +                & (IFF_PROMISC | IFF_ALLMULTI);
> +}
> +
>  /**
>   * iavf_set_rx_mode - NDO callback to set the netdev filters
>   * @netdev: network interface device structure
> @@ -902,19 +912,11 @@ static void iavf_set_rx_mode(struct net_device
> *netdev)
>  	__dev_mc_sync(netdev, iavf_addr_sync, iavf_addr_unsync);
>  	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>  
> -	if (netdev->flags & IFF_PROMISC &&
> -	    !(adapter->flags & IAVF_FLAG_PROMISC_ON))
> -		adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_PROMISC;
> -	else if (!(netdev->flags & IFF_PROMISC) &&
> -		 adapter->flags & IAVF_FLAG_PROMISC_ON)
> -		adapter->aq_required |= IAVF_FLAG_AQ_RELEASE_PROMISC;
> -
> -	if (netdev->flags & IFF_ALLMULTI &&
> -	    !(adapter->flags & IAVF_FLAG_ALLMULTI_ON))
> -		adapter->aq_required |= IAVF_FLAG_AQ_REQUEST_ALLMULTI;
> -	else if (!(netdev->flags & IFF_ALLMULTI) &&
> -		 adapter->flags & IAVF_FLAG_ALLMULTI_ON)
> -		adapter->aq_required |= IAVF_FLAG_AQ_RELEASE_ALLMULTI;
> +	spin_lock_bh(&adapter->current_netdev_promisc_flags_lock);
> +
> +	if (iavf_promiscuous_mode_changed(adapter))
> +		adapter->aq_required |=
> IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE;
> +	spin_unlock_bh(&adapter->current_netdev_promisc_flags_lock);
>  }
>  
>  /**
> @@ -1642,20 +1644,9 @@ static int iavf_process_aq_command(struct
> iavf_adapter *adapter)
>  		return 0;
>  	}
>  
> -	if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_PROMISC) {
> -		iavf_set_promiscuous(adapter, FLAG_VF_UNICAST_PROMISC |
> -				       FLAG_VF_MULTICAST_PROMISC);
> -		return 0;
> -	}
> -
> -	if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_ALLMULTI) {
> -		iavf_set_promiscuous(adapter,
> FLAG_VF_MULTICAST_PROMISC);
> -		return 0;
> -	}
> -	if ((adapter->aq_required & IAVF_FLAG_AQ_RELEASE_PROMISC) ||
> -	    (adapter->aq_required & IAVF_FLAG_AQ_RELEASE_ALLMULTI)) {
> -		iavf_set_promiscuous(adapter, 0);
> -		return 0;
> +	if (adapter->aq_required & IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE)
> {
> +		iavf_set_promiscuous(adapter);
> +		return IAVF_SUCCESS;

Why change from previous return 0 to IAVF_SUCCESS? This function
returns int, not iavf_status.

>  	}
>  
>  	if (adapter->aq_required & IAVF_FLAG_AQ_ENABLE_CHANNELS) {
> @@ -3840,6 +3831,7 @@ static int iavf_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  
>  	spin_lock_init(&adapter->mac_vlan_list_lock);
>  	spin_lock_init(&adapter->cloud_filter_list_lock);
> +	spin_lock_init(&adapter->current_netdev_promisc_flags_lock);
>  	spin_lock_init(&adapter->fdir_fltr_lock);
>  	spin_lock_init(&adapter->adv_rss_lock);
>  
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 9c128462e..401af605a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -729,47 +729,70 @@ void iavf_del_vlans(struct iavf_adapter
> *adapter)
>   *
>   * Request that the PF enable promiscuous mode for our VSI.
>   **/
> -void iavf_set_promiscuous(struct iavf_adapter *adapter, int flags)
> +void iavf_set_promiscuous(struct iavf_adapter *adapter)
>  {
> +	struct net_device *netdev = adapter->netdev;
>  	struct virtchnl_promisc_info vpi;
> -	int promisc_all;
> +	unsigned int flags;
>  
>  	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
>  		/* bail because we already have a command pending */
> -		dev_err(&adapter->pdev->dev, "Cannot set promiscuous
> mode, command %d pending\n",
> +		dev_err(&adapter->pdev->dev,
> +			"Cannot set promiscuous mode, command %d
> pending\n",
>  			adapter->current_op);
>  		return;
>  	}
>  
> -	promisc_all = FLAG_VF_UNICAST_PROMISC |
> -		      FLAG_VF_MULTICAST_PROMISC;
> -	if ((flags & promisc_all) == promisc_all) {
> -		adapter->flags |= IAVF_FLAG_PROMISC_ON;
> -		adapter->aq_required &= ~IAVF_FLAG_AQ_REQUEST_PROMISC;
> -		dev_info(&adapter->pdev->dev, "Entering promiscuous
> mode\n");
> -	}
> +	/* prevent changes to promiscuous flags */
> +	spin_lock_bh(&adapter->current_netdev_promisc_flags_lock);
>  
> -	if (flags & FLAG_VF_MULTICAST_PROMISC) {
> -		adapter->flags |= IAVF_FLAG_ALLMULTI_ON;
> -		adapter->aq_required &= ~IAVF_FLAG_AQ_REQUEST_ALLMULTI;
> -		dev_info(&adapter->pdev->dev, "%s is entering multicast
> promiscuous mode\n",
> -			 adapter->netdev->name);
> +	/* sanity check to prevent duplicate AQ calls */
> +	if (!iavf_promiscuous_mode_changed(adapter)) {
> +		adapter->aq_required &=
> ~IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE;
> +		dev_dbg(&adapter->pdev->dev, "No change in promiscuous
> mode\n");
> +		/* allow changes to promiscuous flags */
> +		spin_unlock_bh(&adapter-
> >current_netdev_promisc_flags_lock);
> +		return;
>  	}
>  
> -	if (!flags) {
> -		if (adapter->flags & IAVF_FLAG_PROMISC_ON) {
> -			adapter->flags &= ~IAVF_FLAG_PROMISC_ON;
> -			adapter->aq_required &=
> ~IAVF_FLAG_AQ_RELEASE_PROMISC;
> -			dev_info(&adapter->pdev->dev, "Leaving
> promiscuous mode\n");
> +	/* there are 2 bits, but only 3 states */
> +	if (!(netdev->flags & IFF_PROMISC) &&
> +	    netdev->flags & IFF_ALLMULTI) {
> +		/* State 1  - only multicast promiscuous mode enabled
> +		 * - !IFF_PROMISC && IFF_ALLMULTI
> +		 */
> +		flags = FLAG_VF_MULTICAST_PROMISC;
> +		adapter->current_netdev_promisc_flags |= IFF_ALLMULTI;
> +		adapter->current_netdev_promisc_flags &= ~IFF_PROMISC;
> +		dev_info(&adapter->pdev->dev,
> +			 "Entering multicast promiscuous mode\n");
> +		} else if (!(netdev->flags & IFF_PROMISC) &&
> +		   !(netdev->flags & IFF_ALLMULTI)) {

This indent looks off

> +		/* State 2 - unicast/multicast promiscuous mode
> disabled
> +		 * - !IFF_PROMISC && !IFF_ALLMULTI
> +		 */
> +		flags = 0;
> +		adapter->current_netdev_promisc_flags &=
> +			~(IFF_PROMISC | IFF_ALLMULTI);
> +		dev_info(&adapter->pdev->dev, "Leaving promiscuous
> mode\n");
> +	} else {
> +		/* State 3 - unicast/multicast promiscuous mode enabled
> +		 * - IFF_PROMISC && IFF_ALLMULTI
> +		 * - IFF_PROMISC && !IFF_ALLMULTI
> +		 */
> +		flags = FLAG_VF_UNICAST_PROMISC |
> FLAG_VF_MULTICAST_PROMISC;
> +		adapter->current_netdev_promisc_flags |= IFF_PROMISC;
> +		if (netdev->flags & IFF_ALLMULTI)
> +			adapter->current_netdev_promisc_flags |=
> IFF_ALLMULTI;
> +		else
> +			adapter->current_netdev_promisc_flags &=
> ~IFF_ALLMULTI;
>  		}
> +		dev_info(&adapter->pdev->dev, "Entering promiscuous
> mode\n");
>  
> -		if (adapter->flags & IAVF_FLAG_ALLMULTI_ON) {
> -			adapter->flags &= ~IAVF_FLAG_ALLMULTI_ON;
> -			adapter->aq_required &=
> ~IAVF_FLAG_AQ_RELEASE_ALLMULTI;
> -			dev_info(&adapter->pdev->dev, "%s is leaving
> multicast promiscuous mode\n",
> -				 adapter->netdev->name);
> -		}
> -	}
> +	adapter->aq_required &= ~IAVF_FLAG_AQ_CONFIGURE_PROMISC_MODE;
> +
> +	/* allow changes to promiscuous flags */
> +	spin_unlock_bh(&adapter->current_netdev_promisc_flags_lock);
>  
>  	adapter->current_op = VIRTCHNL_OP_CONFIG_PROMISCUOUS_MODE;
>  	vpi.vsi_id = adapter->vsi_res->vsi_id;


More information about the Intel-wired-lan mailing list