linux-kernel - Re: [PATCH v2] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPpJ_edU68X-Ki+J61qfws+1-=zv54bcak9tzkMX=CkDS5mOMA@mail.gmail.com>
Date:   Mon, 19 Aug 2019 11:26:02 +0800
From:   Jian-Hong Pan <jian-hong@...lessm.com>
To:     Tony Chuang <yhchuang@...ltek.com>
Cc:     Kalle Valo <kvalo@...eaurora.org>,
        "David S . Miller" <davem@...emloft.net>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux@...lessm.com" <linux@...lessm.com>
Subject: Re: [PATCH v2] rtw88: pci: Move a mass of jobs in hw IRQ to soft IRQ

Tony Chuang <yhchuang@...ltek.com> 於 2019年8月16日 週五 下午6:44寫道：
>
> > From: Jian-Hong Pan
> >
> > There is a mass of jobs between spin lock and unlock in the hardware
> > IRQ which will occupy much time originally. To make system work more
> > efficiently, this patch moves the jobs to the soft IRQ (bottom half) to
> > reduce the time in hardware IRQ.
> >
> > Signed-off-by: Jian-Hong Pan <jian-hong@...lessm.com>
> > ---
> > v2:
> >  Change the spin_lock_irqsave/unlock_irqrestore to spin_lock/unlock in
> >  rtw_pci_interrupt_handler. Because the interrupts are already disabled
> >  in the hardware interrupt handler.
> >
> >  drivers/net/wireless/realtek/rtw88/pci.c | 33 +++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > index 00ef229552d5..0740140d7e46 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -866,12 +866,28 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq,
> > void *dev)
> >  {
> >       struct rtw_dev *rtwdev = dev;
> >       struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> > -     u32 irq_status[4];
> >
> >       spin_lock(&rtwpci->irq_lock);
> >       if (!rtwpci->irq_enabled)
> >               goto out;
> >
> > +     /* disable RTW PCI interrupt to avoid more interrupts before the end of
> > +      * thread function
> > +      */
> > +     rtw_pci_disable_interrupt(rtwdev, rtwpci);
>
> So basically it's to prevent back-to-back interrupts.
>
> Nothing wrong about it, I just wondering why we don't like
> back-to-back interrupts. Does it means that those interrupts
> fired between irq_handler and threadfin would increase
> much more time to consume them.

1. It is one of the reasons.  Besides, the hardware interrupt has
higher priority than soft IRQ.  If next hardware interrupt is coming
faster then the soft IRQ (BH), the software IRQ may always become
pended.
2. Keep the data's state until the interrupt is fully dealt.
3. The process of request_threaded_irq is documented:
https://www.kernel.org/doc/htmldocs/kernel-api/API-request-threaded-irq.html

> > +out:
> > +     spin_unlock(&rtwpci->irq_lock);
> > +
> > +     return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t rtw_pci_interrupt_threadfn(int irq, void *dev)
> > +{
> > +     struct rtw_dev *rtwdev = dev;
> > +     struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> > +     unsigned long flags;
> > +     u32 irq_status[4];
> > +
> >       rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status);
> >
> >       if (irq_status[0] & IMR_MGNTDOK)
> > @@ -891,8 +907,11 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq,
> > void *dev)
> >       if (irq_status[0] & IMR_ROK)
> >               rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU);
> >
> > -out:
> > -     spin_unlock(&rtwpci->irq_lock);
> > +     /* all of the jobs for this interrupt have been done */
> > +     spin_lock_irqsave(&rtwpci->irq_lock, flags);
>
> I suggest to protect the ISRs. Because next patches will require
> to check if the TX DMA path is empty. This means I will also add
> this rtwpci->irq_lock to the TX path, and check if the skb_queue
> does not have any pending SKBs not DMAed successfully.

Ah ... Okay for the TX path

> > +     if (rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
>
> Why check the flag here? Is there any racing or something?
> Otherwise it looks to break the symmetry.

According to backtrace, I notice rtw_pci_stop will be invoked in
rtw_power_off,  rtw_core_stop which clears the running state.
rtw_core_stop is invoked by rtw_enter_ips due to IEEE80211_CONF_IDLE.
If the stop command comes between the hardware interrupt and soft IRQ
(BH) and there is no start command again, I think user wants the WiFi
stop instead of becoming start again.

Jian-Hong Pan