| Message ID | 20241122172718.465539-3-cascardo@igalia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: rtlwifi probe error path fixes | expand |
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > > rtl_wq is allocated at rtl_init_core, so it makes more sense to destroy it > at rtl_deinit_core. In the case of USB, where _rtl_usb_init does not > require anything to be undone, that is fine. But for PCI, rtl_pci_init, > which is called after rtl_init_core, needs to deallocate data, but only if > it has been called. > > That means that destroying the workqueue needs to be done whether > rtl_pci_init has been called or not. And since rtl_pci_deinit was doing it, > it has to be moved out of there. > > It makes more sense to move it to rtl_deinit_core and have it done in both > cases, USB and PCI. > > Since this is a requirement for a followup memory leak fix, mark this as > fixing such memory leak. > > Fixes: 0c8173385e54 ("rtl8192ce: Add new driver") Like patch 1/3, this is a cleanup patch, so I don't think a Fixes is needed. > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> "rtlwifi" is a typo in subject. > --- > drivers/net/wireless/realtek/rtlwifi/base.c | 5 +++++ > drivers/net/wireless/realtek/rtlwifi/pci.c | 2 -- > drivers/net/wireless/realtek/rtlwifi/usb.c | 5 ----- > 3 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c > index fd28c7a722d8..062d5a0a4c11 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/base.c > +++ b/drivers/net/wireless/realtek/rtlwifi/base.c > @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw, > > void rtl_deinit_core(struct ieee80211_hw *hw) > { > + struct rtl_priv *rtlpriv = rtl_priv(hw); A blank line between declarations and statements. > rtl_c2hcmd_launcher(hw, 0); > rtl_free_entries_from_scan_list(hw); > rtl_free_entries_from_ack_queue(hw, false); > + if (rtlpriv->works.rtl_wq) { > + destroy_workqueue(rtlpriv->works.rtl_wq); > + rtlpriv->works.rtl_wq = NULL; > + } > } > EXPORT_SYMBOL_GPL(rtl_deinit_core); > > diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c > index 4388066eb9e2..e60ac910e750 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/pci.c > +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c > @@ -1656,8 +1656,6 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw) > synchronize_irq(rtlpci->pdev->irq); > tasklet_kill(&rtlpriv->works.irq_tasklet); > cancel_work_sync(&rtlpriv->works.lps_change_work); > - > - destroy_workqueue(rtlpriv->works.rtl_wq); > } > > static int rtl_pci_init(struct ieee80211_hw *hw, struct pci_dev *pdev) > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c > index 0368ecea2e81..f5718e570011 100644 > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c > @@ -629,11 +629,6 @@ static void _rtl_usb_cleanup_rx(struct ieee80211_hw *hw) > tasklet_kill(&rtlusb->rx_work_tasklet); > cancel_work_sync(&rtlpriv->works.lps_change_work); > > - if (rtlpriv->works.rtl_wq) { > - destroy_workqueue(rtlpriv->works.rtl_wq); > - rtlpriv->works.rtl_wq = NULL; > - } > - > skb_queue_purge(&rtlusb->rx_queue); > > while ((urb = usb_get_from_anchor(&rtlusb->rx_cleanup_urbs))) { > -- > 2.34.1
On Wed, Nov 27, 2024 at 05:35:23AM +0000, Ping-Ke Shih wrote: > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > > > > rtl_wq is allocated at rtl_init_core, so it makes more sense to destroy it > > at rtl_deinit_core. In the case of USB, where _rtl_usb_init does not > > require anything to be undone, that is fine. But for PCI, rtl_pci_init, > > which is called after rtl_init_core, needs to deallocate data, but only if > > it has been called. > > > > That means that destroying the workqueue needs to be done whether > > rtl_pci_init has been called or not. And since rtl_pci_deinit was doing it, > > it has to be moved out of there. > > > > It makes more sense to move it to rtl_deinit_core and have it done in both > > cases, USB and PCI. > > > > Since this is a requirement for a followup memory leak fix, mark this as > > fixing such memory leak. > > > > Fixes: 0c8173385e54 ("rtl8192ce: Add new driver") > > Like patch 1/3, this is a cleanup patch, so I don't think a Fixes is needed. > This is a pre-requisite for patch 3/4. Without it, the workqueue destruction in the PCI probe error path would not happen when it should, resulting in memory leaks and NULL pointer dereferences. > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> > > "rtlwifi" is a typo in subject. > Can you fix that when applying, or should I send a v2? > > --- > > drivers/net/wireless/realtek/rtlwifi/base.c | 5 +++++ > > drivers/net/wireless/realtek/rtlwifi/pci.c | 2 -- > > drivers/net/wireless/realtek/rtlwifi/usb.c | 5 ----- > > 3 files changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c > > index fd28c7a722d8..062d5a0a4c11 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/base.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/base.c > > @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw, > > > > void rtl_deinit_core(struct ieee80211_hw *hw) > > { > > + struct rtl_priv *rtlpriv = rtl_priv(hw); > > A blank line between declarations and statements. > Same here. > > rtl_c2hcmd_launcher(hw, 0); > > rtl_free_entries_from_scan_list(hw); > > rtl_free_entries_from_ack_queue(hw, false); > > + if (rtlpriv->works.rtl_wq) { > > + destroy_workqueue(rtlpriv->works.rtl_wq); > > + rtlpriv->works.rtl_wq = NULL; > > + } > > } > > EXPORT_SYMBOL_GPL(rtl_deinit_core); > > > > diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c > > index 4388066eb9e2..e60ac910e750 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/pci.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c > > @@ -1656,8 +1656,6 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw) > > synchronize_irq(rtlpci->pdev->irq); > > tasklet_kill(&rtlpriv->works.irq_tasklet); > > cancel_work_sync(&rtlpriv->works.lps_change_work); > > - > > - destroy_workqueue(rtlpriv->works.rtl_wq); > > } > > > > static int rtl_pci_init(struct ieee80211_hw *hw, struct pci_dev *pdev) > > diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c > > index 0368ecea2e81..f5718e570011 100644 > > --- a/drivers/net/wireless/realtek/rtlwifi/usb.c > > +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c > > @@ -629,11 +629,6 @@ static void _rtl_usb_cleanup_rx(struct ieee80211_hw *hw) > > tasklet_kill(&rtlusb->rx_work_tasklet); > > cancel_work_sync(&rtlpriv->works.lps_change_work); > > > > - if (rtlpriv->works.rtl_wq) { > > - destroy_workqueue(rtlpriv->works.rtl_wq); > > - rtlpriv->works.rtl_wq = NULL; > > - } > > - > > skb_queue_purge(&rtlusb->rx_queue); > > > > while ((urb = usb_get_from_anchor(&rtlusb->rx_cleanup_urbs))) { > > -- > > 2.34.1 >
Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > On Wed, Nov 27, 2024 at 05:35:23AM +0000, Ping-Ke Shih wrote: > > Thadeu Lima de Souza Cascardo <cascardo@igalia.com> wrote: > > > index fd28c7a722d8..062d5a0a4c11 100644 > > > --- a/drivers/net/wireless/realtek/rtlwifi/base.c > > > +++ b/drivers/net/wireless/realtek/rtlwifi/base.c > > > @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw, > > > > > > void rtl_deinit_core(struct ieee80211_hw *hw) > > > { > > > + struct rtl_priv *rtlpriv = rtl_priv(hw); > > > > A blank line between declarations and statements. > > > > Same here. Prefer you send v2 to prevent I mess up the code.
diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c index fd28c7a722d8..062d5a0a4c11 100644 --- a/drivers/net/wireless/realtek/rtlwifi/base.c +++ b/drivers/net/wireless/realtek/rtlwifi/base.c @@ -575,9 +575,14 @@ static void rtl_free_entries_from_ack_queue(struct ieee80211_hw *hw, void rtl_deinit_core(struct ieee80211_hw *hw) { + struct rtl_priv *rtlpriv = rtl_priv(hw); rtl_c2hcmd_launcher(hw, 0); rtl_free_entries_from_scan_list(hw); rtl_free_entries_from_ack_queue(hw, false); + if (rtlpriv->works.rtl_wq) { + destroy_workqueue(rtlpriv->works.rtl_wq); + rtlpriv->works.rtl_wq = NULL; + } } EXPORT_SYMBOL_GPL(rtl_deinit_core); diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c index 4388066eb9e2..e60ac910e750 100644 --- a/drivers/net/wireless/realtek/rtlwifi/pci.c +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c @@ -1656,8 +1656,6 @@ static void rtl_pci_deinit(struct ieee80211_hw *hw) synchronize_irq(rtlpci->pdev->irq); tasklet_kill(&rtlpriv->works.irq_tasklet); cancel_work_sync(&rtlpriv->works.lps_change_work); - - destroy_workqueue(rtlpriv->works.rtl_wq); } static int rtl_pci_init(struct ieee80211_hw *hw, struct pci_dev *pdev) diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c index 0368ecea2e81..f5718e570011 100644 --- a/drivers/net/wireless/realtek/rtlwifi/usb.c +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c @@ -629,11 +629,6 @@ static void _rtl_usb_cleanup_rx(struct ieee80211_hw *hw) tasklet_kill(&rtlusb->rx_work_tasklet); cancel_work_sync(&rtlpriv->works.lps_change_work); - if (rtlpriv->works.rtl_wq) { - destroy_workqueue(rtlpriv->works.rtl_wq); - rtlpriv->works.rtl_wq = NULL; - } - skb_queue_purge(&rtlusb->rx_queue); while ((urb = usb_get_from_anchor(&rtlusb->rx_cleanup_urbs))) {
rtl_wq is allocated at rtl_init_core, so it makes more sense to destroy it at rtl_deinit_core. In the case of USB, where _rtl_usb_init does not require anything to be undone, that is fine. But for PCI, rtl_pci_init, which is called after rtl_init_core, needs to deallocate data, but only if it has been called. That means that destroying the workqueue needs to be done whether rtl_pci_init has been called or not. And since rtl_pci_deinit was doing it, it has to be moved out of there. It makes more sense to move it to rtl_deinit_core and have it done in both cases, USB and PCI. Since this is a requirement for a followup memory leak fix, mark this as fixing such memory leak. Fixes: 0c8173385e54 ("rtl8192ce: Add new driver") Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> --- drivers/net/wireless/realtek/rtlwifi/base.c | 5 +++++ drivers/net/wireless/realtek/rtlwifi/pci.c | 2 -- drivers/net/wireless/realtek/rtlwifi/usb.c | 5 ----- 3 files changed, 5 insertions(+), 7 deletions(-)