On Tue, Jun 06, 2023 at 11:22:55AM +0200, Piotr Gardocki wrote:

On 05.06.2023 21:02, Maciej Fijalkowski wrote:

quoted

On Fri, Jun 02, 2023 at 10:13:00AM -0700, Tony Nguyen wrote:

quoted

From: Piotr Gardocki <redacted>

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.

quoted

quoted

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 <redacted>
Reviewed-by: Michal Swiatkowski <redacted>
Tested-by: Rafal Romanowski <redacted>
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?

quoted

quoted

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

Regards,
Piotr