On 8/12/2025 9:55 PM, Paul Menzel wrote:
Dear Emil,
Thank you for the patch.
Am 13.08.25 um 04:42 schrieb Emil Tantilov:
On control planes that allow changing the MAC address of the interface,
the driver must provide a MAC type to avoid errors such as:
idpf 0000:0a:00.0: Transaction failed (op 535)
idpf 0000:0a:00.0: Received invalid MAC filter payload (op 535) (len 0)
idpf 0000:0a:00.0: Transaction failed (op 536)
These errors occur during driver load or when changing the MAC via:
ip link set <iface> address <mac>
Add logic to set the MAC type when sending ADD/DEL (opcodes 535/536) to
the control plane. Since only one primary MAC is supported per vport, the
driver only needs to send an ADD opcode when setting it. Remove the old
address by calling __idpf_del_mac_filter(), which skips the message and
just clears the entry from the internal list.
Could this be split into two patches?
1. Set the type
2. Improve logic
Both changes fix the errors. In the second change DEL/536 opcode
following ADD/535 will also result in "Transaction failed (op 536)" as
it attempt to remove an address which was already cleared by the
preceding ADD/535 opcode. I will update the description to make it clear
the change is more than improvement.
Fixes: ce1b75d0635c ("idpf: add ptypes and MAC filter support")
Reported-by: Jian Liu <[email protected]>
Signed-off-by: Emil Tantilov <[email protected]>
---
Changelog:
v2:
- Make sure to clear the primary MAC from the internal list, following
successful change.
- Update the description to include the error on 536 opcode and
mention the removal of the old address.
v1:
https://lore.kernel.org/intel-wired-lan/20250806192130.3197-1-
[email protected]/
---
drivers/net/ethernet/intel/idpf/idpf_lib.c | 9 ++++++---
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 11 +++++++++++
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/
ethernet/intel/idpf/idpf_lib.c
index 2c2a3e85d693..26edd2cda70b 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -2345,6 +2345,7 @@ static int idpf_set_mac(struct net_device
*netdev, void *p)
struct idpf_vport_config *vport_config;
struct sockaddr *addr = p;
struct idpf_vport *vport;
+ u8 old_addr[ETH_ALEN];
old_mac_addr?
Sure.
int err = 0;
idpf_vport_ctrl_lock(netdev);
@@ -2367,17 +2368,19 @@ static int idpf_set_mac(struct net_device
*netdev, void *p)
if (ether_addr_equal(netdev->dev_addr, addr->sa_data))
goto unlock_mutex;
+ ether_addr_copy(old_addr, vport->default_mac_addr);
+ ether_addr_copy(vport->default_mac_addr, addr->sa_data);
vport_config = vport->adapter->vport_config[vport->idx];
err = idpf_add_mac_filter(vport, np, addr->sa_data, false);
if (err) {
__idpf_del_mac_filter(vport_config, addr->sa_data);
+ ether_addr_copy(vport->default_mac_addr, netdev->dev_addr);
goto unlock_mutex;
}
- if (is_valid_ether_addr(vport->default_mac_addr))
- idpf_del_mac_filter(vport, np, vport->default_mac_addr, false);
+ if (is_valid_ether_addr(old_addr))
+ __idpf_del_mac_filter(vport_config, old_addr);
- ether_addr_copy(vport->default_mac_addr, addr->sa_data);
eth_hw_addr_set(netdev, addr->sa_data);
unlock_mutex:
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index a028c69f7fdc..e60438633cc4 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -3765,6 +3765,15 @@ u32 idpf_get_vport_id(struct idpf_vport *vport)
return le32_to_cpu(vport_msg->vport_id);
}
+static void idpf_set_mac_type(struct idpf_vport *vport,
+ struct virtchnl2_mac_addr *mac_addr)
+{
+ if (ether_addr_equal(vport->default_mac_addr, mac_addr->addr))
+ mac_addr->type = VIRTCHNL2_MAC_ADDR_PRIMARY;
+ else
+ mac_addr->type = VIRTCHNL2_MAC_ADDR_EXTRA;
I’d use the ternary operator. That way, it’s clear the same variable is
assigned a value in each branch.
The assignment is fairly isolated in just this function, but if this is
the preferred style I will change it in v3 along with the old_addr
rename.
+}
+
/**
* idpf_mac_filter_async_handler - Async callback for mac filters
* @adapter: private data struct
@@ -3894,6 +3903,7 @@ int idpf_add_del_mac_filters(struct idpf_vport
*vport,
list) {
if (add && f->add) {
ether_addr_copy(mac_addr[i].addr, f->macaddr);
+ idpf_set_mac_type(vport, &mac_addr[i]);
i++;
f->add = false;
if (i == total_filters)
@@ -3901,6 +3911,7 @@ int idpf_add_del_mac_filters(struct idpf_vport
*vport,
}
if (!add && f->remove) {
ether_addr_copy(mac_addr[i].addr, f->macaddr);
+ idpf_set_mac_type(vport, &mac_addr[i]);
i++;
f->remove = false;
if (i == total_filters)
The overall diff looks good. Feel free to add:
Reviewed-by: Paul Menzel <[email protected]>
Thank you,
Emil
Kind regards,
Paul
