On Wed, Nov 27, 2024 at 05:35:23AM +0000, Ping-Ke Shih wrote:
> Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> "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
>
