Received-SPF: pass (google.com: domain of linux-wireless+bounces-5457-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7873dee1-46a8-48a7-9059-bc300ac98c23@gmail.com>
Date: Thu, 28 Mar 2024 15:31:14 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 11/12] wifi: rtlwifi: Add rtl8192du/sw.{c,h}
To: Ping-Ke Shih <pkshih@realtek.com>, "kvalo@kernel.org" <kvalo@kernel.org>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
 "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: <7f4b3309-1580-48f3-9426-29f1eb4052fd@gmail.com>
 <2eb79c8c-cf2c-4696-b958-e8d961628e17@gmail.com>
 <f86a40493745a53ff73083f87b3e8fae215eac77.camel@realtek.com>
 <66565618-3638-47e5-afe5-3530214da0c9@gmail.com> <87ttkrzf1m.fsf@kernel.org>
 <5696990a-4450-4d92-bbda-1d9ca3a9a619@gmail.com>
 <4672072ef782ae1200886a190c740e0f3ccec2c8.camel@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <4672072ef782ae1200886a190c740e0f3ccec2c8.camel@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 28/03/2024 03:46, Ping-Ke Shih wrote:
> On Thu, 2024-03-28 at 00:53 +0200, Bitterblue Smith wrote:
>>
>> On 27/03/2024 20:48, Kalle Valo wrote:
>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> writes:
>>>
>>>> On 22/03/2024 08:04, Ping-Ke Shih wrote:
>>>>> On Wed, 2024-03-20 at 21:43 +0200, Bitterblue Smith wrote:
>>>>
>>>> [...]
>>>>
>>>>>> +DEFINE_MUTEX(globalmutex_power);
>>>>>> +DEFINE_MUTEX(globalmutex_for_fwdownload);
>>>>>> +DEFINE_MUTEX(globalmutex_for_power_and_efuse);
>>>>>> +DEFINE_MUTEX(globalmutex_for_mac0_2g_mac1_5g);
>>>>>
>>>>> The consumers of globalmutex_for_mac0_2g_mac1_5g are complex. Why do they
>>>>> check mutex_is_locked()? Race conditions between two instances?
>>>>>
>>>>
>>>> I couldn't think of a sufficiently short name, like
>>>> "lock_mac0_2g_mac1_5g", so I used mutex_is_locked(). That's probably
>>>> a bad idea. It should be like this:
>>>>
>>>>      /* Let the first starting mac load RF parameters and do LCK */
>>>>      if (rtlhal->macphymode == DUALMAC_DUALPHY &&
>>>>          ((rtlhal->interfaceindex == 0 && rtlhal->bandset == BAND_ON_2_4G) ||
>>>>           (rtlhal->interfaceindex == 1 && rtlhal->bandset == BAND_ON_5G))) {
>>>>              mutex_lock(&globalmutex_for_mac0_2g_mac1_5g);
>>>>              lock_mac0_2g_mac1_5g = true;
>>>>      }
> 
> globalmutex_for_mac0_2g_mac1_5g is only used in rtl92du_hw_init(), and
> globalmutex_for_power_and_efuse does very similar thing. Can we combine them
> into one? Since both are only used in rtl92du_hw_init(), it would not be a
> problem to enlarge their critical section.  
> 
> 
>>
>> * Dual MAC dual PHY: this I can't find to buy anymore. This
>> appears in the system as two Wifi devices, each working on
>> a different band. It has two USB interfaces. Two instances
>> of the driver access the same device. This is what the
>> mutexes are for.
> 
> I traced the code, and found rules of two MAC/PHY are:
> 1. read efuse to decide single or two MAC/PHY
> 2.1. if single MAC/PHY, register_hw as 2T2R (done)
> 2.2. if dual MAC/PHY (to step 3)
> 3. read interface index (USB: from bInterfaceNumber; PCI: from PCI onfigure space)
> 4. register interface index 0 as 1T1R on 5 GHz band only.
>    register interface index 1 as 1T1R on 2 GHz band only.
> 
> This is the case two instances (netdev) access single one hardware device,
> so seemingly it is hard to avoid global locks to prevent racing between them.
> An alternative thought is to support only single MAC/PHY, but not sure if
> driver can override setting of efuse that programmed the card as two MAC/PHY.
> 
>>
>> I said earlier that I think two devices can work at the same
>> time, even with the global mutexes, but now I remembered there
>> are two more global variables: curveindex_5g[] and
>> curveindex_2g[] in phy.c. One driver instance fills one array
>> during LC calibration, but the other driver instance reads
>> from the array when switching the channel. If I'm reading this
>> right. So two devices plugged in at the same time probably
>> won't work correctly.
> 
> That should be a problem. 
> 
>>
>> How can you avoid this when the hardware is the way it is?
>> My one idea is to add a global map to hold the mutexes and
>> arrays, using the USB port number, etc as the key.
> 
> Seemingly we need something like per device data. 
> 

I got another idea: if we have a guarantee that the two USB
interfaces are probed one at a time, then we can move the global
things into struct rtl_priv. The first probe call will allocate
the arrays and initialise the mutexes. The second probe call
will obtain those from the first struct rtl_priv:

int rtl_usb_probe(struct usb_interface *intf, ...) {
	udev = interface_to_usbdev(intf);
	struct ieee80211_hw *first_hw = usb_get_intfdata(udev->actconfig->interface[0]);
	struct rtl_priv *first_rtlpriv = rtl_priv(first_hw);

Something like that.

I am having regrets...