Re: [PATCH v3 11/12] wifi: rtlwifi: Add rtl8192du/sw.{c,h}

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

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