Received-SPF: pass (google.com: domain of linux-wireless+bounces-5970-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <519e6717-3a58-4814-932d-8b246fd39f37@gmail.com>
Date: Mon, 8 Apr 2024 12:01:26 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 11/12] wifi: rtlwifi: Add rtl8192du/sw.{c,h}
To: Ping-Ke Shih <pkshih@realtek.com>, "kvalo@kernel.org" <kvalo@kernel.org>
Cc: "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>
 <5696990a-4450-4d92-bbda-1d9ca3a9a619@gmail.com>
 <4672072ef782ae1200886a190c740e0f3ccec2c8.camel@realtek.com>
 <7873dee1-46a8-48a7-9059-bc300ac98c23@gmail.com>
 <1f8ebed6a04a4fbebf33b0a3edc2f50b@realtek.com>
 <ff3b97d0-118c-42b3-be0b-47cfcf06ab74@gmail.com>
 <7741a59a6ebf4e0f8c55b0f5c3f55a41@realtek.com>
 <321c6855-2e58-48ae-b131-a3f6d7041713@gmail.com>
 <21299940bf9342ae9c82916084ea7d9e@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <21299940bf9342ae9c82916084ea7d9e@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 08/04/2024 05:45, Ping-Ke Shih wrote:
> 
>> I did not consider using a ref_cnt variable. I made the first
>> probe allocate the things and the first disconnect frees them:
> 
> The allocation seems fine, but I feel we should free shared data by second
> disconnect because the other intf can still use them, no?
> 

Maybe. Sure, the second disconnect can free them.

>>
>>
>> static struct usb_interface *rtl92du_get_other_intf(struct ieee80211_hw *hw)
>> {
>>         struct usb_interface *intf;
>>         struct usb_device *udev;
>>         u8 other_interfaceindex;
>>
>>         /* See SET_IEEE80211_DEV(hw, &intf->dev); in usb.c */
>>         intf = container_of_const(wiphy_dev(hw->wiphy), struct usb_interface, dev);
>>
>>         other_interfaceindex = 1 - intf->altsetting[0].desc.bInterfaceNumber;
> 
> The value of bInterfaceNumber for two instances are 0 and 1, right? Then
> '1 - x' to get each other -- that looks a little tricky ;-) 
> 

The vendor driver assumes bInterfaceNumber can only be 0 or 1.
I can make it more explicit:

	if (intf->altsetting[0].desc.bInterfaceNumber == 0)
		other_interfaceindex = 1;
	else
		other_interfaceindex = 0;

>>
>>         udev = interface_to_usbdev(intf);
>>
>>         return usb_ifnum_to_if(udev, other_interfaceindex);
>> }
>>
> 
> [...]
> 
>>
>> static void rtl92du_deinit_shared_data(struct ieee80211_hw *hw)
>> {
>>         struct usb_interface *other_intf = rtl92du_get_other_intf(hw);
>>         struct rtl_priv *rtlpriv = rtl_priv(hw);
>>
>>         if (!other_intf || usb_get_intfdata(other_intf)) {
>>                 /* The other interface doesn't exist or was not disconnected yet. */
> 
> For the USB adaptor with single one interface, you don't have other_intf.
> Then, just free them.
> 
> If the USB adaptor has two interfaces, it has both other_intf and
> usb_get_intfdata(other_intf), so you want to free them. But, I wonder if both 
> interfaces can enter this branch?
> 

They can't both enter this branch because after the first
disconnect usb_get_intfdata() will return NULL.

> Also as I mentioned above, how can you ensure other_intf isn't still using the
> shared data?
> 

I can make the second disconnect free the shared data by
checking if usb_get_intfdata() returns NULL:

	if (!other_intf || !usb_get_intfdata(other_intf)) {
		/* The other interface doesn't exist or was already disconnected. */
		kfree(rtlpriv->curveindex_2g);