RE: [PATCH rtw-next v1 11/13] wifi: rtw89: Add usb.{c,h}

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> On 13/05/2025 09:12, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >> Add very basic USB support. No TX/RX aggregation, no TX queues, no
> >> switching to USB 3 mode.
> >>
> >> RTL8851BU and RTL8832BU work.
> >>
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >> ---
> >>  drivers/net/wireless/realtek/rtw89/usb.c | 1030 ++++++++++++++++++++++
> >>  drivers/net/wireless/realtek/rtw89/usb.h |   61 ++
> >>  2 files changed, 1091 insertions(+)
> >>  create mode 100644 drivers/net/wireless/realtek/rtw89/usb.c
> >>  create mode 100644 drivers/net/wireless/realtek/rtw89/usb.h
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> >> new file mode 100644
> >> index 000000000000..6e8a544b352c
> >> --- /dev/null
> >> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> >> @@ -0,0 +1,1030 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> >> +/* Copyright(c) 2025  Realtek Corporation
> >> + */
> >> +
> >> +#include <linux/usb.h>
> >> +#include "debug.h"
> >> +#include "mac.h"
> >> +#include "reg.h"
> >> +#include "txrx.h"
> >> +#include "usb.h"
> >> +
> >> +static void rtw89_usb_vendorreq(struct rtw89_dev *rtwdev, u32 addr,
> >> +                               void *data, u16 len, u8 reqtype)
> >> +{
> >> +       struct rtw89_usb *rtwusb = rtw89_get_usb_priv(rtwdev);
> >> +       struct usb_device *udev = rtwusb->udev;
> >> +       unsigned int pipe;
> >> +       u16 value, index;
> >> +       int attempt, ret;
> >> +
> >> +       value = addr & 0x0000ffff;
> >> +       index = (addr & 0x00ff0000) >> 16;
> >
> > u16_get_bits(addr, GENMASK(23, 16)) ?
> >
> >
> >> +
> >> +       mutex_lock(&rtwusb->vendor_req_mutex);
> >
> > rtw89 takes wiphy_lock for control path. Is there any case more than
> > one threads run at the same time?
> >
> 
> Maybe not. I just copied this from the vendor driver. How can I be
> sure only one thread runs?

For rtw89, currently all ieee80211_ops take wiphy_lock except to TX related
ops. The works forked by rtw89 use wiphy works basically. Some works still 
use ieee80211 works only if they only set a simple flags or so. 

Here, I would like to avoid adding unnecessary mutex at development stage,
because it is hard to remove a mutex with simple review. You can see only 
one existing mutex is 'struct mutex rf_mutex'. I want to remove it, but
I'm still afraid that I miss something by review. 

> 
> I added this above the mutex_lock() today:
> 
>         if (mutex_is_locked(&rtwusb->vendor_req_mutex))
>                 pr_err("mutex already locked elsewhere\n");
> 
> So far it hasn't printed the message.

Not sure if this function depends on lock debugging of kernel options.

By the experiments, this mutex seems to be unnecessary, right?