[PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Piotr Gardocki]

The first patch adds an if statement in core to skip early when
the MAC address is not being changes.
The remaining patches remove such checks from Intel drivers
as they're redundant at this point.

v3: removed "This patch ..." from first patch to simplify sentence.
v2: modified check in core to support addresses of any length,
removed redundant checks in i40e and ice

Piotr Gardocki (3):
  net: add check for current MAC address in dev_set_mac_address
  i40e: remove unnecessary check for old MAC == new MAC
  ice: remove unnecessary check for old MAC == new MAC

 drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ------
 drivers/net/ethernet/intel/ice/ice_main.c   | 5 -----
 net/core/dev.c                              | 2 ++
 3 files changed, 2 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH net-next v3 1/3] net: add check for current MAC address in dev_set_mac_address

[Piotr Gardocki]

In some cases it is possible for kernel to come with request
to change primary MAC address to the address that is already
set on the given interface.

Add proper check to return fast from the function in these cases.

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>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index c2456b3667fe..8f1c49ab17df 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8754,6 +8754,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+	if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len))
+		return 0;
 	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
 	if (err)
 		return err;
-- 
2.34.1

Re: [PATCH net-next v3 1/3] net: add check for current MAC address in dev_set_mac_address

[Simon Horman]

On Wed, Jun 14, 2023 at 04:53:00PM +0200, Piotr Gardocki wrote:
> In some cases it is possible for kernel to come with request
> to change primary MAC address to the address that is already
> set on the given interface.
> 
> Add proper check to return fast from the function in these cases.
> 
> 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: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v3 2/3] i40e: remove unnecessary check for old MAC == new MAC

[Piotr Gardocki]

The check has been moved to core. The ndo_set_mac_address callback
is not being called with new MAC address equal to the old one anymore.

Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b847bd105b16..29ad1797adce 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -1788,12 +1788,6 @@ static int i40e_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_info(netdev, "already using mac address %pM\n",
-			    addr->sa_data);
-		return 0;
-	}
-
 	if (test_bit(__I40E_DOWN, pf->state) ||
 	    test_bit(__I40E_RESET_RECOVERY_PENDING, pf->state))
 		return -EADDRNOTAVAIL;
-- 
2.34.1

Re: [PATCH net-next v3 2/3] i40e: remove unnecessary check for old MAC == new MAC

[Simon Horman]

On Wed, Jun 14, 2023 at 04:53:01PM +0200, Piotr Gardocki wrote:
> The check has been moved to core. The ndo_set_mac_address callback
> is not being called with new MAC address equal to the old one anymore.
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v3 3/3] ice: remove unnecessary check for old MAC == new MAC

[Piotr Gardocki]

The check has been moved to core. The ndo_set_mac_address callback
is not being called with new MAC address equal to the old one anymore.

Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index caafb12d9cc7..f0ba896a3f46 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5624,11 +5624,6 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
 	if (!is_valid_ether_addr(mac))
 		return -EADDRNOTAVAIL;
 
-	if (ether_addr_equal(netdev->dev_addr, mac)) {
-		netdev_dbg(netdev, "already using mac %pM\n", mac);
-		return 0;
-	}
-
 	if (test_bit(ICE_DOWN, pf->state) ||
 	    ice_is_reset_in_progress(pf->state)) {
 		netdev_err(netdev, "can't set mac %pM. device not ready\n",
-- 
2.34.1

Re: [PATCH net-next v3 3/3] ice: remove unnecessary check for old MAC == new MAC

[Simon Horman]

On Wed, Jun 14, 2023 at 04:53:02PM +0200, Piotr Gardocki wrote:
> The check has been moved to core. The ndo_set_mac_address callback
> is not being called with new MAC address equal to the old one anymore.
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Maciej Fijalkowski]

On Wed, Jun 14, 2023 at 04:52:59PM +0200, Piotr Gardocki wrote:
> The first patch adds an if statement in core to skip early when
> the MAC address is not being changes.
> The remaining patches remove such checks from Intel drivers
> as they're redundant at this point.
> 
> v3: removed "This patch ..." from first patch to simplify sentence.
> v2: modified check in core to support addresses of any length,
> removed redundant checks in i40e and ice

Any particular reason for not including my rev-by tags on driver patches?

> 
> Piotr Gardocki (3):
>   net: add check for current MAC address in dev_set_mac_address
>   i40e: remove unnecessary check for old MAC == new MAC
>   ice: remove unnecessary check for old MAC == new MAC
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ------
>  drivers/net/ethernet/intel/ice/ice_main.c   | 5 -----
>  net/core/dev.c                              | 2 ++
>  3 files changed, 2 insertions(+), 11 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Piotr Gardocki]

