Re: [PATCH v2] rtw88: pci: enable MSI interrupt

[Jian-Hong Pan <jian-hong@endlessm.com>]

Tony Chuang <yhchuang@realtek.com> 於 2019年8月27日 週二 下午8:12寫道：
>
> Hi Daniel and Ján,
>
>
> > From: Ján Veselý > 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 follow
> > > this.
> > >
> > > You're worried about the case where we're inside the interrupt handler 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 drivers
> > > 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 across 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 interrupt
> 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 by
> disabling the IMR.
>
>
> >
> >
> > >
> > > See e.g. https://patchwork.kernel.org/patch/3333681/ as a reference for
> > > what correct interrupt controller behaviour should look like.
> > >
> > > > +             ret = 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-howto.rst

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

Jian-Hong Pan