[Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()

[Neftin, Sasha <sasha.neftin@intel.com>]

On 3/19/2020 01:00, Andre Guedes wrote:
> Current igc_rar_set_index() implementation is a bit convoluted so this
> patch does some code refactoring to improve it.
> 
> The helper igc_rar_set_index() is about writing MAC filter settings into
> hardware registers. Logic such as address validation belongs to
> functions upper in the call chain such as igc_set_mac() and
> igc_add_mac_filter(). So this patch moves the is_valid_ether_addr() call
> to igc_add_mac_filter(). No need to touch igc_set_mac() since it already
> checks it.
> 
> The variables 'rar_low' and 'rar_high' represent the value in registers
> RAL and RAH so we rename them to 'ral' and 'rah', respectivelly, to
> match the registers names.
> 
> To make it explicity, filter settings are passed as arguments to the
> function instead of reading them from adapter->mac_table "under the
> hood". Also, the function was renamed to igc_set_mac_filter_hw to make
> it more clear what it does.
> 
> Finally, the patch removes some wrfl() calls and comments not needed.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 75 +++++++++++++----------
>   1 file changed, 43 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index cc1e1b0286b3..0ca7afaf6fc7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -764,43 +764,53 @@ static void igc_setup_tctl(struct igc_adapter *adapter)
>   	wr32(IGC_TCTL, tctl);
>   }
>   
> -/**
> - * igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
> - * @adapter: address of board private structure
> - * @index: Index of the RAR entry which need to be synced with MAC table
> +/* Set MAC address filter in hardware.
Small correction to be consistently. Please, keep /** line above method 
declaration line.
> + *
> + * @adapter: Pointer to adapter where the filter should be set.
> + * @index: Filter index.
> + * @addr: Destination MAC address.
> + * @queue: If non-negative, queue assignment feature is enabled and frames
> + * matching the filter are enqueued onto 'queue'. Otherwise, queue assignment
> + * is disabled.
>    */
> -static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
> +static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
> +				  const u8 *addr, int queue)
>   {
> -	u8 *addr = adapter->mac_table[index].addr;
>   	struct igc_hw *hw = &adapter->hw;
> -	u32 rar_low, rar_high;
> +	u32 ral, rah;
>   
> -	/* HW expects these to be in network order when they are plugged
> -	 * into the registers which are little endian.  In order to guarantee
> -	 * that ordering we need to do an leXX_to_cpup here in order to be
> -	 * ready for the byteswap that occurs with writel
> -	 */
> -	rar_low = le32_to_cpup((__le32 *)(addr));
> -	rar_high = le16_to_cpup((__le16 *)(addr + 4));
> +	if (WARN_ON(index >= hw->mac.rar_entry_count))
> +		return;
>   
> -	if (adapter->mac_table[index].state & IGC_MAC_STATE_QUEUE_STEERING) {
> -		u8 queue = adapter->mac_table[index].queue;
> -		u32 qsel = IGC_RAH_QSEL_MASK & (queue << IGC_RAH_QSEL_SHIFT);
> +	ral = le32_to_cpup((__le32 *)(addr));
> +	rah = le16_to_cpup((__le16 *)(addr + 4));
>   
> -		rar_high |= qsel;
> -		rar_high |= IGC_RAH_QSEL_ENABLE;
> +	if (queue >= 0) {
> +		rah &= ~IGC_RAH_QSEL_MASK;
> +		rah |= (queue << IGC_RAH_QSEL_SHIFT);
> +		rah |= IGC_RAH_QSEL_ENABLE;
>   	}
>   
> -	/* Indicate to hardware the Address is Valid. */
> -	if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
> -		if (is_valid_ether_addr(addr))
> -			rar_high |= IGC_RAH_AV;
> -	}
> +	rah |= IGC_RAH_AV;
>   
> -	wr32(IGC_RAL(index), rar_low);
> -	wrfl();
> -	wr32(IGC_RAH(index), rar_high);
> -	wrfl();
> +	wr32(IGC_RAL(index), ral);
> +	wr32(IGC_RAH(index), rah);
> +}
> +
> +/* Clear MAC address filter in hardware.
Same here. Small correction to be consistently. Please, keep /** line 
above method declaration line.
> + *
> + * @adapter: Pointer to adapter where the filter should be cleared.
> + * @index: Filter index.
> + */
> +static void igc_clear_mac_filter_hw(struct igc_adapter *adapter, int index)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +
> +	if (WARN_ON(index >= hw->mac.rar_entry_count))
> +		return;
> +
> +	wr32(IGC_RAL(index), 0);
> +	wr32(IGC_RAH(index), 0);
>   }
>   
>   /* Set default MAC address for the PF in the first RAR entry */
> @@ -811,7 +821,7 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
>   	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
>   	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
>   
> -	igc_rar_set_index(adapter, 0);
> +	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
>   }
>   
>   /**
> @@ -2199,7 +2209,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   	int rar_entries = hw->mac.rar_entry_count;
>   	int i;
>   
> -	if (is_zero_ether_addr(addr))
> +	if (!is_valid_ether_addr(addr))
>   		return -EINVAL;
>   	if (flags & IGC_MAC_STATE_SRC_ADDR)
>   		return -ENOTSUPP;
> @@ -2217,7 +2227,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   		adapter->mac_table[i].queue = queue;
>   		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
>   
> -		igc_rar_set_index(adapter, i);
> +		igc_set_mac_filter_hw(adapter, i, addr, queue);
>   		return 0;
>   	}
>   
> @@ -2261,13 +2271,14 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   			adapter->mac_table[i].state =
>   				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
>   			adapter->mac_table[i].queue = 0;
> +			igc_set_mac_filter_hw(adapter, 0, addr, -1);
>   		} else {
>   			adapter->mac_table[i].state = 0;
>   			adapter->mac_table[i].queue = 0;
>   			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
> +			igc_clear_mac_filter_hw(adapter, i);
>   		}
>   
> -		igc_rar_set_index(adapter, i);
>   		return 0;
>   	}
>   
> 
Thanks Andre - two small nitpicks.