| Message ID | 20241107133322.855112-6-cascardo@igalia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | wifi: rtlwifi: usb probe error path fixes | expand |
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > rtl_init_core creates a workqueue that is then assigned to rtl_wq. > rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which > must be called in the probe error path. > > Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver") > Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.") > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> > --- > drivers/net/wireless/realtek/rtlwifi/usb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c > index 8ec687fab572..0368ecea2e81 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c > @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf, > wait_for_completion(&rtlpriv->firmware_loading_complete); > rtlpriv->cfg->ops->deinit_sw_vars(hw); > error_out: > + rtl_usb_deinit(hw); > rtl_deinit_core(hw); > error_out2: > _rtl_usb_io_handler_release(hw); I think deinit should be in reverse order of init step by step: --- a/drivers/net/wireless/realtek/rtlwifi/usb.c +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c @@ -1017,7 +1017,7 @@ int rtl_usb_probe(struct usb_interface *intf, err = rtl_init_core(hw); if (err) { pr_err("Can't allocate sw for mac80211\n"); - goto error_out2; + goto error_out_usb_deinit; } if (rtlpriv->cfg->ops->init_sw_vars(hw)) { pr_err("Can't init_sw_vars\n"); @@ -1040,6 +1040,8 @@ int rtl_usb_probe(struct usb_interface *intf, rtlpriv->cfg->ops->deinit_sw_vars(hw); error_out: rtl_deinit_core(hw); +error_out_usb_deinit: + rtl_usb_deinit(hw); error_out2: _rtl_usb_io_handler_release(hw); usb_put_dev(udev); Have you also considered PCI? It seems that rtl_pci_deinit() isn't called if PCI probe fails.
On Fri, Nov 08, 2024 at 02:23:48AM +0000, Ping-Ke Shih wrote: > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > > rtl_init_core creates a workqueue that is then assigned to rtl_wq. > > rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which > > must be called in the probe error path. > > > > Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver") > > Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.") > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> > > --- > > drivers/net/wireless/realtek/rtlwifi/usb.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c > > index 8ec687fab572..0368ecea2e81 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c > > @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf, > > wait_for_completion(&rtlpriv->firmware_loading_complete); > > rtlpriv->cfg->ops->deinit_sw_vars(hw); > > error_out: > > + rtl_usb_deinit(hw); > > rtl_deinit_core(hw); > > error_out2: > > _rtl_usb_io_handler_release(hw); > > I think deinit should be in reverse order of init step by step: Well, I kept the order that they appear in the remove path. Also, I checked that they are not exactly independent and rtl_usb_deinit does not need to be called when rtl_init_core fails. I have just checked and it wouldn't cause any harm either if rtl_usb_deinit is called in case rtl_init_core fails. So either way should be fine. Cascardo. > > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c > @@ -1017,7 +1017,7 @@ int rtl_usb_probe(struct usb_interface *intf, > err = rtl_init_core(hw); > if (err) { > pr_err("Can't allocate sw for mac80211\n"); > - goto error_out2; > + goto error_out_usb_deinit; > } > if (rtlpriv->cfg->ops->init_sw_vars(hw)) { > pr_err("Can't init_sw_vars\n"); > @@ -1040,6 +1040,8 @@ int rtl_usb_probe(struct usb_interface *intf, > rtlpriv->cfg->ops->deinit_sw_vars(hw); > error_out: > rtl_deinit_core(hw); > +error_out_usb_deinit: > + rtl_usb_deinit(hw); > error_out2: > _rtl_usb_io_handler_release(hw); > usb_put_dev(udev); > > Have you also considered PCI? It seems that rtl_pci_deinit() isn't called if > PCI probe fails. > >
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > On Fri, Nov 08, 2024 at 02:23:48AM +0000, Ping-Ke Shih wrote: > > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > > > rtl_init_core creates a workqueue that is then assigned to rtl_wq. > > > rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which > > > must be called in the probe error path. > > > > > > Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver") > > > Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.") > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> > > > --- > > > drivers/net/wireless/realtek/rtlwifi/usb.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c > > > index 8ec687fab572..0368ecea2e81 100644 > > > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c > > > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c > > > @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf, > > > wait_for_completion(&rtlpriv->firmware_loading_complete); > > > rtlpriv->cfg->ops->deinit_sw_vars(hw); > > > error_out: > > > + rtl_usb_deinit(hw); > > > rtl_deinit_core(hw); > > > error_out2: > > > _rtl_usb_io_handler_release(hw); > > > > I think deinit should be in reverse order of init step by step: > > Well, I kept the order that they appear in the remove path. Also, I checked > that they are not exactly independent and rtl_usb_deinit does not need to > be called when rtl_init_core fails. I have just checked and it wouldn't > cause any harm either if rtl_usb_deinit is called in case rtl_init_core > fails. So either way should be fine. Well. Original implementation of the workqueue is not symmetric. Without real hardware verification, we can't move the code of destroy queue from rtl_usb_deinit() to rtl_deinit_core().
diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c index 8ec687fab572..0368ecea2e81 100644 --- a/drivers/net/wireless/realtek/rtlwifi/usb.c +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c @@ -1039,6 +1039,7 @@ int rtl_usb_probe(struct usb_interface *intf, wait_for_completion(&rtlpriv->firmware_loading_complete); rtlpriv->cfg->ops->deinit_sw_vars(hw); error_out: + rtl_usb_deinit(hw); rtl_deinit_core(hw); error_out2: _rtl_usb_io_handler_release(hw);
rtl_init_core creates a workqueue that is then assigned to rtl_wq. rtl_deinit_core does not destroy it. It is left to rtl_usb_deinit, which must be called in the probe error path. Fixes: 2ca20f79e0d8 ("rtlwifi: Add usb driver") Fixes: 851639fdaeac ("rtlwifi: Modify some USB de-initialize code.") Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> --- drivers/net/wireless/realtek/rtlwifi/usb.c | 1 + 1 file changed, 1 insertion(+)