From nobody Fri Apr 22 10:53:51 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 10F8A1989DCF; Fri, 22 Apr 2022 10:53:58 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4KlB9K6gc6z580q; Fri, 22 Apr 2022 10:53:57 +0000 (UTC) (envelope-from bz@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1650624838; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=sZgKF7WAQopGRT+B1W9LJL59LwINjN1OQdpA4bj6nN4=; b=LkKcTeiuOeE6rmRJf23/eX6IfedoSmRtfsAfl0fFZnkePc/Ofs7eO61/tm+2WrlhqFBIwu 97/uMoJ69Pf1rDEl1KPdTAFxLc/NQnEJttATPLbAM+z02iaavmzTpzsknzndCclLfEazY4 +dunrj+ndrLT1A1LRqRdoV05RBJAtWYpC7Di/4JpK7QoMUA9jfpIRryd/y9dQpbid/25Fh vBPdWt1a934CTOfHuCdd5T7E3qEeC2XN5yvemg/kFdQa3xS2XnSooRmggRcWFMCAi9M/1e wYPZZtuqdKEKjEQIBbcYwoXTdhVbyNBlcG9VZHBNwfZCd0r4/iY679ouMPFkcQ== Received: from mx1.sbone.de (cross.sbone.de [195.201.62.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.sbone.de", Issuer "SBone.DE" (not verified)) (Authenticated sender: bz/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id AC1E47081; Fri, 22 Apr 2022 10:53:57 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from mail.sbone.de (mail.sbone.de [IPv6:fde9:577b:c1a9:31::2013:587]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.sbone.de (Postfix) with ESMTPS id BBAD58D4A15D; Fri, 22 Apr 2022 10:53:55 +0000 (UTC) Received: from content-filter.sbone.de (content-filter.sbone.de [IPv6:fde9:577b:c1a9:31::2013:2742]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPS id 3B7ECE707C4; Fri, 22 Apr 2022 10:53:55 +0000 (UTC) X-Virus-Scanned: amavisd-new at sbone.de Received: from mail.sbone.de ([IPv6:fde9:577b:c1a9:31::2013:587]) by content-filter.sbone.de (content-filter.sbone.de [fde9:577b:c1a9:31::2013:2742]) (amavisd-new, port 10024) with ESMTP id bX7Wym0sg8_k; Fri, 22 Apr 2022 10:53:52 +0000 (UTC) Received: from nv.sbone.de (nv.sbone.de [IPv6:fde9:577b:c1a9:31::2013:138]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPSA id 7CA09E707C1; Fri, 22 Apr 2022 10:53:52 +0000 (UTC) Date: Fri, 22 Apr 2022 10:53:51 +0000 (UTC) From: "Bjoern A. Zeeb" To: Adrian Chadd cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: e8de31caceaa - main - net80211: Fix traffic hang on STA/AP VAPs on a multi-VAP interface In-Reply-To: <202204220549.23M5nEhP099682@gitrepo.freebsd.org> Message-ID: References: <202204220549.23M5nEhP099682@gitrepo.freebsd.org> X-OpenPGP-Key-Id: 0x14003F198FEFA3E77207EE8D2B58B8F83CCF1842 List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1650624838; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=sZgKF7WAQopGRT+B1W9LJL59LwINjN1OQdpA4bj6nN4=; b=ftjL1AsTdFjMHN1m7meL1DX65QOHD88SrF2It9ybQ6QUn2RtOFwuxTXYw1+9Tu5mPvI37w vk2Da7aVnvTIcNck3GxMDZNoJ6YoBJW0dRCMZaiuHJZGRZbQ+ZVprIIaI2SgRFS/KNRvwz dgQ4hZ9g0gmWiXe/NDxOQODsOdVRijmIk+RiadG72jsgC/ydZnpwg1wLkPEEm/zynI95LW vkg9L78KW9cT2L4YRJS8GNALQ9wp7DNmN/O/j+5+3vsRfEzdQEj4sysVry9TwOzoLae02t CyXxG1+d98PTqF4mRmM+Be4D41Fkf5zVth1uaE6WOpWTIPaxZ2jXVbRmoaUinQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1650624838; a=rsa-sha256; cv=none; b=cJVoSUVJ6elWeFiEmFj0kj4tgpP5YS0ObxZH++j/P/RSBL4/tIiergOhGS2orBpYY0cPGH e6qs01MlZedbefQ9AJUnbXUe2s46G0O36cZnsvUex3svRr9jPcN/oOaBBf7mxnDuGi2U+D kdpX0B5eizDHgczIogedEDeDnPav+NFK6OBqKuxIh7r+Ger4nJdv7kC7zi8VgpBALb+KtH jFeQUdOomkQtYMlNgIyfQoOGN7/jwAMLRBR8pcaCzOV6tMqTLEyouGrkky0QCOC+0Pj2zl W5FIzF44UZfuAMIwcKz5DHwDparfvgaLTYAvfWzlcKUGqDtwOrNtI7l8fL6muQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N On Fri, 22 Apr 2022, Adrian Chadd wrote: > The branch main has been updated by adrian: > > URL: https://cgit.FreeBSD.org/src/commit/?id=e8de31caceaa36caf5d7b4355072f148e2433b82 > > commit e8de31caceaa36caf5d7b4355072f148e2433b82 > Author: Adrian Chadd > AuthorDate: 2022-04-12 20:20:28 +0000 > Commit: Adrian Chadd > CommitDate: 2022-04-22 05:49:01 +0000 > > net80211: Fix traffic hang on STA/AP VAPs on a multi-VAP interface > > This took an embarrasingly long time to find. > > The state changes for a radio with a STA /and/ AP VAP gets a bit messy. > The AP maps are marked as waiting, waiting for the STA AP to find a > channel to use before the AP VAPs become active. > > However, the code path that clears the OACTIVE flag on a VAP only runs > during a successful run of ieee80211_newstate_cb(). > > So here is how it goes: > > * the STA VAP goes down and needs to scan; > * the AP vap goes RUN->INIT; but it doesn't YET call ieee80211_newstate_cb(); > * meanwhile - a send on the AP VAP causes the VAP to set the OACTIVE flag here; > * then the STA VAP finishes scan and goes to RUN; > * which will call wakeupwaiting() as part of the STA VAP transition to RUN; > * .. then the AP VAP goes INIT->RUN directly via a call to hostap_newstate > in wakeupwaiting rather than it being through the deferred path; > * /then/ the ieee80211_newstate_cb() is called, but it sees the state go > RUN->RUN; > * .. which results in the OACTIVE flag never being cleared. > > This clears the OACTIVE flag when a VAP transitions RUN->RUN; the > driver layer or net80211 layer can set it if required in a subsequent > transmit. > > Differential Revision: https://reviews.freebsd.org/D34920 > > Reviewed by: bz I had not reviewed any final version of this given I was on the road and hadn't checked anything beyond my direct inbox until minutes ago. And I still see three large blobs of text in three places only with minor changes of words and the same one and only line of work of this change done twice and not removed the 2nd time, so clearly review comments weren't fully addressed. > --- > sys/net80211/ieee80211_proto.c | 47 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/sys/net80211/ieee80211_proto.c b/sys/net80211/ieee80211_proto.c > index 2228983050a2..d2bde99ce79c 100644 > --- a/sys/net80211/ieee80211_proto.c > +++ b/sys/net80211/ieee80211_proto.c > @@ -2469,6 +2469,29 @@ wakeupwaiting(struct ieee80211vap *vap0) > vap->iv_flags_ext &= ~IEEE80211_FEXT_SCANWAIT; > /* NB: sta's cannot go INIT->RUN */ > /* NB: iv_newstate may drop the lock */ > + > + /* > + * This is problematic if the interface has OACTIVE > + * set. Only the deferred ieee80211_newstate_cb() > + * will end up actually /clearing/ the OACTIVE > + * flag on a state transition to RUN from a non-RUN > + * state. > + * > + * But, we're not actually deferring this callback; > + * and when the deferred call occurs it shows up as > + * a RUN->RUN transition! So the flag isn't/wasn't > + * cleared! > + * > + * I'm also not sure if it's correct to actually > + * do the transitions here fully through the deferred > + * paths either as other things can be invoked as > + * part of that state machine. > + * > + * So just keep this in mind when looking at what > + * the markwaiting/wakeupwaiting routines are doing > + * and how they invoke vap state changes. > + */ > + > vap->iv_newstate(vap, > vap->iv_opmode == IEEE80211_M_STA ? > IEEE80211_S_SCAN : IEEE80211_S_RUN, 0); > @@ -2543,6 +2566,30 @@ ieee80211_newstate_cb(void *xvap, int npending) > goto done; > } > > + /* > + * Handle the case of a RUN->RUN transition occuring when STA + AP > + * VAPs occur on the same radio. > + * > + * The mark and wakeup waiting routines call iv_newstate() directly, > + * but they do not end up deferring state changes here. > + * Thus, although the VAP newstate method sees a transition > + * of RUN->INIT->RUN, the deferred path here only sees a RUN->RUN > + * transition. If OACTIVE is set then it is never cleared. > + * > + * So, if we're here and the state is RUN, just clear OACTIVE. > + * At some point if the markwaiting/wakeupwaiting paths end up > + * also invoking the deferred state updates then this will > + * be no-op code - and also if OACTIVE is finally retired, it'll > + * also be no-op code. > + */ > + if (nstate == IEEE80211_S_RUN) { > + /* > + * Unblock the VAP queue; a RUN->RUN state can happen > + * on a STA+AP setup on the AP vap. See wakeupwaiting(). > + */ > + vap->iv_ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; > + } > + > /* No actual transition, skip post processing */ > if (ostate == nstate) > goto done; > -- Bjoern A. Zeeb r15:7