[05/12] igc: Refactor igc_rar_set_index()

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(-)

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.

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()
> 
> 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(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Quoting Neftin, Sasha (2020-03-31 04:22:12)
> > -/**
> > - * 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.

Yes, I can fix that. I thought we utilized kernel-doc style comments only for
APIs, not local helper functions like this.

@Jeff, what works better for your workflow, I send a v2 in-reply-to this patch
or I send a v2 of the whole patchset?

- Andre

Hi Jeff,

> > > -/**
> > > - * 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.
> 
> Yes, I can fix that. I thought we utilized kernel-doc style comments only for
> APIs, not local helper functions like this.
> 
> @Jeff, what works better for your workflow, I send a v2 in-reply-to this patch
> or I send a v2 of the whole patchset?

Since the modifications are very trivial and only 3 patches from this series
were actually updated, I sent the v2 patches in-reply-to. Hope that's fine.

- Andre