Re: rtw88: rtw_{read,write}_rf locking questions

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hello Ping-Ke,

On Wed, Jul 14, 2021 at 3:48 AM Pkshih <pkshih@realtek.com> wrote:
>
>
> > -----Original Message-----
> > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com]
> > Sent: Wednesday, July 14, 2021 12:51 AM
> > To: Yan-Hsuan Chuang; Pkshih; Tzu-En Huang
> > Cc: linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou;
> > Jernej Skrabec
> > Subject: rtw88: rtw_{read,write}_rf locking questions
> >
> > Hello rtw88 maintainers and contributors,
> >
> > there is an ongoing effort where Jernej and I are working on adding
> > SDIO support to the rtw88 driver.
> > The hardware we use at the moment is RTL8822BS and RTL8822CS.
> > Work-in-progress code can be found in Jernej's repo (note: this may be
> > rebased): [0]
>
> Thanks for your nice work!
A quick update: we got scanning and authentication to work.

> > We are at a point where we can communicate with the SDIO card and
> > successfully upload the firmware to it.
> > Right now I have two questions about the locking in
> > rtw_{read,write}_rf from hci.h:
> > 1) A spinlock is used to protect RF register access. This is
> > problematic for SDIO, more information below. Would you accept a patch
> > to convert this into a mutex? I don't have any rtw88 PCIe card for
> > testing any regressions there myself.
>
> I think it's okay.
Great, thanks for confirming this!
I'll send a series of patches with locking preparations (patches which
add SDIO support will come later as we're still trying to narrow down
a few issues).

> > 2) I would like to understand why the RF register access needs to be
> > protected by a lock. From what I can tell RF register access doesn't
> > seem to be used from IRQ handlers.
>
> The use of lock isn't because we want to access the RF register in IRQ
> handlers. The reasons are
> 1. The ieee80211 iterative vif function we use is atomic type, so we can't
>    use mutex.
>    Do you change the type of iterative function?
yes, that is part of the "locking preparation" patches I mentioned above

> 2. RF register access isn't an atomic. If more than one threads access the
>    register at the same time, the value will be wrong.
Understood, thanks for pointing this out.


Best regards,
Martin