A zero MAC address was considered a special case while creating a new
MAC filter. There is no particular reason for that other than the fact
that the union containing it was assumed to be zeroed out. Now, address
is pulled out of the union by ice_fltr_mac_address which checks all of
the previously assumed zero-address cases and returns an error if they
are hit.
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Jakub Slepecki <jakub.slepecki@intel.com>
---
No changes in v3.
No changes in v2.
---
drivers/net/ethernet/intel/ice/ice_switch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 0275e2910c6b..04e5d653efce 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -3648,7 +3648,7 @@ int ice_add_mac(struct ice_hw *hw, struct list_head *m_list)
u16 hw_vsi_id;
err = ice_fltr_mac_address(addr, &m_list_itr->fltr_info);
- if (err || is_zero_ether_addr(addr))
+ if (err)
return -EINVAL;
m_list_itr->fltr_info.flag = ICE_FLTR_TX;
vsi_handle = m_list_itr->fltr_info.vsi_handle;
--
2.43.0On 1/20/2026 2:34 AM, Jakub Slepecki wrote: > A zero MAC address was considered a special case while creating a new > MAC filter. There is no particular reason for that other than the fact > that the union containing it was assumed to be zeroed out. Now, address > is pulled out of the union by ice_fltr_mac_address which checks all of > the previously assumed zero-address cases and returns an error if they > are hit. > > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Signed-off-by: Jakub Slepecki <jakub.slepecki@intel.com> > > --- > No changes in v3. > No changes in v2. > --- > drivers/net/ethernet/intel/ice/ice_switch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c > index 0275e2910c6b..04e5d653efce 100644 > --- a/drivers/net/ethernet/intel/ice/ice_switch.c > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c > @@ -3648,7 +3648,7 @@ int ice_add_mac(struct ice_hw *hw, struct list_head *m_list) > u16 hw_vsi_id; > > err = ice_fltr_mac_address(addr, &m_list_itr->fltr_info); > - if (err || is_zero_ether_addr(addr)) This is introduced in the previous patch; it would be better to remove it in the original patch. Also, AI Review says: The is_zero_ether_addr(addr) check was removed in line 3651, relying on the claim that ice_fltr_mac_address() performs this validation. However, the helper function only extracts the MAC address and validates the lookup type—it does NOT validate against zero addresses. Thanks, Tony > + if (err) > return -EINVAL; > m_list_itr->fltr_info.flag = ICE_FLTR_TX; > vsi_handle = m_list_itr->fltr_info.vsi_handle;
On 2026-01-27 0:21, Tony Nguyen wrote:
>> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
>> index 0275e2910c6b..04e5d653efce 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
>> @@ -3648,7 +3648,7 @@ int ice_add_mac(struct ice_hw *hw, struct list_head *m_list)
>> u16 hw_vsi_id;
>> err = ice_fltr_mac_address(addr, &m_list_itr->fltr_info);
>> - if (err || is_zero_ether_addr(addr))
>
> This is introduced in the previous patch; it would be better to remove it in the original patch.
Previous patch moves it from
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 84848f0123e7..0275e2910c6b 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -3634,17 +3660,19 @@ int ice_add_mac(struct ice_hw *hw, struct list_head *m_list)
if (m_list_itr->fltr_info.src_id != ICE_SRC_ID_VSI)
return -EINVAL;
m_list_itr->fltr_info.src = hw_vsi_id;
- if (m_list_itr->fltr_info.lkup_type != ICE_SW_LKUP_MAC ||
- is_zero_ether_addr(add))
return -EINVAL;
...
here, as a call to is_zero_ether_addr(), to the chunk right above, in
@@ -3614,16 +3637,19 @@ bool ice_vlan_fltr_exist(struct ice_hw *hw, u16 vlan_id, u16 vsi_handle)
int ice_add_mac(struct ice_hw *hw, struct list_head *m_list)
{
struct ice_fltr_list_entry *m_list_itr;
- int status = 0;
+ int err;
if (!m_list || !hw)
return -EINVAL;
list_for_each_entry(m_list_itr, m_list, list_entry) {
- u8 *add = &m_list_itr->fltr_info.l_data.mac.mac_addr[0];
+ u8 addr[ETH_ALEN];
u16 vsi_handle;
u16 hw_vsi_id;
+ err = ice_fltr_mac_address(addr, &m_list_itr->fltr_info);
+ if (err || is_zero_ether_addr(addr))
+ return -EINVAL;
...
here, same call.
The intention of the previous patch is to allow adding mac,vlan filters.
Check is removed separately to make it visible. Alternative is hiding
it somewhere in two active chunks and in a long commit message. I think
this fits well into "separate each logical change into a separate patch."
> Also, AI Review says:
>
> The is_zero_ether_addr(addr) check was removed in line 3651, relying on the claim that ice_fltr_mac_address() performs this validation. However, the helper function only extracts the MAC address and validates the lookup type—it does NOT validate against zero addresses.
Removal is a result of internal discussion about ice_add_mac() and
ice_fltr_mac_address() using zero addresses to mark errors. Reading
through the comments now does not make me convinced it's the best way.
As long as errors are reported via int returns, I think the zero address
check can act as a sanity check. AFAIK, all calls that may result in
ice_add_mac() currently are guarded by is_valid_ether_addr().
As for the phrasing in the commit message. That's my mistake and if the
patch remains, I'll correct this. This version of this patch should
not say "previously assumed zero-address cases."
I'd prefer to remove this patch in v4.
> Thanks,
> Tony
Thank you!
On 1/27/2026 2:31 AM, Jakub Slepecki wrote:
> On 2026-01-27 0:21, Tony Nguyen wrote:
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/
>>> net/ethernet/intel/ice/ice_switch.c
>>> index 0275e2910c6b..04e5d653efce 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
>>> @@ -3648,7 +3648,7 @@ int ice_add_mac(struct ice_hw *hw, struct
>>> list_head *m_list)
>>> u16 hw_vsi_id;
>>> err = ice_fltr_mac_address(addr, &m_list_itr->fltr_info);
>>> - if (err || is_zero_ether_addr(addr))
>>
>> This is introduced in the previous patch; it would be better to remove
>> it in the original patch.
>
> Previous patch moves it from
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/
> ethernet/intel/ice/ice_switch.c
> index 84848f0123e7..0275e2910c6b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -3634,17 +3660,19 @@ int ice_add_mac(struct ice_hw *hw, struct
> list_head *m_list)
> if (m_list_itr->fltr_info.src_id != ICE_SRC_ID_VSI)
> return -EINVAL;
> m_list_itr->fltr_info.src = hw_vsi_id;
> - if (m_list_itr->fltr_info.lkup_type != ICE_SW_LKUP_MAC ||
> - is_zero_ether_addr(add))
> return -EINVAL;
> ...
>
> here, as a call to is_zero_ether_addr(), to the chunk right above, in
>
> @@ -3614,16 +3637,19 @@ bool ice_vlan_fltr_exist(struct ice_hw *hw, u16
> vlan_id, u16 vsi_handle)
> int ice_add_mac(struct ice_hw *hw, struct list_head *m_list)
> {
> struct ice_fltr_list_entry *m_list_itr;
> - int status = 0;
> + int err;
>
> if (!m_list || !hw)
> return -EINVAL;
>
> list_for_each_entry(m_list_itr, m_list, list_entry) {
> - u8 *add = &m_list_itr->fltr_info.l_data.mac.mac_addr[0];
> + u8 addr[ETH_ALEN];
> u16 vsi_handle;
> u16 hw_vsi_id;
>
> + err = ice_fltr_mac_address(addr, &m_list_itr->fltr_info);
> + if (err || is_zero_ether_addr(addr))
> + return -EINVAL;
> ...
>
> here, same call.
I see now, I mixed up the hunks/functions. I'm ok with this being by itself.
>
> The intention of the previous patch is to allow adding mac,vlan filters.
> Check is removed separately to make it visible. Alternative is hiding
> it somewhere in two active chunks and in a long commit message. I think
> this fits well into "separate each logical change into a separate patch."
>
>> Also, AI Review says:
>>
>> The is_zero_ether_addr(addr) check was removed in line 3651, relying
>> on the claim that ice_fltr_mac_address() performs this validation.
>> However, the helper function only extracts the MAC address and
>> validates the lookup type—it does NOT validate against zero addresses.
>
> Removal is a result of internal discussion about ice_add_mac() and
> ice_fltr_mac_address() using zero addresses to mark errors. Reading
> through the comments now does not make me convinced it's the best way.
> As long as errors are reported via int returns, I think the zero address
> check can act as a sanity check. AFAIK, all calls that may result in
> ice_add_mac() currently are guarded by is_valid_ether_addr().
>
> As for the phrasing in the commit message. That's my mistake and if the
> patch remains, I'll correct this. This version of this patch should
> not say "previously assumed zero-address cases."
>
> I'd prefer to remove this patch in v4.
Sounds good.
Thanks,
Tony
© 2016 - 2026 Red Hat, Inc.