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>
In-Reply-To: <20190823063728.14117-1-drake@endlessm.com>
From:   =?UTF-8?B?SsOhbiBWZXNlbMO9?= <jano.vesely@gmail.com>
Date:   Fri, 23 Aug 2019 03:22:18 -0400
Message-ID: <CA+K+NcSYKEkdx5ux6iwUs7pMidObZBrg9yDcP1zT73DcccpDPQ@mail.gmail.com>
Subject: Re: [PATCH v2] rtw88: pci: enable MSI interrupt
To:     Daniel Drake <drake@endlessm.com>
Cc:     Tony Chuang <yhchuang@realtek.com>, briannorris@chromium.org,
        gojun077@gmail.com, kvalo@codeaurora.org,
        linux-wireless <linux-wireless@vger.kernel.org>,
        linux@endlessm.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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


>
> 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"
>
> Thanks
> Daniel
>