Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1565166487-22048-1-git-send-email-yhchuang@realtek.com>
 <20190823063728.14117-1-drake@endlessm.com> <CA+K+NcSYKEkdx5ux6iwUs7pMidObZBrg9yDcP1zT73DcccpDPQ@mail.gmail.com>
 <F7CD281DE3E379468C6D07993EA72F84D18AE7C8@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D18AE7C8@RTITMBSVM04.realtek.com.tw>
From:   Jian-Hong Pan <jian-hong@endlessm.com>
Date:   Tue, 27 Aug 2019 21:02:52 +0800
Message-ID: <CAPpJ_echMhQoKG=+rtkhMjdeFGDPbuyzE-DLBZ-7KL5+WcEVHw@mail.gmail.com>
Subject: Re: [PATCH v2] rtw88: pci: enable MSI interrupt
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     =?UTF-8?B?SsOhbiBWZXNlbMO9?= <jano.vesely@gmail.com>,
        Daniel Drake <drake@endlessm.com>,
        "briannorris@chromium.org" <briannorris@chromium.org>,
        "gojun077@gmail.com" <gojun077@gmail.com>,
        "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        "linux@endlessm.com" <linux@endlessm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Tony Chuang <yhchuang@realtek.com> =E6=96=BC 2019=E5=B9=B48=E6=9C=8827=E6=
=97=A5 =E9=80=B1=E4=BA=8C =E4=B8=8B=E5=8D=888:12=E5=AF=AB=E9=81=93=EF=BC=9A
>
> Hi Daniel and J=C3=A1n,
>
>
> > From: J=C3=A1n Vesel=C3=BD > Sent: Friday, August 23, 2019 3:22 PM
> > On Fri, Aug 23, 2019 at 2:37 AM Daniel Drake <drake@endlessm.com> wrote=
:
> > >
> > > > +     rtw_pci_disable_interrupt(rtwdev, rtwpci);
> > >
> > > I checked the discussion on the v1 patch thread but I still don't fol=
low
> > > this.
> > >
> > > You're worried about the case where we're inside the interrupt handle=
r and:
> > >  1. We read the interrupt status to note what needs to be done
> > >  2. <another interrupt arrives here, requiring other work to be done>
> > >  3. We clear the interrupt status bits
> > >  4. We proceed to handle the interrupt but missing any work requested=
 by
> > >     the interrupt in step 2.
> > >
> > > Is that right?
> > >
> > > I'm not an expert here, but I don't think this is something that driv=
ers
> > > have to worry about. Surely the interrupt controller can be expected =
to
> > > have a mechanism to "queue up" any interrupt that arrives while an
> > > interrupt is being handled? Otherwise handling of all types of
> > > edge-triggered interrupts (not just MSI) would be overly painful acro=
ss the
> > > board.
> >
> > That's my understanding as well.
> > entering the interrupt vector clears the IFLAG, so any interrupt will
> > wait until the IFLAG is restored, or delivered to a different CPU.
> > wouldn't it be safer to enable interrupts only _after_ registering the
> > handler in the "rtw_pci_request_irq" function?
> >
> > regards,
> > Jan
>
>
> Yes that's not something drivers need to care about. But I think it is
> Because there's a race condition between SW/HW when clearing the ISR.
> If interrupt comes after reading ISR and before write-1-clear, the interr=
upt
> controller would have interrupt status raised, and never issue interrupt
> signal to host when other new interrupts status are raised.
>
> To avoid this, driver requires to protect the ISR write-1-clear process b=
y
> disabling the IMR.
>
>
> >
> >
> > >
> > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference f=
or
> > > what correct interrupt controller behaviour should look like.
> > >
> > > > +             ret =3D pci_enable_msi(pdev);
> > >
> > > pci_enable_msi() is "deprecated, don't use"
>
> Do you mean I should remove this?
> But I cannot find another proper way to enable the MSI.
> Maybe pci_alloc_irq_vectors() could work but I am not sure if
> It is recommended.

According to the kernel documentation "The MSI Driver Guide HOWTO",
pci_alloc_irq_vectors, pci_irq_vector and pci_free_irq_vectors are the
functions.
https://elixir.bootlin.com/linux/v5.3-rc6/source/Documentation/PCI/msi-howt=
o.rst

Here is an example in r8169 module.
https://elixir.bootlin.com/linux/v5.3-rc6/source/drivers/net/ethernet/realt=
ek/r8169_main.c#L6603

Jian-Hong Pan