| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <95adf862-6c52-ddb9-b96a-e278a1925053@socionext.com>
Date: Wed, 1 Jul 2020 11:18:29 +0900
From: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
To: Marc Zyngier <maz@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Jingoo Han <jingoohan1@...il.com>,
Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
Rob Herring <robh+dt@...nel.org>,
Masahiro Yamada <yamada.masahiro@...ionext.com>,
linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Masami Hiramatsu <masami.hiramatsu@...aro.org>,
Jassi Brar <jaswinder.singh@...aro.org>
Subject: Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to
invoke PME and AER
Hi Marc,
On 2020/06/30 22:23, Marc Zyngier wrote:
> On 2020-06-29 10:49, Kunihiko Hayashi wrote:
>> Hi Marc,
>>
>> On 2020/06/27 18:48, Marc Zyngier wrote:
>>> On Thu, 18 Jun 2020 09:38:09 +0100,
>>> Kunihiko Hayashi <hayashi.kunihiko@...ionext.com> wrote:
>>>>
>>>> The misc interrupts consisting of PME, AER, and Link event, is handled
>>>> by INTx handler, however, these interrupts should be also handled by
>>>> MSI handler.
>>>>
>>>> This adds the function uniphier_pcie_misc_isr() that handles misc
>>>> interrupts, which is called from both INTx and MSI handlers.
>>>> This function detects PME and AER interrupts with the status register,
>>>> and invoke PME and AER drivers related to MSI.
>>>>
>>>> And this sets the mask for misc interrupts from INTx if MSI is enabled
>>>> and sets the mask for misc interrupts from MSI if MSI is disabled.
>>>>
>>>> Cc: Marc Zyngier <maz@...nel.org>
>>>> Cc: Jingoo Han <jingoohan1@...il.com>
>>>> Cc: Gustavo Pimentel <gustavo.pimentel@...opsys.com>
>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
>>>> 1 file changed, 46 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> index a5401a0..5ce2479 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
>>>> @@ -44,7 +44,9 @@
>>>> #define PCL_SYS_AUX_PWR_DET BIT(8)
>>>> #define PCL_RCV_INT 0x8108
>>>> +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25)
>>>> #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17)
>>>> +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9)
>>>> #define PCL_CFG_BW_MGT_STATUS BIT(4)
>>>> #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3)
>>>> #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2)
>>>> @@ -167,7 +169,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci)
>>>> static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>>>> {
>>>> - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT);
>>>> + u32 val;
>>>> +
>>>> + val = PCL_RCV_INT_ALL_ENABLE;
>>>> + if (pci_msi_enabled())
>>>> + val |= PCL_RCV_INT_ALL_INT_MASK;
>>>> + else
>>>> + val |= PCL_RCV_INT_ALL_MSI_MASK;
>>>
>>> Does this affect endpoints? Or just the RC itself?
>>
>> These interrupts are asserted by RC itself, so this part affects only RC.
>>
>>>> +
>>>> + writel(val, priv->base + PCL_RCV_INT);
>>>> writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>>>> }
>>>> @@ -231,32 +241,56 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = {
>>>> .map = uniphier_pcie_intx_map,
>>>> };
>>>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
>>>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
>>>> {
>>>> - struct pcie_port *pp = irq_desc_get_handler_data(desc);
>>>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
>>>> - struct irq_chip *chip = irq_desc_get_chip(desc);
>>>> - unsigned long reg;
>>>> - u32 val, bit, virq;
>>>> + u32 val, virq;
>>>> - /* INT for debug */
>>>> val = readl(priv->base + PCL_RCV_INT);
>>>> if (val & PCL_CFG_BW_MGT_STATUS)
>>>> dev_dbg(pci->dev, "Link Bandwidth Management Event\n");
>>>> +
>>>> if (val & PCL_CFG_LINK_AUTO_BW_STATUS)
>>>> dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n");
>>>> - if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>> - dev_dbg(pci->dev, "Root Error\n");
>>>> - if (val & PCL_CFG_PME_MSI_STATUS)
>>>> - dev_dbg(pci->dev, "PME Interrupt\n");
>>>> +
>>>> + if (is_msi) {
>>>> + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
>>>> + dev_dbg(pci->dev, "Root Error Status\n");
>>>> +
>>>> + if (val & PCL_CFG_PME_MSI_STATUS)
>>>> + dev_dbg(pci->dev, "PME Interrupt\n");
>>>> +
>>>> + if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
>>>> + PCL_CFG_PME_MSI_STATUS)) {
>>>> + virq = irq_linear_revmap(pp->irq_domain, 0);
>>>> + generic_handle_irq(virq);
>>>> + }
>>>> + }
>>>
>>> Please have two handlers: one for interrupts that are from the RC,
>>> another for interrupts coming from the endpoints.
>> I assume that this handler treats interrupts from the RC only and
>> this is set on the member ".msi_host_isr" added in the patch 1/6.
>> I think that the handler for interrupts coming from endpoints should be
>> treated as a normal case (after calling .msi_host_isr in
>> dw_handle_msi_irq()).
>
> It looks pretty odd that you end-up dealing with both from the
> same "parent" interrupt. I guess this is in keeping with the
> rest of the DW PCIe hacks... :-/
It might be odd, however, in case of UniPhier SoC,
both MSI interrupts from endpoints and PME/AER interrupts from RC are
asserted by same "parent" interrupt. In other words, PME/AER interrupts
are notified using the parent interrupt for MSI.
MSI interrupts are treated as child interrupts with reference to
the status register in DW core. This is handled in a for-loop in
dw_handle_msi_irq().
PME/AER interrupts are treated with reference to the status register
in UniPhier glue layer, however, this couldn't be handled in the same way
directly.
So I'm trying to add .msi_host_isr function to handle this
with reference to the SoC-specific registers.
This exported function asserts MSI-0 as a shared child interrupt.
As a result, PME/AER are registered like the followings in dmesg:
pcieport 0000:00:00.0: PME: Signaling with IRQ 25
pcieport 0000:00:00.0: AER: enabled with IRQ 25
And these interrupts are shared as MSI-0:
# cat /proc/interrupts | grep 25:
25: 0 0 0 0 PCI-MSI 0 Edge PCIe PME, aerdrv
This might be a special case, though, I think that this is needed to handle
interrupts from RC sharing MSI parent.
> It is for Lorenzo to make up his mind about this anyway.
I'd like to Lorenzo's opinion, too.
Thank you,
---
Best Regards
Kunihiko Hayashi
Powered by blists - more mailing lists