On 14.06.2023 17:50, Maciej Fijalkowski wrote:
> On Wed, Jun 14, 2023 at 04:52:59PM +0200, Piotr Gardocki wrote:
>> The first patch adds an if statement in core to skip early when
>> the MAC address is not being changes.
>> The remaining patches remove such checks from Intel drivers
>> as they're redundant at this point.
>>
>> v3: removed "This patch ..." from first patch to simplify sentence.
>> v2: modified check in core to support addresses of any length,
>> removed redundant checks in i40e and ice
> 
> Any particular reason for not including my rev-by tags on driver patches?

No, I just forgot I can include them when I'm not changing the entire
patch set. I'm sorry :(

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Maciej Fijalkowski]

On Wed, Jun 14, 2023 at 05:52:17PM +0200, Piotr Gardocki wrote:
> On 14.06.2023 17:50, Maciej Fijalkowski wrote:
> > On Wed, Jun 14, 2023 at 04:52:59PM +0200, Piotr Gardocki wrote:
> >> The first patch adds an if statement in core to skip early when
> >> the MAC address is not being changes.
> >> The remaining patches remove such checks from Intel drivers
> >> as they're redundant at this point.
> >>
> >> v3: removed "This patch ..." from first patch to simplify sentence.
> >> v2: modified check in core to support addresses of any length,
> >> removed redundant checks in i40e and ice
> > 
> > Any particular reason for not including my rev-by tags on driver patches?
> 
> No, I just forgot I can include them when I'm not changing the entire
> patch set. I'm sorry :(

No worries, I think we can ask Jakub to include them if he thinks current
state of this work is fine.

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Simon Horman]

On Wed, Jun 14, 2023 at 04:52:59PM +0200, Piotr Gardocki wrote:
> The first patch adds an if statement in core to skip early when
> the MAC address is not being changes.
> The remaining patches remove such checks from Intel drivers
> as they're redundant at this point.

Hi Pitor,

I am wondering if you audited other drivers to see if they
could also be updated in a similar way to the Intel changes
in this patch-set.

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Piotr Gardocki]

On 15.06.2023 09:31, Simon Horman wrote:
> On Wed, Jun 14, 2023 at 04:52:59PM +0200, Piotr Gardocki wrote:
>> The first patch adds an if statement in core to skip early when
>> the MAC address is not being changes.
>> The remaining patches remove such checks from Intel drivers
>> as they're redundant at this point.
> 
> Hi Pitor,
> 
> I am wondering if you audited other drivers to see if they
> could also be updated in a similar way to the Intel changes
> in this patch-set.

Hi Simon,

We already discussed this in "[PATCH net-next v2 3/3]". Other drivers
should be audited at one point, but there are hundreds of ndo_set_mac_address
callbacks in kernel and I don't have resources to review all of them. I just
fixed Intel drivers for now.
When this patch set is applied someone else can work on that.

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Simon Horman]

