RE: [PATCH net] wifi: rtw88: properly drop usb interface reference on error

Posted by Ping-Ke Shih 1 month, 1 week ago

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> If an error happens in the usb probe path, in rtw_usb_intf_init(), the
> usb interface reference needs to be properly dropped, otherwise is is
> incorrectly increased when returning to the USB core.
> 
> Cc: Ping-Ke Shih <pkshih@realtek.com>
> Cc: stable <stable@kernel.org>
> Assisted-by: gkh_clanker_2000
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 433b06c8d8a6..36ac20039ce2 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -1046,13 +1046,17 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
> 
>         rtwusb->udev = udev;
>         ret = rtw_usb_parse(rtwdev, intf);
> -       if (ret)
> +       if (ret) {
> +               usb_put_dev(udev);
>                 return ret;
> +       }
> 
>         rtwusb->usb_data = kcalloc(RTW_USB_MAX_RXTX_COUNT, sizeof(u32),
>                                    GFP_KERNEL);
> -       if (!rtwusb->usb_data)
> +       if (!rtwusb->usb_data) {
> +               usb_put_dev(udev);
>                 return -ENOMEM;
> +       }
> 
>         usb_set_intfdata(intf, rtwdev->hw);
> 

Since rtwusb->udev isn't used right after assignment in this function.
Would it be simpler that moving usb_get_dev() downward like below?

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index db60e142268d..6e5c9c6f3e00 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -1041,10 +1041,8 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
                             struct usb_interface *intf)
 {
        struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
-       struct usb_device *udev = usb_get_dev(interface_to_usbdev(intf));
        int ret;

-       rtwusb->udev = udev;
        ret = rtw_usb_parse(rtwdev, intf);
        if (ret)
                return ret;
@@ -1054,6 +1052,8 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
        if (!rtwusb->usb_data)
                return -ENOMEM;

+       rtwusb->udev = usb_get_dev(interface_to_usbdev(intf));
+
        usb_set_intfdata(intf, rtwdev->hw);

        SET_IEEE80211_DEV(rtwdev->hw, &intf->dev);

Re: [PATCH net] wifi: rtw88: properly drop usb interface reference on error

Posted by Greg Kroah-Hartman 1 month, 1 week ago

On Tue, Feb 24, 2026 at 12:46:02AM +0000, Ping-Ke Shih wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > If an error happens in the usb probe path, in rtw_usb_intf_init(), the
> > usb interface reference needs to be properly dropped, otherwise is is
> > incorrectly increased when returning to the USB core.
> > 
> > Cc: Ping-Ke Shih <pkshih@realtek.com>
> > Cc: stable <stable@kernel.org>
> > Assisted-by: gkh_clanker_2000
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/net/wireless/realtek/rtw88/usb.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> > index 433b06c8d8a6..36ac20039ce2 100644
> > --- a/drivers/net/wireless/realtek/rtw88/usb.c
> > +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> > @@ -1046,13 +1046,17 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
> > 
> >         rtwusb->udev = udev;
> >         ret = rtw_usb_parse(rtwdev, intf);
> > -       if (ret)
> > +       if (ret) {
> > +               usb_put_dev(udev);
> >                 return ret;
> > +       }
> > 
> >         rtwusb->usb_data = kcalloc(RTW_USB_MAX_RXTX_COUNT, sizeof(u32),
> >                                    GFP_KERNEL);
> > -       if (!rtwusb->usb_data)
> > +       if (!rtwusb->usb_data) {
> > +               usb_put_dev(udev);
> >                 return -ENOMEM;
> > +       }
> > 
> >         usb_set_intfdata(intf, rtwdev->hw);
> > 
> 
> Since rtwusb->udev isn't used right after assignment in this function.
> Would it be simpler that moving usb_get_dev() downward like below?

What is even simpler, and easier, is to never call usb_get_dev() at all
anyway as it's not needed :)

I created that pattern a few decades ago when we thought that it was
going to be required, but as long as the usb interface is bound to the
driver, that pointer is going to be valid so there's no real need to
increment the reference count, except to feel good about doing it.

I'll gladly do that fix instead, if you want me to, I was just trying to
follow the style of the existing code and fix up the current bug.

> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index db60e142268d..6e5c9c6f3e00 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -1041,10 +1041,8 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
>                              struct usb_interface *intf)
>  {
>         struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> -       struct usb_device *udev = usb_get_dev(interface_to_usbdev(intf));
>         int ret;
> 
> -       rtwusb->udev = udev;
>         ret = rtw_usb_parse(rtwdev, intf);
>         if (ret)
>                 return ret;
> @@ -1054,6 +1052,8 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
>         if (!rtwusb->usb_data)
>                 return -ENOMEM;
> 
> +       rtwusb->udev = usb_get_dev(interface_to_usbdev(intf));

That too works, or again, just drop the usb_get_dev() and usb_put_dev()
calls entirely.

thanks,

greg k-h

RE: [PATCH net] wifi: rtw88: properly drop usb interface reference on error

Posted by Ping-Ke Shih 1 month, 1 week ago

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, Feb 24, 2026 at 12:46:02AM +0000, Ping-Ke Shih wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > If an error happens in the usb probe path, in rtw_usb_intf_init(), the
> > > usb interface reference needs to be properly dropped, otherwise is is
> > > incorrectly increased when returning to the USB core.
> > >
> > > Cc: Ping-Ke Shih <pkshih@realtek.com>
> > > Cc: stable <stable@kernel.org>
> > > Assisted-by: gkh_clanker_2000
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  drivers/net/wireless/realtek/rtw88/usb.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> > > index 433b06c8d8a6..36ac20039ce2 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/usb.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> > > @@ -1046,13 +1046,17 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
> > >
> > >         rtwusb->udev = udev;
> > >         ret = rtw_usb_parse(rtwdev, intf);
> > > -       if (ret)
> > > +       if (ret) {
> > > +               usb_put_dev(udev);
> > >                 return ret;
> > > +       }
> > >
> > >         rtwusb->usb_data = kcalloc(RTW_USB_MAX_RXTX_COUNT, sizeof(u32),
> > >                                    GFP_KERNEL);
> > > -       if (!rtwusb->usb_data)
> > > +       if (!rtwusb->usb_data) {
> > > +               usb_put_dev(udev);
> > >                 return -ENOMEM;
> > > +       }
> > >
> > >         usb_set_intfdata(intf, rtwdev->hw);
> > >
> >
> > Since rtwusb->udev isn't used right after assignment in this function.
> > Would it be simpler that moving usb_get_dev() downward like below?
> 
> What is even simpler, and easier, is to never call usb_get_dev() at all
> anyway as it's not needed :)
> 
> I created that pattern a few decades ago when we thought that it was
> going to be required, but as long as the usb interface is bound to the
> driver, that pointer is going to be valid so there's no real need to
> increment the reference count, except to feel good about doing it.
> 
> I'll gladly do that fix instead, if you want me to, I was just trying to
> follow the style of the existing code and fix up the current bug.

Because I'm not much familiar with USB devices, I can't afford to the change
that is too big to me. :)

