Re: [PATCH iwl-next v3 3/8] ice: do not check for zero mac when creating mac filters

Tony Nguyen
2026-01-27T18:45:52Z

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