Received-SPF: pass (google.com: domain of linux-wireless+bounces-5393-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Kalle Valo <kvalo@kernel.org>
To: Bitterblue Smith <rtl8821cerfe2@gmail.com>
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>
Subject: Re: [PATCH v3 11/12] wifi: rtlwifi: Add rtl8192du/sw.{c,h}
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>
Date: Wed, 27 Mar 2024 20:48:53 +0200
In-Reply-To: <66565618-3638-47e5-afe5-3530214da0c9@gmail.com> (Bitterblue
	Smith's message of "Wed, 27 Mar 2024 16:07:20 +0200")
Message-ID: <87ttkrzf1m.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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.

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