Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set_mac callback

[Piotr Gardocki <piotrx.gardocki@intel.com>]

On 05.06.2023 21:02, Maciej Fijalkowski wrote:
> On Fri, Jun 02, 2023 at 10:13:00AM -0700, Tony Nguyen wrote:
>> From: Piotr Gardocki <piotrx.gardocki@intel.com>
>>
>> In some cases it is possible for kernel to come with request
>> to change primary MAC address to the address that is actually
>> already set on the given interface.
>>
>> If the old and new MAC addresses are equal there is no need
>> for going through entire routine, including AdminQ and
>> waitqueue.
>>
>> This patch adds proper check to return fast from the function
>> in these cases. The same check can also be found in i40e and
>> ice drivers.
> 
> couldn't this be checked the layer above then? and pulled out of drivers?
> 

Probably it could, but I can't tell for all drivers if such request should
always be ignored. I'm not aware of all possible use cases for this callback
to be called and I can imagine designs where such request should be
always handled.

>>
>> An example of such case is adding an interface to bonding
>> channel in balance-alb mode:
>> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
>> ip link set bond0 up
>> ifenslave bond0 <eth>
>>
>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>>  drivers/net/ethernet/intel/iavf/iavf_main.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 2de4baff4c20..420aaca548a0 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -1088,6 +1088,12 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>>  	if (!is_valid_ether_addr(addr->sa_data))
>>  		return -EADDRNOTAVAIL;
>>  
>> +	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
>> +		netdev_dbg(netdev, "already using mac address %pM\n",
>> +			   addr->sa_data);
> 
> i am not sure if this is helpful message, you end up with an address that
> you requested, why would you care that it was already same us you wanted?
> 

You can find similar message in i40e and ice drivers. Please note that this
is a debug message, so it won't print by default. I would leave it this way,
it might be useful in a future for debugging.

>> +		return 0;
>> +	}
>> +
>>  	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
>>  
>>  	if (ret)
>> -- 
>> 2.38.1
>>
>>

Regards,
Piotr