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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

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