linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: "Marek Behún" <kabel@kernel.org>,
	"Bjorn Helgaas" <helgaas@kernel.org>,
	pali@kernel.org, linux-pci@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number
Date: Sat, 05 Feb 2022 10:53:48 +0000	[thread overview]
Message-ID: <87czk137pf.wl-maz@kernel.org> (raw)
In-Reply-To: <20220204172400.GA4867@lpieralisi>

Hi Lorenzo,

On Fri, 04 Feb 2022 17:24:00 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Mon, Jan 10, 2022 at 02:49:57AM +0100, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > In advk_pcie_handle_msi() the authors expect that when bit i in the W1C
> > register PCIE_MSI_STATUS_REG is cleared, the PCIE_MSI_PAYLOAD_REG is
> > updated to contain the MSI number corresponding to index i.
> > 
> > Experiments show that this is not so, and instead PCIE_MSI_PAYLOAD_REG
> > always contains the number of the last received MSI, overall.
> > 
> > Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
> > number. Since Aardvark already forbids more than 32 interrupts and uses
> > own allocated hwirq numbers, the msi_idx already corresponds to the
> > received MSI number.
> > 
> > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 62baddd2ca95..fd95ad64c887 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1393,7 +1393,6 @@ static void advk_pcie_remove_irq_domain(struct advk_pcie *pcie)
> >  static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> >  {
> >  	u32 msi_val, msi_mask, msi_status, msi_idx;
> > -	u16 msi_data;
> >  
> >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > @@ -1403,13 +1402,9 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> >  		if (!(BIT(msi_idx) & msi_status))
> >  			continue;
> >  
> > -		/*
> > -		 * msi_idx contains bits [4:0] of the msi_data and msi_data
> > -		 * contains 16bit MSI interrupt number
> > -		 */
> >  		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
> > -		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
> 
> Ok, it took me a while to understand how aardvark handles MSIs.
> 
> IIUC, msi_data contains the payload of the latest MSI write received.
>
> First off, I believe that using a Linux IRQ number for MSI data
> (payload)(I guess you rely on its truncated bits [4:0] to trigger the
> related MSI IRQs, which I believe is questionable - if not broken) is
> not a good idea.
> 
> Is my understanding correct ?
>
> This patch and the following one are fixing this. Given that this is IRQ
> domain code if Marc can cast a look into it that would help me, to make
> sure I have not missed anything.

I had a look at this series a long while ago, and nothing seem out of
sorts on the MSI front. If anything, this is better than what is
currently there.

As for the rest of the series:

Patch 18 is odd: the use of the handle_simple_irq() flow is likely
papering over something (I'd expect RP interrupts to be edge
triggered), and it is impossible to mask them. But hey, after being
singled out by the author as the big bad bully who prevents people
from fixing things, I can't say I care.

I only objected to patch 23, which is a total no-go.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

  reply	other threads:[~2022-02-05 10:53 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-10  1:49 [PATCH v2 00/23] PCI: aardvark controller fixes BATCH 4 Marek Behún
2022-01-10  1:49 ` [PATCH v2 01/23] PCI: aardvark: Replace custom PCIE_CORE_INT_* macros with PCI_INTERRUPT_* Marek Behún
2022-01-10 17:07   ` Bjorn Helgaas
2022-01-10  1:49 ` [PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number Marek Behún
2022-02-04 17:24   ` Lorenzo Pieralisi
2022-02-05 10:53     ` Marc Zyngier [this message]
2022-01-10  1:49 ` [PATCH v2 03/23] PCI: aardvark: Fix support for MSI interrupts Marek Behún
2022-01-10  1:49 ` [PATCH v2 04/23] PCI: aardvark: Rewrite IRQ code to chained IRQ handler Marek Behún
2022-01-10  1:50 ` [PATCH v2 05/23] PCI: aardvark: Check return value of generic_handle_domain_irq() when processing INTx IRQ Marek Behún
2022-01-10  1:50 ` [PATCH v2 06/23] PCI: aardvark: Make MSI irq_chip structures static driver structures Marek Behún
2022-01-10  1:50 ` [PATCH v2 07/23] PCI: aardvark: Make msi_domain_info structure a static driver structure Marek Behún
2022-01-10  1:50 ` [PATCH v2 08/23] PCI: aardvark: Use dev_fwnode() instead of of_node_to_fwnode(dev->of_node) Marek Behún
2022-01-10  1:50 ` [PATCH v2 09/23] PCI: aardvark: Refactor unmasking summary MSI interrupt Marek Behún
2022-01-10  1:50 ` [PATCH v2 10/23] PCI: aardvark: Add support for masking MSI interrupts Marek Behún
2022-01-10  1:50 ` [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address Marek Behún
2022-02-17 17:14   ` Bjorn Helgaas
2022-02-18 14:43     ` Marek Behún
2022-02-23 18:13       ` Bjorn Helgaas
2022-02-24 12:59         ` Pali Rohár
2022-02-24 19:43           ` Bjorn Helgaas
2022-01-10  1:50 ` [PATCH v2 12/23] PCI: aardvark: Enable MSI-X support Marek Behún
2022-01-10  1:50 ` [PATCH v2 13/23] PCI: aardvark: Add support for ERR interrupt on emulated bridge Marek Behún
2022-01-10  1:50 ` [PATCH v2 14/23] PCI: aardvark: Fix reading PCI_EXP_RTSTA_PME bit " Marek Behún
2022-01-10  1:50 ` [PATCH v2 15/23] PCI: aardvark: Optimize writing PCI_EXP_RTCTL_PMEIE and PCI_EXP_RTSTA_PME " Marek Behún
2022-01-10  1:50 ` [PATCH v2 16/23] PCI: aardvark: Add support for PME interrupts Marek Behún
2022-01-10  1:50 ` [PATCH v2 17/23] PCI: aardvark: Fix support for PME requester on emulated bridge Marek Behún
2022-01-10  1:50 ` [PATCH v2 18/23] PCI: aardvark: Use separate INTA interrupt for emulated root bridge Marek Behún
2022-01-10  1:50 ` [PATCH v2 19/23] PCI: aardvark: Remove irq_mask_ack callback for INTx interrupts Marek Behún
2022-01-10  1:50 ` [PATCH v2 20/23] PCI: aardvark: Don't mask irq when mapping Marek Behún
2022-01-10  1:50 ` [PATCH v2 21/23] PCI: aardvark: Drop __maybe_unused from advk_pcie_disable_phy() Marek Behún
2022-01-10  1:50 ` [PATCH v2 22/23] PCI: aardvark: Update comment about link going down after link-up Marek Behún
2022-01-10  1:50 ` [PATCH v2 23/23] PCI: aardvark: Make main irq_chip structure a static driver structure Marek Behún
2022-01-10  9:28   ` Marc Zyngier
2022-01-10 10:23     ` Marek Behún
2022-01-10 10:53     ` Pali Rohár
2022-01-10 14:44       ` Marc Zyngier
2022-01-10 15:19         ` Marek Behún
2022-02-08 10:50 ` (subset) [PATCH v2 00/23] PCI: aardvark controller fixes BATCH 4 Lorenzo Pieralisi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87czk137pf.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=helgaas@kernel.org \
    --cc=kabel@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=pali@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).