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;
> }
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.
I'm starting to wonder if extending rtlwifi is actually the right
approach. We have modern drivers like rtl8xxxu, rtw88 etc. with better
design.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
