Re: Maybe problem with ieee80211_tear_down_links return value.

[Ben Greear <greearb@candelatech.com>]

On 2/27/26 22:45, Rameshkumar Sundaram wrote:
> 
> 
> On 2/28/2026 6:28 AM, Ben Greear wrote:
>> While checking on some other problems, I ended up adding logging to the code path
>> below from net/mac80211/link.c.  This path is hit very often on my system, and if I understand
>> the code correctly, it should only hit in error cases where MLO links have duplicated
>> MAC addresses.
>>
>>      ret = ieee80211_check_dup_link_addrs(sdata);
>>      if (!ret) {
>>          /* for keys we will not be able to undo this */
>>          ieee80211_tear_down_links(sdata, to_free, rem);
>>
>> The ieee80211_check_dup_link_addrs method appears to return 0 when there are no duplicates,
>> and -EALREADY when there are duplicates.  So maybe the check above should be reversed to be:
>>
>> if (ret) {
>> ??
>>
> 
> 
> The ieee80211_check_dup_link_addrs() helper returns 0 when no duplicates are found and -EALREADY on duplicates, as you described.
> 
> However, in the caller the pattern:
> 
>          if (!ret) {
>                  /* for keys we will not be able to undo this */
>                  ieee80211_tear_down_links(sdata, to_free, rem);
> 
>                  ieee80211_set_vif_links_bitmaps(sdata, new_links, dormant_links);
> 
>                  /* tell the driver */
>                  if (sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
>                          ret = drv_change_vif_links(sdata->local, sdata,
>                                                     old_links & old_active,
>                                                     new_links & sdata->vif.active_links,
>                                                     old);
>                  if (!new_links)
>                          ieee80211_debugfs_recreate_netdev(sdata, false);
> 
>                  if (sdata->vif.type == NL80211_IFTYPE_AP)
>                          ieee80211_update_apvlan_links(sdata);
>          }
> 
> treats ret == 0 as the success/commit path (i.e., only proceed with tearing down removed links, updating link bitmaps, and notifying the driver after validating 
> the new link configuration)
> 
> 
> So I don’t think the condition should be reversed. If it were changed to if (ret), we’d end up committing the update only when duplicates are detected 
> (-EALREADY), which seems backwards given the current flow (and the later rollback handling when ret is non-zero).

Hello Ramesh,

Thanks for the response.  Yes, I was confused...I thought that branch of code was the error case,
but it is actually expected case.

I'm seeing some rare debugfs corruption/crash in that code branch and started thinking it was
error handling.

Thanks,
Ben


--Ben

> 
> -- 
> Ramesh
> 
> 

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com