On Thu, Jun 15, 2023 at 10:51:28AM +0200, Piotr Gardocki wrote:
> On 15.06.2023 09:31, Simon Horman wrote:
> > On Wed, Jun 14, 2023 at 04:52:59PM +0200, Piotr Gardocki wrote:
> >> The first patch adds an if statement in core to skip early when
> >> the MAC address is not being changes.
> >> The remaining patches remove such checks from Intel drivers
> >> as they're redundant at this point.
> > 
> > Hi Pitor,
> > 
> > I am wondering if you audited other drivers to see if they
> > could also be updated in a similar way to the Intel changes
> > in this patch-set.
> 
> Hi Simon,
> 
> We already discussed this in "[PATCH net-next v2 3/3]". Other drivers
> should be audited at one point, but there are hundreds of ndo_set_mac_address
> callbacks in kernel and I don't have resources to review all of them. I just
> fixed Intel drivers for now.
> When this patch set is applied someone else can work on that.

Sorry, I forgot about that discussion.
Of course this is fine.

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 14 Jun 2023 16:52:59 +0200 you wrote:
> The first patch adds an if statement in core to skip early when
> the MAC address is not being changes.
> The remaining patches remove such checks from Intel drivers
> as they're redundant at this point.
> 
> v3: removed "This patch ..." from first patch to simplify sentence.
> v2: modified check in core to support addresses of any length,
> removed redundant checks in i40e and ice
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/3] net: add check for current MAC address in dev_set_mac_address
    https://git.kernel.org/netdev/net-next/c/ad72c4a06acc
  - [net-next,v3,2/3] i40e: remove unnecessary check for old MAC == new MAC
    https://git.kernel.org/netdev/net-next/c/c45a6d1a23c5
  - [net-next,v3,3/3] ice: remove unnecessary check for old MAC == new MAC
    https://git.kernel.org/netdev/net-next/c/96868cca7971

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

RE: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Fijalkowski, Maciej]

> 
> Hello:
> 
> This series was applied to netdev/net-next.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
> 
> On Wed, 14 Jun 2023 16:52:59 +0200 you wrote:
> > The first patch adds an if statement in core to skip early when
> > the MAC address is not being changes.
> > The remaining patches remove such checks from Intel drivers
> > as they're redundant at this point.
> >
> > v3: removed "This patch ..." from first patch to simplify sentence.
> > v2: modified check in core to support addresses of any length,
> > removed redundant checks in i40e and ice
> >
> > [...]
> 
> Here is the summary with links:
>   - [net-next,v3,1/3] net: add check for current MAC address in dev_set_mac_address
>     https://git.kernel.org/netdev/net-next/c/ad72c4a06acc
>   - [net-next,v3,2/3] i40e: remove unnecessary check for old MAC == new MAC
>     https://git.kernel.org/netdev/net-next/c/c45a6d1a23c5
>   - [net-next,v3,3/3] ice: remove unnecessary check for old MAC == new MAC
>     https://git.kernel.org/netdev/net-next/c/96868cca7971

Ah, so next time I will respond to each revision with rev-by tags 😉

> 
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> 

Re: [PATCH net-next v3 0/3] optimize procedure of changing MAC address on interface

[Jakub Kicinski]

On Fri, 16 Jun 2023 10:02:12 +0000 Fijalkowski, Maciej wrote:
> > Here is the summary with links:
> >   - [net-next,v3,1/3] net: add check for current MAC address in dev_set_mac_address
> >     https://git.kernel.org/netdev/net-next/c/ad72c4a06acc
> >   - [net-next,v3,2/3] i40e: remove unnecessary check for old MAC == new MAC
> >     https://git.kernel.org/netdev/net-next/c/c45a6d1a23c5
> >   - [net-next,v3,3/3] ice: remove unnecessary check for old MAC == new MAC
> >     https://git.kernel.org/netdev/net-next/c/96868cca7971  
> 
> Ah, so next time I will respond to each revision with rev-by tags 😉

I may have done this slightly out of spite :P
You tell the man that he didn't keep your R-b tag, why not throw it in
again in one of your responses? It's bit of a PITA for me to add it
manually. One tag in reply to the cover letter is enough FWIW (at least
on netdev, I explained to BPF folks how to get the tag propagation
from cover letter to work but I'm not 100% sure they set it up for
themselves).