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

[Ping-Ke Shih <pkshih@realtek.com>]

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.