Let's take your version, so

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

This patch can go via rtw-next tree (subtree of wireless-next), right?

Re: [PATCH net] wifi: rtw88: properly drop usb interface reference on error

Posted by Greg Kroah-Hartman 1 month, 1 week ago

On Tue, Feb 24, 2026 at 01:49:24AM +0000, Ping-Ke Shih wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > On Tue, Feb 24, 2026 at 12:46:02AM +0000, Ping-Ke Shih wrote:
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > > If an error happens in the usb probe path, in rtw_usb_intf_init(), the
> > > > usb interface reference needs to be properly dropped, otherwise is is
> > > > incorrectly increased when returning to the USB core.
> > > >
> > > > Cc: Ping-Ke Shih <pkshih@realtek.com>
> > > > Cc: stable <stable@kernel.org>
> > > > Assisted-by: gkh_clanker_2000
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > >  drivers/net/wireless/realtek/rtw88/usb.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> > > > index 433b06c8d8a6..36ac20039ce2 100644
> > > > --- a/drivers/net/wireless/realtek/rtw88/usb.c
> > > > +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> > > > @@ -1046,13 +1046,17 @@ static int rtw_usb_intf_init(struct rtw_dev *rtwdev,
> > > >
> > > >         rtwusb->udev = udev;
> > > >         ret = rtw_usb_parse(rtwdev, intf);
> > > > -       if (ret)
> > > > +       if (ret) {
> > > > +               usb_put_dev(udev);
> > > >                 return ret;
> > > > +       }
> > > >
> > > >         rtwusb->usb_data = kcalloc(RTW_USB_MAX_RXTX_COUNT, sizeof(u32),
> > > >                                    GFP_KERNEL);
> > > > -       if (!rtwusb->usb_data)
> > > > +       if (!rtwusb->usb_data) {
> > > > +               usb_put_dev(udev);
> > > >                 return -ENOMEM;
> > > > +       }
> > > >
> > > >         usb_set_intfdata(intf, rtwdev->hw);
> > > >
> > >
> > > Since rtwusb->udev isn't used right after assignment in this function.
> > > Would it be simpler that moving usb_get_dev() downward like below?
> > 
> > What is even simpler, and easier, is to never call usb_get_dev() at all
> > anyway as it's not needed :)
> > 
> > I created that pattern a few decades ago when we thought that it was
> > going to be required, but as long as the usb interface is bound to the
> > driver, that pointer is going to be valid so there's no real need to
> > increment the reference count, except to feel good about doing it.
> > 
> > I'll gladly do that fix instead, if you want me to, I was just trying to
> > follow the style of the existing code and fix up the current bug.
> 
> Because I'm not much familiar with USB devices, I can't afford to the change
> that is too big to me. :)
> 
> Let's take your version, so
> 
> Acked-by: Ping-Ke Shih <pkshih@realtek.com>

Great, thanks!

> This patch can go via rtw-next tree (subtree of wireless-next), right?

Sure, please do so.

thanks,

greg k-h