LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: "Pali Rohár" <pali@kernel.org>
Cc: "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Marek Behún" <kabel@kernel.org>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts
Date: Sun, 15 Aug 2021 22:55:01 +0100	[thread overview]
Message-ID: <D64971ED-5502-4A7B-8176-B4FBF00CD80B@kernel.org> (raw)
In-Reply-To: <20210815173659.2mjug3jffp6a4ybg@pali>



On 15 August 2021 18:36:59 BST, "Pali Rohár" <pali@kernel.org> wrote:
>On Sunday 15 August 2021 17:56:04 Marc Zyngier wrote:
>> On Sun, 15 Aug 2021 11:36:23 +0100,
>> Pali Rohár <pali@kernel.org> wrote:
>> > 
>> > Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
>> > register. At the driver probe time mask all MSI interrupts and then let
>> > kernel IRQ chip code to unmask particular MSI interrupt when needed.
>> > 
>> > Signed-off-by: Pali Rohár <pali@kernel.org>
>> > Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
>> > ---
>> >  drivers/pci/controller/pci-aardvark.c | 44 ++++++++++++++++++++++++---
>> >  1 file changed, 40 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
>> > index bacfccee44fe..96580e1e4539 100644
>> > --- a/drivers/pci/controller/pci-aardvark.c
>> > +++ b/drivers/pci/controller/pci-aardvark.c
>> > @@ -480,12 +480,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
>> >  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
>> >  
>> > -	/* Disable All ISR0/1 Sources */
>> > +	/* Disable All ISR0/1 and MSI Sources */
>> >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
>> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
>> > -
>> > -	/* Unmask all MSIs */
>> > -	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
>> > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
>> >  
>> >  	/* Unmask summary MSI interrupt */
>> >  	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
>> > @@ -1026,6 +1024,40 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
>> >  	return -EINVAL;
>> >  }
>> >  
>> > +static void advk_msi_irq_mask(struct irq_data *d)
>> > +{
>> > +	struct advk_pcie *pcie = d->domain->host_data;
>> > +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
>> > +	u32 mask;
>> > +
>> > +	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
>> > +	mask |= BIT(hwirq);
>> > +	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
>> 
>> This isn't atomic, and will results in corruption when two MSIs are
>> masked/unmasked concurrently.
>
>Does it mean that also current implementation of masking legacy
>interrupt is incorrect?
>
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?h=v5.13#n874

Yes, that's completely busted. If you have configuration registers that are shared between interrupts and that the HW doesn't provide set/clear accessors so that it can cope with such races, you need mutual exclusion.

You'd think people would have worked that one out.... 60 years ago?

        M.

Jazz is not dead, it just smells funny

  reply	other threads:[~2021-08-15 21:55 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-15 10:36 [PATCH 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár
2021-08-15 10:36 ` [PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár
2021-08-15 10:36 ` [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár
2021-08-15 16:56   ` Marc Zyngier
2021-08-15 17:36     ` Pali Rohár
2021-08-15 21:55       ` Marc Zyngier [this message]
2021-08-15 23:10         ` Pali Rohár
2021-08-15 10:36 ` [PATCH 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár
2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár
2021-08-23 16:40   ` [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár
2021-08-23 16:52     ` Marc Zyngier
2021-08-23 16:40   ` [PATCH v2 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár
2021-08-23 16:40   ` [PATCH v2 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár

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=D64971ED-5502-4A7B-8176-B4FBF00CD80B@kernel.org \
    --to=maz@kernel.org \
    --cc=bhelgaas@google.com \
    --cc=kabel@kernel.org \
    --cc=kw@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=pali@kernel.org \
    --cc=robh@kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    --subject='Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts' \
    /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

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).