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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Jun 06, 2023 at 11:22:55AM +0200, Piotr Gardocki wrote:
> 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.

if you can imagine a case where such request should be handled then i'm
all ears. it feels like this is in an optimization where everyone could
benefit from (no expert in this scope though), but yeah this callback went
into the wild and it's implemented all over the place.

> 
> >>
> >> 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.

hmm fair enough :) :
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

CC: Olek
do you think libie could implement common ndo callbacks?

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