Received-SPF: pass (google.com: domain of linux-wireless+bounces-4748-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <f734bd50-8a04-447b-b1e8-11eb6c958f34@gmail.com>
Date: Thu, 14 Mar 2024 15:49:44 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Subject: Re: [PATCH 2/3] wifi: rtlwifi: Adjust rtl8192d-common for USB
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
 "s.l-h@gmx.de" <s.l-h@gmx.de>, "chewitt@libreelec.tv" <chewitt@libreelec.tv>
References: <e1922019-21eb-4013-a35f-0077167e92cf@gmail.com>
 <d70dc39c-a3e5-43f6-9cb4-612d08035c57@gmail.com>
 <fdd95844c3a127356bab4250dfc46b16041829ee.camel@realtek.com>
 <a674d357-c917-413a-a4fb-1ee22f43d9ff@gmail.com>
 <f07686170eed48c21f18ce2850ffde4401c25bf5.camel@realtek.com>
Content-Language: en-US
In-Reply-To: <f07686170eed48c21f18ce2850ffde4401c25bf5.camel@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 14/03/2024 03:20, Ping-Ke Shih wrote:
> On Thu, 2024-03-14 at 00:47 +0200, Bitterblue Smith wrote:
>>
>> On 13/03/2024 05:46, Ping-Ke Shih wrote:
>>> On Wed, 2024-03-13 at 00:20 +0200, Bitterblue Smith wrote:
>>>>
>>>> @@ -966,12 +980,17 @@ static void rtl92de_update_hal_rate_mask(struct ieee80211_hw *hw,
>>>>                 break;
>>>>         }
>>>>
>>>> -       value[0] = (ratr_bitmap & 0x0fffffff) | (ratr_index << 28);
>>>> -       value[1] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
>>>> +       *(u32 *)&rate_mask = (ratr_bitmap & 0x0fffffff) |
>>>> +                                    (ratr_index << 28);
>>>
>>> 'u32' is weird to me. Shouldn't it be __le32?
>>> But I prefer a struct of rate_mask.
>>>
>>
>> I don't like this either, but it was easy to copy from rtl8192cu.
>>
>> Something like this?
>>
>> #define RAID_MASK               GENMASK(31, 28)
>> #define RATE_MASK_MASK          GENMASK(27, 0)
>> #define SHORT_GI_MASK           BIT(5)
>> #define MACID_MASK              GENMASK(4, 0)
>>
>> struct rtl92d_rate_mask {
>>         __le32 rate_mask_and_raid;
>>         u8 macid_and_short_gi;
>> } __packed;
> 
> Yes, something like that, but struct size should be 5.
> 
>>>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c
>>>> b/drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c
>>>> index 487628ac491b..1e39940a3ba7 100644
>>>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c
>>>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.c
>>>> @@ -81,11 +81,13 @@ u32 rtl92d_phy_query_rf_reg(struct ieee80211_hw *hw, enum radio_path
>>>> rfpath,
>>>>         rtl_dbg(rtlpriv, COMP_RF, DBG_TRACE,
>>>>                 "regaddr(%#x), rfpath(%#x), bitmask(%#x)\n",
>>>>                 regaddr, rfpath, bitmask);
>>>> -       spin_lock(&rtlpriv->locks.rf_lock);
>>>> +       if (rtlpriv->rtlhal.interface == INTF_PCI)
>>>> +               spin_lock(&rtlpriv->locks.rf_lock);
>>>
>>> Does it mean USB never read/write RF registers simultaneously? How can you
>>> ensure that?
>>>
>>
>> I don't know. It seems to work fine. The out-of-tree driver
>> doesn't have locks here:
>> https://github.com/lwfinger/rtl8192du/blob/2c5450dd3783e1085f09a8c7a632318c7d0f1d39/hal/rtl8192d_phycfg.c#L637
>>
>> rtl8xxxu and rtl8192cu don't have locks either.
> 
> Not sure if race condition is existing to read/write RF registers, but
> no idea to dig the problem. Maybe, current code has no problem though.
> At least, your newly changes don't affect original PCI behavior, right?
> 

Yes, the PCI driver should behave like before.

> 
>>>> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c
>>>> b/drivers/net/wireless/realtek/rtlwifi/usb.c
>>>> index 6e8c87a2fae4..2ea72d9e3957 100644
>>>> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
>>>> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
>>>> @@ -979,6 +979,9 @@ int rtl_usb_probe(struct usb_interface *intf,
>>>>         usb_priv->dev.intf = intf;
>>>>         usb_priv->dev.udev = udev;
>>>>         usb_set_intfdata(intf, hw);
>>>> +       /* For dual MAC RTL8192DU, which has two interfaces. */
>>>> +       rtlpriv->rtlhal.interfaceindex =
>>>> +               intf->altsetting[0].desc.bInterfaceNumber;
>>>
>>> So, you will see two USB adapters when you plug 8192DU?
>>>
>>
>> When you plug the dual MAC version, lsusb will show one device,
>> with two interfaces. rtl_usb_probe() is called twice. This is
>> copied from linux-hardware.org:
>>
>> Mine is the single MAC version:
>>
> 
> Does it mean you only tested single MAC version? But, your code will support
> two MAC version, right?
> 
> 

Theoretically all the code for dual MAC is there. But I only
tested the single MAC version, and Stefan also has the single
MAC version.