rtw88: rtw_{read,write}_rf locking questions

rtw88: rtw_{read,write}_rf locking questions

[Martin Blumenstingl]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

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

[-- Attachment #2: rtw88-sdio-scheduling-while-atomic.txt --]
[-- Type: text/plain, Size: 2384 bytes --]

[   27.249435] BUG: scheduling while atomic: ip/2749/0x00000002
[   27.249482] Modules linked in:
[   27.252485] CPU: 2 PID: 2749 Comm: ip Not tainted 5.14.0-rc1+ #30
[   27.258521] Hardware name: Shenzhen Amediatech Technology Co., Ltd X96 Air (DT)
[   27.265767] Call trace:
[   27.268181]  dump_backtrace+0x0/0x19c
[   27.271804]  show_stack+0x1c/0x30
[   27.275081]  dump_stack_lvl+0x68/0x84
[   27.278702]  dump_stack+0x1c/0x38
[   27.281980]  __schedule_bug+0x60/0x80
[   27.285602]  __schedule+0x600/0x6c0
[   27.289052]  schedule+0x74/0x110
[   27.292243]  schedule_timeout+0xc0/0xf0
[   27.296039]  wait_for_completion+0x80/0x120
[   27.300178]  mmc_wait_for_req_done+0x6c/0xa0
[   27.304404]  mmc_wait_for_req+0xb4/0x104
[   27.308286]  mmc_io_rw_extended+0x1cc/0x2c0
[   27.312426]  sdio_io_rw_ext_helper+0x194/0x240
[   27.316824]  sdio_memcpy_toio+0x28/0x34
[   27.320619]  rtw_sdio_write_block+0x88/0xf0
[   27.324760]  rtw_sdio_write+0x138/0x18c
[   27.328555]  rtw_sdio_write32+0x3c/0x90
[   27.332349]  rtw_phy_write_rf_reg_sipi+0x98/0xf4
[   27.336921]  rtw_phy_write_rf_reg_mix+0x18/0x40
[   27.341406]  rtw_phy_cfg_rf+0x74/0xd0
[   27.345028]  rtw_parse_tbl_phy_cond+0x130/0x180
[   27.349513]  rtw_phy_load_tables+0x178/0x1a0
[   27.353739]  rtw8822c_phy_set_param+0x114/0xe40
[   27.358224]  rtw_core_start+0xb0/0x240
[   27.361934]  rtw_ops_start+0x30/0x50
[   27.365469]  drv_start+0x38/0x60
[   27.368661]  ieee80211_do_open+0x188/0x7d0
[   27.372714]  ieee80211_open+0x64/0xb0
[   27.376337]  __dev_open+0x10c/0x1c0
[   27.379787]  __dev_change_flags+0x194/0x210
[   27.383927]  dev_change_flags+0x28/0x6c
[   27.387723]  do_setlink+0x204/0xd14
[   27.391173]  __rtnl_newlink+0x484/0x830
[   27.394967]  rtnl_newlink+0x50/0x80
[   27.398418]  rtnetlink_rcv_msg+0x120/0x334
[   27.402472]  netlink_rcv_skb+0x5c/0x130
[   27.406267]  rtnetlink_rcv+0x1c/0x2c
[   27.409803]  netlink_unicast+0x2bc/0x310
[   27.413683]  netlink_sendmsg+0x1a8/0x3d0
[   27.417565]  ____sys_sendmsg+0x250/0x294
[   27.421447]  ___sys_sendmsg+0x7c/0xc0
[   27.425068]  __sys_sendmsg+0x68/0xc4
[   27.428605]  __arm64_sys_sendmsg+0x28/0x3c
[   27.432659]  invoke_syscall+0x48/0x114
[   27.436367]  el0_svc_common+0xc4/0xdc
[   27.439991]  do_el0_svc+0x2c/0x94
[   27.443267]  el0_svc+0x2c/0x54
[   27.446286]  el0t_64_sync_handler+0xa8/0x12c
[   27.450513]  el0t_64_sync+0x198/0x19c

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

[Pkshih]

> -----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

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

[Martin Blumenstingl]

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