Received-SPF: pass (google.com: domain of linux-wireless+bounces-5415-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <5696990a-4450-4d92-bbda-1d9ca3a9a619@gmail.com>
Date: Thu, 28 Mar 2024 00:53:24 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 11/12] wifi: rtlwifi: Add rtl8192du/sw.{c,h}
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>
Cc: Ping-Ke Shih <pkshih@realtek.com>,
 "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>
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <87ttkrzf1m.fsf@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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;
>> 	}
> 
> Few quick comments, I haven't reviewed the actual patchset yet:
> 
> The mutexes look too finegrained and makes me suspicious about the
> locking design.
> > Having global variables like globalmutex_power is a big no no. They would
> not work if there are two devices on the same host, right?
> 
> Conditional locking is usually something to avoid.
> 

The design is not mine, I can't fix that. I don't even have
the type of device which needs all these mutexes. There are
two types of RTL8192DU:

* Single MAC single PHY: this is the one I have. I can still
buy it from Aliexpress. It's a lot like the other Realtek
Wifi 4 USB devices.

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

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.

> I'm starting to wonder if extending rtlwifi is actually the right
> approach. We have modern drivers like rtl8xxxu, rtw88 etc. with better
> design.
> 

I think extending rtlwifi is the right approach when it already
has most of the code needed. I really don't want to reinvent this
particular wheel. I don't want to add the dual MAC stuff to
rtl8xxxu or rtw88. I don't think rtw88 is the right driver for
this old chip, anyway.