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]
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.
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.
Some background on why SDIO access (for example: sdio_writeb) cannot
be done with a spinlock held:
- when using for example sdio_writeb the MMC subsystem in Linux
prepares a so-called MMC request
- this request is submitted to the MMC host controller hardware
- the host controller hardware forwards the MMC request to the card
- the card signals when it's done processing the request
- the MMC subsystem in Linux waits for the card to signal that it's
done processing the request in mmc_wait_for_req_done() -> this uses
wait_for_completion() internally, which might sleep (which is not
allowed while a spinlock is held)
I am looking forward to your advice on this rtw_{read,write}_rf locking topic.
Thank you and best regards,
Martin
[0] https://github.com/jernejsk/linux-1/commits/rtw88-sdio
-----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!
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.
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?
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.
Some background on why SDIO access (for example: sdio_writeb) cannot
be done with a spinlock held:
- when using for example sdio_writeb the MMC subsystem in Linux
prepares a so-called MMC request
- this request is submitted to the MMC host controller hardware
- the host controller hardware forwards the MMC request to the card
- the card signals when it's done processing the request
- the MMC subsystem in Linux waits for the card to signal that it's
done processing the request in mmc_wait_for_req_done() -> this uses
wait_for_completion() internally, which might sleep (which is not
allowed while a spinlock is held)
I am looking forward to your advice on this rtw_{read,write}_rf locking topic.
[0] https://github.com/jernejsk/linux-1/commits/rtw88-sdio
Ping-Ke
Hello Ping-Ke,
On Wed, Jul 14, 2021 at 3:48 AM Pkshih [off-list ref] wrote:
quoted
-----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.
quoted
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).
quoted
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