* [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
@ 2020-09-26 16:09 marek.vasut
2020-09-27 8:29 ` Sergei Shtylyov
2020-09-28 9:35 ` Geert Uytterhoeven
0 siblings, 2 replies; 9+ messages in thread
From: marek.vasut @ 2020-09-26 16:09 UTC (permalink / raw)
To: linux-pci
Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven,
Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda,
linux-renesas-soc
From: Marek Vasut <marek.vasut+renesas@gmail.com>
The R-Car PCIe controller is capable of handling L0s/L1 link states.
While the controller can enter and exit L0s link state, and exit L1
link state, without any additional action from the driver, to enter
L1 link state, the driver must complete the link state transition by
issuing additional commands to the controller.
The problem is, this transition is not atomic. The controller sets
PMEL1RX bit in PMSR register upon reception of PM_ENTER_L1 DLLP from
the PCIe card, but then the controller enters some sort of inbetween
state. The driver must detect this condition and complete the link
state transition, by setting L1IATN bit in PMCTLR and waiting for
the link state transition to complete.
If a PCIe access happens inside of this window, where the controller
is inbetween L0 and L1 link states, the access generates a fault and
the ARM 'imprecise external abort' handler is invoked.
Just like other PCI controller drivers, here we hook the fault handler,
perform the fixup to help the controller enter L1 link state, and then
restart the instruction which triggered the fault. Since the controller
is in L1 link state now, the link can exit from L1 link state to L0 and
successfully complete the access.
Note that this fixup is applicable only to Aarch32 R-Car controllers,
the Aarch64 R-Car perform the same fixup in TFA, see TFA commit [1]
0969397f2 ("rcar_gen3: plat: Prevent PCIe hang during L1X config access")
[1] https://github.com/ARM-software/arm-trusted-firmware/commit/0969397f295621aa26b3d14b76dd397d22be58bf
Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-renesas-soc@vger.kernel.org
---
V2: - Update commit message, add link to TFA repository commit
- Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
- Cache clock and check whether they are enabled before register
access
---
drivers/pci/controller/pcie-rcar-host.c | 65 +++++++++++++++++++++++++
drivers/pci/controller/pcie-rcar.h | 7 +++
2 files changed, 72 insertions(+)
diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c
index cdc0963f154e..f8ff8c55716a 100644
--- a/drivers/pci/controller/pcie-rcar-host.c
+++ b/drivers/pci/controller/pcie-rcar-host.c
@@ -13,6 +13,7 @@
#include <linux/bitops.h>
#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
@@ -42,6 +43,21 @@ struct rcar_msi {
int irq2;
};
+#ifdef CONFIG_ARM
+/*
+ * Here we keep a static copy of the remapped PCIe controller address.
+ * This is only used on aarch32 systems, all of which have one single
+ * PCIe controller, to provide quick access to the PCIe controller in
+ * the L1 link state fixup function, called from ARM fault handler.
+ */
+static void __iomem *pcie_base;
+/*
+ * Static copy of bus clock pointer, so we can check whether the clock
+ * are enabled or not.
+ */
+static struct clk *pcie_bus_clk;
+#endif
+
static inline struct rcar_msi *to_rcar_msi(struct msi_controller *chip)
{
return container_of(chip, struct rcar_msi, chip);
@@ -804,6 +820,12 @@ static int rcar_pcie_get_resources(struct rcar_pcie_host *host)
}
host->msi.irq2 = i;
+#ifdef CONFIG_ARM
+ /* Cache static copy for L1 link state fixup hook on aarch32 */
+ pcie_base = pcie->base;
+ pcie_bus_clk = host->bus_clk;
+#endif
+
return 0;
err_irq2:
@@ -1050,4 +1072,47 @@ static struct platform_driver rcar_pcie_driver = {
},
.probe = rcar_pcie_probe,
};
+
+#ifdef CONFIG_ARM
+static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
+ unsigned int fsr, struct pt_regs *regs)
+{
+ u32 pmsr;
+
+ if (!pcie_base || !pcie_bus_clk || !__clk_is_enabled(pcie_bus_clk))
+ return 1;
+
+ pmsr = readl(pcie_base + PMSR);
+
+ /*
+ * Test if the PCIe controller received PM_ENTER_L1 DLLP and
+ * the PCIe controller is not in L1 link state. If true, apply
+ * fix, which will put the controller into L1 link state, from
+ * which it can return to L0s/L0 on its own.
+ */
+ if ((pmsr & PMEL1RX) && ((pmsr & PMSTATE) != PMSTATE_L1)) {
+ writel(L1IATN, pcie_base + PMCTLR);
+ while (!(readl(pcie_base + PMSR) & L1FAEG))
+ ;
+ writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
+ return 0;
+ }
+
+ return 1;
+}
+
+static int __init rcar_pcie_init(void)
+{
+#ifdef CONFIG_ARM_LPAE
+ hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
+ "asynchronous external abort");
+#else
+ hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
+ "imprecise external abort");
+#endif
+ return platform_driver_register(&rcar_pcie_driver);
+}
+device_initcall(rcar_pcie_init);
+#else
builtin_platform_driver(rcar_pcie_driver);
+#endif
diff --git a/drivers/pci/controller/pcie-rcar.h b/drivers/pci/controller/pcie-rcar.h
index d4c698b5f821..9bb125db85c6 100644
--- a/drivers/pci/controller/pcie-rcar.h
+++ b/drivers/pci/controller/pcie-rcar.h
@@ -85,6 +85,13 @@
#define LTSMDIS BIT(31)
#define MACCTLR_INIT_VAL (LTSMDIS | MACCTLR_NFTS_MASK)
#define PMSR 0x01105c
+#define L1FAEG BIT(31)
+#define PMEL1RX BIT(23)
+#define PMSTATE GENMASK(18, 16)
+#define PMSTATE_L1 (3 << 16)
+#define PMCTLR 0x011060
+#define L1IATN BIT(31)
+
#define MACS2R 0x011078
#define MACCGSPSETR 0x011084
#define SPCNGRSN BIT(31)
--
2.28.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-09-26 16:09 [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook marek.vasut
@ 2020-09-27 8:29 ` Sergei Shtylyov
2020-10-04 14:10 ` Marek Vasut
2020-09-28 9:35 ` Geert Uytterhoeven
1 sibling, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2020-09-27 8:29 UTC (permalink / raw)
To: marek.vasut, linux-pci
Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven,
Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda,
linux-renesas-soc
Hello!
On 26.09.2020 19:09, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> The R-Car PCIe controller is capable of handling L0s/L1 link states.
> While the controller can enter and exit L0s link state, and exit L1
> link state, without any additional action from the driver, to enter
> L1 link state, the driver must complete the link state transition by
> issuing additional commands to the controller.
>
> The problem is, this transition is not atomic. The controller sets
> PMEL1RX bit in PMSR register upon reception of PM_ENTER_L1 DLLP from
> the PCIe card, but then the controller enters some sort of inbetween
> state. The driver must detect this condition and complete the link
> state transition, by setting L1IATN bit in PMCTLR and waiting for
> the link state transition to complete.
>
> If a PCIe access happens inside of this window, where the controller
s/of//.
> is inbetween L0 and L1 link states, the access generates a fault and
My spellchecker trips on "inbetween"...
> the ARM 'imprecise external abort' handler is invoked.
>
> Just like other PCI controller drivers, here we hook the fault handler,
> perform the fixup to help the controller enter L1 link state, and then
> restart the instruction which triggered the fault. Since the controller
If this is imprecise or async external abort, how we can re-execute the
instruction that triggered the fault? It's been probably executed already, no?
> is in L1 link state now, the link can exit from L1 link state to L0 and
> successfully complete the access.
>
> Note that this fixup is applicable only to Aarch32 R-Car controllers,
> the Aarch64 R-Car perform the same fixup in TFA, see TFA commit [1]
> 0969397f2 ("rcar_gen3: plat: Prevent PCIe hang during L1X config access")
> [1] https://github.com/ARM-software/arm-trusted-firmware/commit/0969397f295621aa26b3d14b76dd397d22be58bf
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org
[...]
> +static int __init rcar_pcie_init(void)
> +{
> +#ifdef CONFIG_ARM_LPAE
> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> + "asynchronous external abort");
> +#else
> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> + "imprecise external abort");
> +#endif
> + return platform_driver_register(&rcar_pcie_driver);
> +}
> +device_initcall(rcar_pcie_init);
> +#else
> builtin_platform_driver(rcar_pcie_driver);
> +#endif
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-09-26 16:09 [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook marek.vasut
2020-09-27 8:29 ` Sergei Shtylyov
@ 2020-09-28 9:35 ` Geert Uytterhoeven
2020-10-04 14:14 ` Marek Vasut
1 sibling, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-09-28 9:35 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pci, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi,
Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas
Hi Marek,
On Sat, Sep 26, 2020 at 6:09 PM <marek.vasut@gmail.com> wrote:
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> The R-Car PCIe controller is capable of handling L0s/L1 link states.
> While the controller can enter and exit L0s link state, and exit L1
> link state, without any additional action from the driver, to enter
> L1 link state, the driver must complete the link state transition by
> issuing additional commands to the controller.
>
> The problem is, this transition is not atomic. The controller sets
> PMEL1RX bit in PMSR register upon reception of PM_ENTER_L1 DLLP from
> the PCIe card, but then the controller enters some sort of inbetween
> state. The driver must detect this condition and complete the link
> state transition, by setting L1IATN bit in PMCTLR and waiting for
> the link state transition to complete.
>
> If a PCIe access happens inside of this window, where the controller
> is inbetween L0 and L1 link states, the access generates a fault and
> the ARM 'imprecise external abort' handler is invoked.
>
> Just like other PCI controller drivers, here we hook the fault handler,
> perform the fixup to help the controller enter L1 link state, and then
> restart the instruction which triggered the fault. Since the controller
> is in L1 link state now, the link can exit from L1 link state to L0 and
> successfully complete the access.
>
> Note that this fixup is applicable only to Aarch32 R-Car controllers,
> the Aarch64 R-Car perform the same fixup in TFA, see TFA commit [1]
> 0969397f2 ("rcar_gen3: plat: Prevent PCIe hang during L1X config access")
> [1] https://github.com/ARM-software/arm-trusted-firmware/commit/0969397f295621aa26b3d14b76dd397d22be58bf
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> V2: - Update commit message, add link to TFA repository commit
> - Handle the LPAE case as in ARM fault.c and fsr-{2,3}level.c
> - Cache clock and check whether they are enabled before register
> access
Thanks for the update!
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -13,6 +13,7 @@
>
> #include <linux/bitops.h>
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> @@ -42,6 +43,21 @@ struct rcar_msi {
> int irq2;
> };
>
> +#ifdef CONFIG_ARM
> +/*
> + * Here we keep a static copy of the remapped PCIe controller address.
> + * This is only used on aarch32 systems, all of which have one single
> + * PCIe controller, to provide quick access to the PCIe controller in
> + * the L1 link state fixup function, called from ARM fault handler.
the ARM fault handler
> + */
> +static void __iomem *pcie_base;
> +/*
> + * Static copy of bus clock pointer, so we can check whether the clock
> + * are enabled or not.
is
> + */
> +static struct clk *pcie_bus_clk;
> +#endif
> @@ -1050,4 +1072,47 @@ static struct platform_driver rcar_pcie_driver = {
> },
> .probe = rcar_pcie_probe,
> };
> +
> +#ifdef CONFIG_ARM
> +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> + unsigned int fsr, struct pt_regs *regs)
> +{
> + u32 pmsr;
> +
> + if (!pcie_base || !pcie_bus_clk || !__clk_is_enabled(pcie_bus_clk))
As __clk_is_enabled() handles a NULL pointer parameter, the second
test is not needed.
> + return 1;
> +
> + pmsr = readl(pcie_base + PMSR);
> +
> + /*
> + * Test if the PCIe controller received PM_ENTER_L1 DLLP and
> + * the PCIe controller is not in L1 link state. If true, apply
> + * fix, which will put the controller into L1 link state, from
> + * which it can return to L0s/L0 on its own.
> + */
> + if ((pmsr & PMEL1RX) && ((pmsr & PMSTATE) != PMSTATE_L1)) {
> + writel(L1IATN, pcie_base + PMCTLR);
> + while (!(readl(pcie_base + PMSR) & L1FAEG))
> + ;
To answer my own question from previous review cycle: this takes 7
cycles on r8a7791/koelsch.
> + writel(L1FAEG | PMEL1RX, pcie_base + PMSR);
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +static int __init rcar_pcie_init(void)
> +{
> +#ifdef CONFIG_ARM_LPAE
> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> + "asynchronous external abort");
> +#else
> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> + "imprecise external abort");
> +#endif
As there can be only a single handler, this may interfere with a handler
for another platform in a multi-platform kernel.
Hence I think this should not be done unconditionally, but be moved to
the driver's .probe() callback.
> + return platform_driver_register(&rcar_pcie_driver);
> +}
> +device_initcall(rcar_pcie_init);
> +#else
> builtin_platform_driver(rcar_pcie_driver);
> +#endif
As this fixes the crash seen during s2ram with an Intel E1000E card
present and the e1000e driver loaded:
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-09-27 8:29 ` Sergei Shtylyov
@ 2020-10-04 14:10 ` Marek Vasut
0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2020-10-04 14:10 UTC (permalink / raw)
To: Sergei Shtylyov, linux-pci
Cc: Bjorn Helgaas, Geert Uytterhoeven, Lorenzo Pieralisi,
Wolfram Sang, Yoshihiro Shimoda, linux-renesas-soc
On 9/27/20 10:29 AM, Sergei Shtylyov wrote:
[...]
>> the ARM 'imprecise external abort' handler is invoked.
>>
>> Just like other PCI controller drivers, here we hook the fault handler,
>> perform the fixup to help the controller enter L1 link state, and then
>> restart the instruction which triggered the fault. Since the controller
>
> If this is imprecise or async external abort, how we can re-execute
> the instruction that triggered the fault? It's been probably executed
> already, no?
It has been executed and triggered the fault, because it could not
access the data across the bus. Now the bus is back in operational
state, so restart the instruction, let it access the data and do its task.
[...]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-09-28 9:35 ` Geert Uytterhoeven
@ 2020-10-04 14:14 ` Marek Vasut
2020-10-05 7:31 ` Geert Uytterhoeven
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2020-10-04 14:14 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-pci, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi,
Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas
On 9/28/20 11:35 AM, Geert Uytterhoeven wrote:
[...]
>> +static int __init rcar_pcie_init(void)
>> +{
>> +#ifdef CONFIG_ARM_LPAE
>> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>> + "asynchronous external abort");
>> +#else
>> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>> + "imprecise external abort");
>> +#endif
>
> As there can be only a single handler, this may interfere with a handler
> for another platform in a multi-platform kernel.
> Hence I think this should not be done unconditionally, but be moved to
> the driver's .probe() callback.
Why is nobody doing this in the probe code then ? It seems all the other
drivers which hook fault code do it in init as well. I can imagine that
something might trip the fault handler even before probe is called, e.g.
some PM handling or simply user accessing that PCIe area using setpci.
[...]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-10-04 14:14 ` Marek Vasut
@ 2020-10-05 7:31 ` Geert Uytterhoeven
2020-10-05 8:00 ` Marek Vasut
2020-10-07 18:29 ` Rob Herring
0 siblings, 2 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-10-05 7:31 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pci, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi,
Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas
Hi Marek,
On Sun, Oct 4, 2020 at 4:16 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 9/28/20 11:35 AM, Geert Uytterhoeven wrote:
> [...]
> >> +static int __init rcar_pcie_init(void)
> >> +{
> >> +#ifdef CONFIG_ARM_LPAE
> >> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> >> + "asynchronous external abort");
> >> +#else
> >> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> >> + "imprecise external abort");
> >> +#endif
> >
> > As there can be only a single handler, this may interfere with a handler
> > for another platform in a multi-platform kernel.
> > Hence I think this should not be done unconditionally, but be moved to
> > the driver's .probe() callback.
>
> Why is nobody doing this in the probe code then ? It seems all the other
drivers/pci/controller/dwc/pci-keystone.c is:
ks_pcie_probe()
ks_pcie_add_pcie_port()
dw_pcie_host_init()
pp->ops->host_init(pp) = ks_pcie_host_init()
hook_fault_code()
> drivers which hook fault code do it in init as well. I can imagine that
Probably nobody bothered exercising the external abort handler on
multi-platform kernels?
> something might trip the fault handler even before probe is called, e.g.
> some PM handling or simply user accessing that PCIe area using setpci.
If that is the case, it must indeed by done earlier, but still
conditional on the presence of the actual PCIe controller.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-10-05 7:31 ` Geert Uytterhoeven
@ 2020-10-05 8:00 ` Marek Vasut
2020-10-05 8:07 ` Geert Uytterhoeven
2020-10-07 18:29 ` Rob Herring
1 sibling, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2020-10-05 8:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Wolfram Sang,
Yoshihiro Shimoda, Linux-Renesas, m-karicheri2
On 10/5/20 9:31 AM, Geert Uytterhoeven wrote:
> Hi Marek,
Hi,
> On Sun, Oct 4, 2020 at 4:16 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 9/28/20 11:35 AM, Geert Uytterhoeven wrote:
>> [...]
>>>> +static int __init rcar_pcie_init(void)
>>>> +{
>>>> +#ifdef CONFIG_ARM_LPAE
>>>> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>>>> + "asynchronous external abort");
>>>> +#else
>>>> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
>>>> + "imprecise external abort");
>>>> +#endif
>>>
>>> As there can be only a single handler, this may interfere with a handler
>>> for another platform in a multi-platform kernel.
>>> Hence I think this should not be done unconditionally, but be moved to
>>> the driver's .probe() callback.
>>
>> Why is nobody doing this in the probe code then ? It seems all the other
>
> drivers/pci/controller/dwc/pci-keystone.c is:
>
> ks_pcie_probe()
> ks_pcie_add_pcie_port()
> dw_pcie_host_init()
> pp->ops->host_init(pp) = ks_pcie_host_init()
> hook_fault_code()
Well that one is interesting. I wonder whether that driver has the same
LPAE bug (different fault code for LPAE and non-LPAE configuration) we
found here too, since it is used on CA15 TI SoCs.
>> drivers which hook fault code do it in init as well. I can imagine that
>
> Probably nobody bothered exercising the external abort handler on
> multi-platform kernels?
>
>> something might trip the fault handler even before probe is called, e.g.
>> some PM handling or simply user accessing that PCIe area using setpci.
>
> If that is the case, it must indeed by done earlier, but still
> conditional on the presence of the actual PCIe controller.
I am open to suggestions how to do that part.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-10-05 8:00 ` Marek Vasut
@ 2020-10-05 8:07 ` Geert Uytterhoeven
0 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-10-05 8:07 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Wolfram Sang,
Yoshihiro Shimoda, Linux-Renesas, Murali Karicheri
Hi Marek,
On Mon, Oct 5, 2020 at 10:00 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 10/5/20 9:31 AM, Geert Uytterhoeven wrote:
> > On Sun, Oct 4, 2020 at 4:16 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 9/28/20 11:35 AM, Geert Uytterhoeven wrote:
> >> [...]
> >>>> +static int __init rcar_pcie_init(void)
> >>>> +{
> >>>> +#ifdef CONFIG_ARM_LPAE
> >>>> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> >>>> + "asynchronous external abort");
> >>>> +#else
> >>>> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> >>>> + "imprecise external abort");
> >>>> +#endif
> >>>
> >>> As there can be only a single handler, this may interfere with a handler
> >>> for another platform in a multi-platform kernel.
> >>> Hence I think this should not be done unconditionally, but be moved to
> >>> the driver's .probe() callback.
> >>
> >> Why is nobody doing this in the probe code then ? It seems all the other
> >
> > drivers/pci/controller/dwc/pci-keystone.c is:
> >
> > ks_pcie_probe()
> > ks_pcie_add_pcie_port()
> > dw_pcie_host_init()
> > pp->ops->host_init(pp) = ks_pcie_host_init()
> > hook_fault_code()
>
> Well that one is interesting. I wonder whether that driver has the same
> LPAE bug (different fault code for LPAE and non-LPAE configuration) we
> found here too, since it is used on CA15 TI SoCs.
Isn't that an ARM "feature"?
arch/arm/mm/fault.c-/* FSR definition */
arch/arm/mm/fault.c:#ifdef CONFIG_ARM_LPAE
arch/arm/mm/fault.c-#include "fsr-3level.c"
arch/arm/mm/fault.c-#else
arch/arm/mm/fault.c-#include "fsr-2level.c"
arch/arm/mm/fault.c-#endif
> >> drivers which hook fault code do it in init as well. I can imagine that
> >
> > Probably nobody bothered exercising the external abort handler on
> > multi-platform kernels?
> >
> >> something might trip the fault handler even before probe is called, e.g.
> >> some PM handling or simply user accessing that PCIe area using setpci.
> >
> > If that is the case, it must indeed by done earlier, but still
> > conditional on the presence of the actual PCIe controller.
>
> I am open to suggestions how to do that part.
if (of_find_matching_node(...))
do_the_right_stuff();
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook
2020-10-05 7:31 ` Geert Uytterhoeven
2020-10-05 8:00 ` Marek Vasut
@ 2020-10-07 18:29 ` Rob Herring
1 sibling, 0 replies; 9+ messages in thread
From: Rob Herring @ 2020-10-07 18:29 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Marek Vasut, linux-pci, Marek Vasut, Bjorn Helgaas,
Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda,
Linux-Renesas
On Mon, Oct 05, 2020 at 09:31:54AM +0200, Geert Uytterhoeven wrote:
> Hi Marek,
>
> On Sun, Oct 4, 2020 at 4:16 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> > On 9/28/20 11:35 AM, Geert Uytterhoeven wrote:
> > [...]
> > >> +static int __init rcar_pcie_init(void)
> > >> +{
> > >> +#ifdef CONFIG_ARM_LPAE
> > >> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> > >> + "asynchronous external abort");
> > >> +#else
> > >> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> > >> + "imprecise external abort");
> > >> +#endif
> > >
> > > As there can be only a single handler, this may interfere with a handler
> > > for another platform in a multi-platform kernel.
> > > Hence I think this should not be done unconditionally, but be moved to
> > > the driver's .probe() callback.
> >
> > Why is nobody doing this in the probe code then ? It seems all the other
>
> drivers/pci/controller/dwc/pci-keystone.c is:
>
> ks_pcie_probe()
> ks_pcie_add_pcie_port()
> dw_pcie_host_init()
> pp->ops->host_init(pp) = ks_pcie_host_init()
> hook_fault_code()
Looks broken in deferred probe case as hook_fault_code is __init.
Really, hook_fault_code needs to be exported so these drivers can be
modules. Or we split out all the abort handlers to a separate broken,
aborting PCI hosts module.
> > drivers which hook fault code do it in init as well. I can imagine that
>
> Probably nobody bothered exercising the external abort handler on
> multi-platform kernels?
>
> > something might trip the fault handler even before probe is called, e.g.
> > some PM handling or simply user accessing that PCIe area using setpci.
I don't see how that's possible. You'd first hit translation faults as
nothing is mapped.
> If that is the case, it must indeed by done earlier, but still
> conditional on the presence of the actual PCIe controller.
imx6 should be conditional too.
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-07 18:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-26 16:09 [PATCH V2] PCI: rcar: Add L1 link state fix into data abort hook marek.vasut
2020-09-27 8:29 ` Sergei Shtylyov
2020-10-04 14:10 ` Marek Vasut
2020-09-28 9:35 ` Geert Uytterhoeven
2020-10-04 14:14 ` Marek Vasut
2020-10-05 7:31 ` Geert Uytterhoeven
2020-10-05 8:00 ` Marek Vasut
2020-10-05 8:07 ` Geert Uytterhoeven
2020-10-07 18:29 ` Rob Herring
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).