[Intel-wired-lan] [PATCH 4/4] igc: Refactor VLAN priority filtering code

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Hi,

Andre Guedes <andre.guedes@intel.com> writes:

> The whole VLAN priority filtering code is implemented in igc_ethtool.c
> and mixes logic from ethtool and core parts. This patch refactors it so
> core logic is moved to igc_main.c, aligning the VLAN priority filtering
> code organization with the MAC address filtering code.
>
> This patch also takes the opportunity to add some log messages to ease
> debugging.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h         |  3 +
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 64 ++++----------------
>  drivers/net/ethernet/intel/igc/igc_main.c    | 52 ++++++++++++++++
>  3 files changed, 68 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 1b07e8b870c2..48eb9c00a44d 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -235,6 +235,9 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>  		       const s8 queue, const u8 flags);
>  int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>  		       const u8 flags);
> +int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio,
> +			     int queue);
> +void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio);
>  void igc_update_stats(struct igc_adapter *adapter);
>  
>  /* igc_dump declarations */
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 036a2244b76c..35bc125183a0 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1223,35 +1223,6 @@ static int igc_rxnfc_write_etype_filter(struct igc_adapter *adapter,
>  	return 0;
>  }
>  
> -static int igc_rxnfc_write_vlan_prio_filter(struct igc_adapter *adapter,
> -					    struct igc_nfc_filter *input)
> -{
> -	struct igc_hw *hw = &adapter->hw;
> -	u8 vlan_priority;
> -	u16 queue_index;
> -	u32 vlapqf;
> -
> -	vlapqf = rd32(IGC_VLANPQF);
> -	vlan_priority = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> -				>> VLAN_PRIO_SHIFT;
> -	queue_index = (vlapqf >> (vlan_priority * 4)) & IGC_VLANPQF_QUEUE_MASK;
> -
> -	/* check whether this vlan prio is already set */
> -	if (vlapqf & IGC_VLANPQF_VALID(vlan_priority) &&
> -	    queue_index != input->action) {
> -		netdev_err(adapter->netdev,
> -			   "ethtool rxnfc set vlan prio filter failed");
> -		return -EEXIST;
> -	}
> -
> -	vlapqf |= IGC_VLANPQF_VALID(vlan_priority);
> -	vlapqf |= IGC_VLANPQF_QSEL(vlan_priority, input->action);
> -
> -	wr32(IGC_VLANPQF, vlapqf);
> -
> -	return 0;
> -}
> -
>  int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
>  {
>  	struct igc_hw *hw = &adapter->hw;
> @@ -1285,10 +1256,15 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
>  			return err;
>  	}
>  
> -	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
> -		err = igc_rxnfc_write_vlan_prio_filter(adapter, input);
> +	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
> +		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> +				>> VLAN_PRIO_SHIFT;
> +		err = igc_add_vlan_prio_filter(adapter, prio, input->action);
> +		if (err)
> +			return err;
> +	}
>  
> -	return err;
> +	return 0;
>  }
>  
>  static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
> @@ -1306,31 +1282,17 @@ static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
>  	adapter->etype_bitmap[reg_index] = false;
>  }
>  
> -static void igc_clear_vlan_prio_filter(struct igc_adapter *adapter,
> -				       u16 vlan_tci)
> -{
> -	struct igc_hw *hw = &adapter->hw;
> -	u8 vlan_priority;
> -	u32 vlapqf;
> -
> -	vlan_priority = (vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
> -
> -	vlapqf = rd32(IGC_VLANPQF);
> -	vlapqf &= ~IGC_VLANPQF_VALID(vlan_priority);
> -	vlapqf &= ~IGC_VLANPQF_QSEL(vlan_priority, IGC_VLANPQF_QUEUE_MASK);
> -
> -	wr32(IGC_VLANPQF, vlapqf);
> -}
> -
>  int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
>  {
>  	if (input->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
>  		igc_clear_etype_filter_regs(adapter,
>  					    input->etype_reg_index);
>  
> -	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
> -		igc_clear_vlan_prio_filter(adapter,
> -					   ntohs(input->filter.vlan_tci));
> +	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
> +		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> +				>> VLAN_PRIO_SHIFT;

As you are refactoring this, one suggestion/question: is it possible to
move the ntohs() and friends closer to writing/reading to/from the
registers?

> +		igc_del_vlan_prio_filter(adapter, prio);
> +	}
>  
>  	if (input->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR)
>  		igc_del_mac_filter(adapter, input->filter.src_addr,
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index c3555148ca0e..70f861b418b2 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2315,6 +2315,58 @@ int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>  	return 0;
>  }
>  
> +/**
> + * igc_add_vlan_prio_filter() - Add VLAN priority filter.
> + * @adapter: Pointer to adapter where the filter should be added.
> + * @prio: VLAN priority value.
> + * @queue: Queue number which matching frames are assigned to.
> + *
> + * Return: 0 in case of success, negative errno code otherwise.
> + */
> +int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio, int queue)
> +{
> +	struct net_device *dev = adapter->netdev;
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 vlanpqf;
> +
> +	vlanpqf = rd32(IGC_VLANPQF);
> +
> +	if (vlanpqf & IGC_VLANPQF_VALID(prio)) {
> +		netdev_dbg(dev, "VLAN priority filter already in use");
> +		return -EEXIST;
> +	}
> +
> +	vlanpqf |= IGC_VLANPQF_QSEL(prio, queue);
> +	vlanpqf |= IGC_VLANPQF_VALID(prio);
> +
> +	wr32(IGC_VLANPQF, vlanpqf);
> +
> +	netdev_dbg(dev, "Add VLAN priority filter: prio %d queue %d",
> +		   prio, queue);

As you added a way to dump this register, I don't think this debug
statement, and the one below, are that useful.

> +	return 0;
> +}
> +
> +/**
> + * igc_del_vlan_prio_filter() - Delete VLAN priority filter.
> + * @adapter: Pointer to adapter where the filter should be deleted from.
> + * @prio: VLAN priority value.
> + */
> +void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 vlanpqf;
> +
> +	vlanpqf = rd32(IGC_VLANPQF);
> +
> +	vlanpqf &= ~IGC_VLANPQF_VALID(prio);
> +	vlanpqf |= ~IGC_VLANPQF_QSEL(prio, 0);
> +
> +	wr32(IGC_VLANPQF, vlanpqf);
> +
> +	netdev_dbg(adapter->netdev, "Delete VLAN priority filter: prio %d",
> +		   prio);
> +}
> +
>  static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
>  {
>  	struct igc_adapter *adapter = netdev_priv(netdev);
> -- 
> 2.26.0
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

-- 
Vinicius