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