Received-SPF: pass (google.com: domain of linux-wireless+bounces-4983-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: <75128dc9-402b-4904-8c00-15dee1537c6a@gmail.com>
Date: Wed, 20 Mar 2024 15:57:24 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 00/12] wifi: rtlwifi: Add new rtl8192du driver
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "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: <5c23149c-1487-438d-bb37-69e2dd8173dc@gmail.com>
 <2280b6c991fa09e66506088441f63790d092e343.camel@realtek.com>
 <f19a1e6c-fd56-45b7-9936-a1a72d1988ad@gmail.com>
 <45b98bd53119455fa727d67640211fab@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <45b98bd53119455fa727d67640211fab@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 20/03/2024 02:57, Ping-Ke Shih wrote:
> 
>>>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:60:39: warning: context imbalance in
>>> 'rtl92d_bandtype_2_4G' - unexpected unlock
>>>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:60:39: warning: context imbalance in
>>> 'rtl92d_dm_false_alarm_counter_statistics' - unexpected unlock
>>>   drivers/net/wireless/realtek/rtlwifi/rtl8192d/phy_common.h:60:39: warning: context imbalance in
>>> 'rtl92d_dm_cck_packet_detection_thresh' - unexpected unlock
>>
>> These look like false positives. Every unlock is preceded by
>> a lock. I found a suggestion to annotate the functions with
>> "__acquires(...)" and "__releases(...)" to quiet these warnings,
>> but that didn't do anything. I can only fix it by copying the
>> contents of rtl92d_acquire_cckandrw_pagea_ctl() and
>> rtl92d_release_cckandrw_pagea_ctl() to the eight places where
>> they are called, and duplicating the code that needs locking:
>>
>>         if (rtlpriv->rtlhal.interfaceindex == 1 &&
>>             rtlpriv->rtlhal.interface == INTF_PCI) {
>>                 spin_lock_irqsave(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
>>                 temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2,
>>                                          MASKDWORD) & MASKCCK;
>>                 spin_unlock_irqrestore(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
>>         } else {
>>                 temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2,
>>                                          MASKDWORD) & MASKCCK;
>>         }
>>
> 
> Duplicate of main statements 'temp_cck = ....' isn't good. I prefer
> 
> bool need_lock = rtlpriv->rtlhal.interfaceindex == 1 &&
>                  rtlpriv->rtlhal.interface == INTF_PCI;
> 
> if (need_lock)
> 	spin_lock_irqsave(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
> 
> 	temp_cck = rtl_get_bbreg(hw, RCCK0_TXFILTER2, MASKDWORD) & MASKCCK;
> 
> if (need_lock)
> 	spin_unlock_irqrestore(&rtlpriv->locks.cck_and_rw_pagea_lock, flag);
> 

Even this doesn't work. I get the same warning as before.

> 
> But, I wonder why sparse doesn't complain original code (before your patchset)
> that used static inline already. Can we keep original style?
> 
I found the reason. In patch 2/12 I moved the two functions
from rtl8192de/phy.h to rtl8192d/phy_common.h. This should be
harmless. But I also deleted these lines from the end of
rtl8192de/phy.h:

void rtl92d_acquire_cckandrw_pagea_ctl(struct ieee80211_hw *hw,
				       unsigned long *flag);
void rtl92d_release_cckandrw_pagea_ctl(struct ieee80211_hw *hw,
				       unsigned long *flag);

They seemed pointless. If I add them to phy_common.h all the
warnings about locks go away. I will do this for v3.