Re: [PATCH v3 11/12] wifi: rtlwifi: Add rtl8192du/sw.{c,h}

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

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().