Received-SPF: pass (google.com: domain of linux-wireless+bounces-5230-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <7b30b488-b513-e3ab-cdbf-75449d0dcdda@candelatech.com>
Date: Mon, 25 Mar 2024 10:53:50 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] wifi: mac80211: Ensure links are cleaned up when driver
 fails.
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>,
 linux-wireless <linux-wireless@vger.kernel.org>
References: <20231111001023.1335122-1-greearb@candelatech.com>
 <5fe480c4-fa39-1d29-a5e9-ba2447b08dd2@candelatech.com>
 <449f6afbc20c57f704dbeabc3a8eac8fec676405.camel@sipsolutions.net>
 <6bed94b3-4674-7fe9-ee82-bac7f9c7ffd6@candelatech.com>
 <973efc2cb87812f512fd33f354a91da17e3d8564.camel@sipsolutions.net>
From: Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <973efc2cb87812f512fd33f354a91da17e3d8564.camel@sipsolutions.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 3/25/24 10:43, Johannes Berg wrote:
> On Mon, 2024-03-25 at 10:40 -0700, Ben Greear wrote:
>> It is needed because if FW crashes while you are trying to remove links, then link
>> removal would fail, which causes mac80211 to not clean up its links.
> 
> OK. It should get back to some legal state after recovery though?

I assume mac80211 will re-add links on re-association/recovery.

> 
>> In case where you
>> are trying to delete the station, this causes un-cleaned links which caused crashes
>> back when I was creating this patch.  My patch allows always cleaning up the links
>> regardless of driver errors in the teardown paths.
> 
> Seems potentially more like driver errors. Or perhaps we should just
> ignore driver errors entirely?

I guess you need errors reported when building links, but maybe removal always
has to succeed.  Last I looked at iwlwifi, it returned errors on link
removal when FW crashed as it was trying to remove them.

>> Always possible some intervening changes made this less of a problem, especially since
>> MLO is disabled for be200 in upstream code anyway now.
> 
> Well that's just temporary, but we've also done a lot of work on FW
> error recovery, though likely unrelated to this particular issue.
> 
>> I can remove the extra logging if you are otherwise OK with the approach
>> but don't want the logs.
> 
> Well it'd be nice to actually see what crashed, and maybe it should
> really be driver fixes? I don't really understand why mac80211 would
> crash on failure of link removal.

6.6-ish with old be200 firmware + MLO was a bad experience.  There were multiple crashes all
over the place.  This was one of them, but I don't recall exactly how it crashed, just
something about un-deleted (or re-built) links when rest of the code expected them to be deleted since
STA was going away.

This below is the real change in the patch.  It would keep us from re-building links
in teardown paths.  Rest of the patch is passing in the knowledge of whether we can ignore
the failure or not.

@@ -325,13 +328,17 @@ static int ieee80211_vif_update_links(struct ieee80211_sub_if_data *sdata,
       }
         if (ret) {
-        /* restore config */
-        memcpy(sdata->link, old_data, sizeof(old_data));
-        memcpy(sdata->vif.link_conf, old, sizeof(old));
-        ieee80211_set_vif_links_bitmaps(sdata, old_links, dormant_links);
-        /* and free (only) the newly allocated links */
-        memset(to_free, 0, sizeof(links));
-        goto free;
+        sdata_info(sdata, "driver error applying links: %d  Restoring old configuration, old_links: 0x%x  dormant_links: 0x%x requested new_links: 0x%x 
ignore-driver-failures: %d\n",
+               ret, old_links, dormant_links, new_links, ignore_driver_failures);
+        if (!ignore_driver_failures) {
+            /* restore config */
+            memcpy(sdata->link, old_data, sizeof(old_data));
+            memcpy(sdata->vif.link_conf, old, sizeof(old));
+            ieee80211_set_vif_links_bitmaps(sdata, old_links, dormant_links);
+            /* and free (only) the newly allocated links */
+            memset(to_free, 0, sizeof(links));
+            goto free;
+        }
       }

Thanks,
Ben

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