Message-ID: <18728c67cffad35f827d9a32af34044592254093.camel@sipsolutions.net>
Subject: Re: [PATCH] iwlwifi: mvm: fix double list_add at
 iwl_mvm_mac_wake_tx_queue (roaming)
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Cc:     gregory.greenman@intel.com, kvalo@kernel.org,
        linux-wireless@vger.kernel.org, pstourac@redhat.com
Date:   Tue, 14 Mar 2023 15:56:59 +0100
In-Reply-To: <20230314145209.401875-1-jtornosm@redhat.com>
References: <20230314103840.30771-1-jtornosm@redhat.com>
         <20230314145209.401875-1-jtornosm@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-03-14 at 15:52 +0100, Jose Ignacio Tornos Martinez wrote:
>=20
> We have some Bugzilla (i.e.=20
> https://bugzilla.redhat.com/show_bug.cgi?id=3D2152168).
> You only have to create a user for this tool.

Looks like it's not public, and anyway I was mostly asking for a commit
log record, so that's not useful even if I could get access:

"You are not authorized to access bug #2152168."

> > > This can be reproduced with a single script from the station:
> > >     while true; do
> > >         wpa_cli -i wlp3s0 roam 34:13:E8:B1:DB:9A
> > >         sleep 2
> > >         wpa_cli -i wlp3s0 roam 34:13:E8:3C:FB:DB
> > >         sleep 2
> > >     done
> > > And flooding with tx traffic.
> > Oh, nice to have a reproducer.
> It is not immediate but I can reproduce here like this.

Right.

> > Funny thing is, I was _just_ looking at this exact bug, because we were
> > discussing all this concurrency over in
> So more people is struggling this this, good to get the best solution.

Well that was a different issue in different drivers, but then we looked
at it ...

> > While this might fix the issue as far as you could observe, that is
> > clearly not sufficient, since you don't protect the list on the other
> > side, where the items are removed from it again.

> Ok, I thought about that as well but I was not able to find any problem w=
ith
> the other side. Anyway, the better the solution is made the better.

Well it doesn't _look_ like it accesses the list, but the
list_del_init(&mvmtxq->list) on the other side will access the list too.

> > Below are the two patches that I've come up with so far, if anyone want=
s
> > to try them. Please ignore all the extra metadata, I exported this
> > directly from our internal code base.
> Of course, I can test the soutions here in order to be sure.
> Do you prefer I reply with the result here or in the other thread that yo=
u have
> commented me before?

Here is fine, thanks!

johannes