Received-SPF: pass (google.com: domain of linux-wireless+bounces-6002-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <ab85199c-e82b-4ecf-994c-77fb418429bd@gmail.com>
Date: Tue, 9 Apr 2024 14:16:46 +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>
 <519e6717-3a58-4814-932d-8b246fd39f37@gmail.com>
 <253da7b1650d47b59114c635e7ad8dc3@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <253da7b1650d47b59114c635e7ad8dc3@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 09/04/2024 03:27, Ping-Ke Shih wrote:
> 
> 
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
>>
>> On 08/04/2024 05:45, Ping-Ke Shih wrote:
>>>>
>>>>
>>>> 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;
>>
> 
> That looks easier to understand. 
> 
>>>
>>>>
>>>> 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);
> 
> Will usb_get_intfdata(other_intf) return NULL if the intf disconnected? 
> If yes, that looks good to me. 
> 
> 

It should. rtl_usb_disconnect() has usb_set_intfdata(intf, NULL);
at the end. Also usb_unbind_interface() in drivers/usb/core/driver.c
does the same after calling rtl_usb_disconnect().