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

[Pkshih]

> -----Original Message-----
> From: Martin Blumenstingl [mailto:martin.blumenstingl@xxxxxxxxxxxxxx]
> Sent: Wednesday, July 14, 2021 12:51 AM
> To: Yan-Hsuan Chuang; Pkshih; Tzu-En Huang
> Cc: linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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