PCI: rcar: Finish transition to L1 state in rcar_pcie_config_access()

From: Marek Vasut <[email protected]>

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

The current asynchronous external abort hook implementation restarts the instruction which finally triggered the fault, which can be a different instruction than the read/write instruction which started the faulting access. Usually the instruction which finally triggers the fault is one which has some data dependency on the result of the read/write. In case of read, the read value after fixup is undefined, while a read value of faulting read should be all Fs.

It is possible to enforce the fault using 'isb' instruction placed right after the read/write instruction which started the faulting access. Add custom register accessors which perform the read/write followed immediately by 'isb'.

This way, the fault always happens on the 'isb' and in case of read, which is located one instruction before the 'isb', it is now possible to fix up the return value of the read in the asynchronous external abort hook and make that read return all Fs.

Signed-off-by Marek Vasut <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc Krzysztof Wilczyński <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Yoshihiro Shimoda <[email protected]>

Cc [email protected]

drivers/pci/controller/pcie-rcar-host.c +5 -0

+#ifdef CONFIG_ARM
+#define __rcar_pci_rw_reg_workaround(instr)                            \
+               "1:     " instr " %1, [%2]\n"                           \
+               "2:     isb\n"                                          \
+               "3:     .pushsection .text.fixup,\"ax\"\n"              \

Quoted 12 lines

drivers/pci/controller/pcie-rcar-host.c +12 -0

+#ifdef CONFIG_ARM
+#define __rcar_pci_rw_reg_workaround(instr)                            \
+               "1:     " instr " %1, [%2]\n"                           \
+               "2:     isb\n"                                          \
+               "3:     .pushsection .text.fixup,\"ax\"\n"              \
+               "       .align  2\n"                                    \
+               "4:     mov     %0, #" __stringify(PCIBIOS_SET_FAILED) "\n" \
+               "       b       3b\n"                                   \
+               "       .popsection\n"                                  \
+               "       .pushsection __ex_table,\"a\"\n"                \
+               "       .align  3\n"                                    \
+               "       .long   1b, 4b\n"                               \

drivers/pci/controller/pcie-rcar-host.c static u32 rcar_read_conf(struct rcar_pcie *pcie, int where) · line 131

Quoted 3 lines

drivers/pci/controller/pcie-rcar-host.c +3 -0

+               "       .long   1b, 4b\n"                               \
+               "       .popsection\n"
+#endif

You list the fixup for the ldr/str instruction here twice, (.long 1b,4b), but no fixup for the isb instruction (.long 2b, 4b). Your description says that the fault happens on the isb, not the ldr, so I don't understand what is going on here.

From: Marek Vasut <[email protected]>

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

The current asynchronous external abort hook implementation restarts the instruction which finally triggered the fault, which can be a different instruction than the read/write instruction which started the faulting access. Usually the instruction which finally triggers the fault is one which has some data dependency on the result of the read/write. In case of read, the read value after fixup is undefined, while a read value of faulting read should be all Fs.

It is possible to enforce the fault using 'isb' instruction placed right after the read/write instruction which started the faulting access. Add custom register accessors which perform the read/write followed immediately by 'isb'.

This way, the fault always happens on the 'isb' and in case of read, which is located one instruction before the 'isb', it is now possible to fix up the return value of the read in the asynchronous external abort hook and make that read return all Fs.

Signed-off-by Marek Vasut <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc Krzysztof Wilczyński <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Yoshihiro Shimoda <[email protected]>

Cc [email protected]

---
V2: Rebase on 1/2
V3: - Add .text.fixup on all three ldr/str/isb instructions and call
fixup_exception() in the abort handler to trigger the fixup.
- Propagate return value from read/write accessors, in case the access fails, return PCIBIOS_SET_FAILED, else PCIBIOS_SUCCESSFUL.

---
 drivers/pci/controller/pcie-rcar-host.c | 53 +++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 4 deletions(-)

drivers/pci/controller/pcie-rcar-host.c +49 -4

index 7d38a9c50093..b2e521ee95eb 100644
@@ -114,6 +114,51 @@ static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 	return val >> shift;
 }
 
+#ifdef CONFIG_ARM
+#define __rcar_pci_rw_reg_workaround(instr)				\
+		"1:	" instr " %1, [%2]\n"				\
+		"2:	isb\n"						\
+		"3:	.pushsection .text.fixup,\"ax\"\n"		\
+		"	.align	2\n"					\
+		"4:	mov	%0, #" __stringify(PCIBIOS_SET_FAILED) "\n" \
+		"	b	3b\n"					\
+		"	.popsection\n"					\
+		"	.pushsection __ex_table,\"a\"\n"		\
+		"	.align	3\n"					\
+		"	.long	1b, 4b\n"				\
+		"	.long	1b, 4b\n"				\
+		"	.popsection\n"
+#endif
+
+int rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
+{
+	int error = PCIBIOS_SUCCESSFUL;
+#ifdef CONFIG_ARM
+	asm volatile(
+		__rcar_pci_rw_reg_workaround("str")
+	: "+r"(error):"r"(val), "r"(pcie->base + reg) : "memory");
+#else
+	rcar_pci_write_reg(pcie, val, reg);
+#endif
+	return error;
+}
+
+int rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, u32 *val, unsigned int reg)
+{
+	int error = PCIBIOS_SUCCESSFUL;
+#ifdef CONFIG_ARM
+	asm volatile(
+		__rcar_pci_rw_reg_workaround("ldr")
+	: "+r"(error), "=r"(*val) : "r"(pcie->base + reg) : "memory");
+
+	if (error != PCIBIOS_SUCCESSFUL)
+		*val = 0xffffffff;
+#else
+	*val = rcar_pci_read_reg(pcie, reg);
+#endif
+	return error;
+}
+
 /* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
 static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		unsigned char access_type, struct pci_bus *bus,

@@ -185,14 +230,14 @@ static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (access_type == RCAR_PCI_ACCESS_READ)
-		*data = rcar_pci_read_reg(pcie, PCIECDR);
+		ret = rcar_pci_read_reg_workaround(pcie, data, PCIECDR);
 	else
-		rcar_pci_write_reg(pcie, *data, PCIECDR);
+		ret = rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
 
 	/* Disable the configuration access */
 	rcar_pci_write_reg(pcie, 0, PCIECCTLR);
 
-	return PCIBIOS_SUCCESSFUL;
+	return ret;
 }
 
 static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,

@@ -1097,7 +1142,7 @@ static struct platform_driver rcar_pcie_driver = {
 static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
-	return !!rcar_pcie_wakeup(pcie_dev, pcie_base);
+	return !fixup_exception(regs);
 }
 
 static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst = {

Signature · 1 line

2.34.1

Quoted 6 lines · On Saturday 22 January 2022 23:15:54 [email protected] wrote:

From: Marek Vasut <[email protected]>

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

Hello!

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

It is really important to know this fact.

I'm in impression that this patch still is not enough as similar issues are also in other PCIe controllers which I know...

Quoted 17 lines · Pali Rohár

Quoted 6 lines · On Saturday 22 January 2022 23:15:54 [email protected] wrote:

From: Marek Vasut <[email protected]>

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

Hello!

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

Do you have some specific test which fails ?

This patch addresses the case where the link transition to L1 state has to be completed manually. If the CPU accesses the config space before that happened, you get an imprecise data abort.

Quoted 4 lines

It is really important to know this fact.

I'm in impression that this patch still is not enough as similar issues are also in other PCIe controllers which I know...

Do you have a suggestion for a patch which would be enough on this hardware ?

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Quoted 3 lines

This patch addresses the case where the link transition to L1 state has to be completed manually. If the CPU accesses the config space before that happened, you get an imprecise data abort.

Yes, I see. But it does not have to complete and the question how is handled this case... And that is why is needed to know what happens in such cases.

And IIRC you cannot go from L1 state directly to L0, but only via Recovery state. And from Recovery you may end up in Detect state. (e.g. after hot unplug or if some buggy card with kernel quirk is used)

Quoted 2 lines

Quoted 2 lines

It is really important to know this fact.

Quoted 5 lines

Quoted 2 lines

I'm in impression that this patch still is not enough as similar issues are also in other PCIe controllers which I know...

Do you have a suggestion for a patch which would be enough on this hardware ?

I do not have enough information.

On 1/23/22 17:49, Pali Rohár wrote:

Hi,

[...]

Quoted 35 lines

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Quoted 3 lines

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Can you give me the exact setpci invocation ? If so, then I can test this for you on the hardware.

Quoted 41 lines · On Monday 24 January 2022 06:46:47 Marek Vasut wrote:

On 1/23/22 17:49, Pali Rohár wrote:

Hi,

[...]

Quoted 35 lines

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Quoted 6 lines

Quoted 3 lines

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Can you give me the exact setpci invocation ? If so, then I can test this for you on the hardware.

Call "setpci -s $bdf_root_port BRIDGE_CONTROL" with address of the PCIe Root Port device (parent of selected device). This will print value of bridge control register. Logical OR it with value 0x20 (Secondary Bus Reset Bit) and call "setpci -s $bdf_root_port BRIDGE_CONTROL=$new_value". After this call is link in the Hot Reset state and you can do any test. To bring link back, call setpci again with cleared 0x20 bit mask.

Similar test you can done also with setting Link Disable bit (bit 4) in PCIe Link Control register. Offset to this register is not static and you can figure it out from lspci -s $bdf_root_port -vv output. Retrain Link is bit 5 in the same register.

Quoted 56 lines · Pali Rohár

Quoted 41 lines · On Monday 24 January 2022 06:46:47 Marek Vasut wrote:

On 1/23/22 17:49, Pali Rohár wrote:

Hi,

[...]

Quoted 35 lines

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Quoted 6 lines

Quoted 3 lines

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Can you give me the exact setpci invocation ? If so, then I can test this for you on the hardware.

Call "setpci -s $bdf_root_port BRIDGE_CONTROL" with address of the PCIe Root Port device (parent of selected device). This will print value of bridge control register. Logical OR it with value 0x20 (Secondary Bus Reset Bit) and call "setpci -s $bdf_root_port BRIDGE_CONTROL=$new_value". After this call is link in the Hot Reset state and you can do any test. To bring link back, call setpci again with cleared 0x20 bit mask.

Quoted 4 lines

Similar test you can done also with setting Link Disable bit (bit 4) in PCIe Link Control register. Offset to this register is not static and you can figure it out from lspci -s $bdf_root_port -vv output. Retrain Link is bit 5 in the same register.

Flipping either bit makes no difference, suspend/resume behaves the same and the link always recovers.

Quoted 62 lines · On Saturday 29 January 2022 05:39:40 Marek Vasut wrote:

Quoted 60 lines · Pali Rohár

Quoted 41 lines · On Monday 24 January 2022 06:46:47 Marek Vasut wrote:

On 1/23/22 17:49, Pali Rohár wrote:

Hi,

[...]

Quoted 35 lines

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Quoted 6 lines

Quoted 3 lines

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Can you give me the exact setpci invocation ? If so, then I can test this for you on the hardware.

Call "setpci -s $bdf_root_port BRIDGE_CONTROL" with address of the PCIe Root Port device (parent of selected device). This will print value of bridge control register. Logical OR it with value 0x20 (Secondary Bus Reset Bit) and call "setpci -s $bdf_root_port BRIDGE_CONTROL=$new_value". After this call is link in the Hot Reset state and you can do any test. To bring link back, call setpci again with cleared 0x20 bit mask.

Similar test you can done also with setting Link Disable bit (bit 4) in PCIe Link Control register. Offset to this register is not static and you can figure it out from lspci -s $bdf_root_port -vv output. Retrain Link is bit 5 in the same register.

Quoted 2 lines

Flipping either bit makes no difference, suspend/resume behaves the same and the link always recovers.

Ok, perfect! And what happens without suspend/resume (just in normal conditions)? E.g. during active usage of some PCIe card (wifi, sata, etc..).

Quoted 63 lines · On Monday 31 January 2022 13:53:41 Pali Rohár wrote:

Quoted 62 lines · On Saturday 29 January 2022 05:39:40 Marek Vasut wrote:

Quoted 60 lines · Pali Rohár

Quoted 41 lines · On Monday 24 January 2022 06:46:47 Marek Vasut wrote:

On 1/23/22 17:49, Pali Rohár wrote:

Hi,

[...]

Quoted 35 lines

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Quoted 6 lines

Quoted 3 lines

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Can you give me the exact setpci invocation ? If so, then I can test this for you on the hardware.

Call "setpci -s $bdf_root_port BRIDGE_CONTROL" with address of the PCIe Root Port device (parent of selected device). This will print value of bridge control register. Logical OR it with value 0x20 (Secondary Bus Reset Bit) and call "setpci -s $bdf_root_port BRIDGE_CONTROL=$new_value". After this call is link in the Hot Reset state and you can do any test. To bring link back, call setpci again with cleared 0x20 bit mask.

Similar test you can done also with setting Link Disable bit (bit 4) in PCIe Link Control register. Offset to this register is not static and you can figure it out from lspci -s $bdf_root_port -vv output. Retrain Link is bit 5 in the same register.

Quoted 5 lines

Quoted 2 lines

Flipping either bit makes no difference, suspend/resume behaves the same and the link always recovers.

Ok, perfect! And what happens without suspend/resume (just in normal conditions)? E.g. during active usage of some PCIe card (wifi, sata, etc..).

PING? Also what lspci see for the root port and card itself during hot reset?

Quoted 67 lines · Pali Rohár

Quoted 66 lines · On Monday 31 January 2022 13:53:41 Pali Rohár wrote:

Quoted 64 lines · On Saturday 29 January 2022 05:39:40 Marek Vasut wrote:

Quoted 56 lines · Pali Rohár

Quoted 41 lines · On Monday 24 January 2022 06:46:47 Marek Vasut wrote:

On 1/23/22 17:49, Pali Rohár wrote:

Hi,

[...]

Quoted 35 lines

Quoted 7 lines

Quoted 7 lines

I must admit that this patch from its initial version evolved into giant hack...
https://lore.kernel.org/linux-pci/[email protected]/

During review of the previous patch I have asked some important questions but I have not got any answer to them. So I'm reminding it:
https://lore.kernel.org/linux-pci/20210805183024.ftdwknkttfwwogks@pali/

Quoted 5 lines

Quoted 3 lines

So could please answer what happens when PCIe controller is in some non-L* state and either MMIO happen or config read happens or config write happens?

What kind of non-L state ?

E.g. Hot Reset, Detect, Polling, Configuration or Recovery.

Quoted 1 line

Do you have some specific test which fails ?

Yes, by putting PCIe controller into one of those states. I have already wrote you in some previous email to trigger hot reset as this is the easiest test and can be done also by userspace (setpci).

Link goes to Recovery state automatically when doing link retraining (e.g. by setting RT bit in PCIe Root Port config space) and from Recovery to Configuration or directly back to L0. So testing this path needs precise timing and repeating it more times to trigger.

So the easiest test is really via PCIe Hot Reset by setting Secondary Bus Reset bit in Bridge Control register of PCIe Root Port. After this is link in Hot Reset and does not go back to L0 until you clear that bit. So in this state you can do all these operations which cause aborts, like calling that kernel function which is reading from config space which belongs to device on the other end of the PCIe link or doing MMIO read / write operation of mapped memory which again belongs to other end of PCIe link.

Quoted 6 lines

Quoted 3 lines

Or instead of Hot Reset, you can set link disable bit in config space of PCIe Root Port. Then link also would not be in L0 state (until you clear that bit), so again you have lot of time to do same tests.

Can you give me the exact setpci invocation ? If so, then I can test this for you on the hardware.

Call "setpci -s $bdf_root_port BRIDGE_CONTROL" with address of the PCIe Root Port device (parent of selected device). This will print value of bridge control register. Logical OR it with value 0x20 (Secondary Bus Reset Bit) and call "setpci -s $bdf_root_port BRIDGE_CONTROL=$new_value". After this call is link in the Hot Reset state and you can do any test. To bring link back, call setpci again with cleared 0x20 bit mask.

Quoted 4 lines

Similar test you can done also with setting Link Disable bit (bit 4) in PCIe Link Control register. Offset to this register is not static and you can figure it out from lspci -s $bdf_root_port -vv output. Retrain Link is bit 5 in the same register.

Flipping either bit makes no difference, suspend/resume behaves the same and the link always recovers.

Quoted 4 lines

Quoted 2 lines

Ok, perfect! And what happens without suspend/resume (just in normal conditions)? E.g. during active usage of some PCIe card (wifi, sata, etc..).

PING? Also what lspci see for the root port and card itself during hot reset?

If I recall, lspci showed the root port and card.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Flipping either bit makes no difference, suspend/resume behaves the same and the link always recovers.

Ok, perfect! And what happens without suspend/resume (just in normal conditions)? E.g. during active usage of some PCIe card (wifi, sata, etc..).

PING? Also what lspci see for the root port and card itself during hot reset?

Quoted 1 line

If I recall, lspci showed the root port and card.

This is suspicious. Card should not respond to config read requests when is in hot reset state. Could you send output of lspci -vvxx of the root port and also card during this test? Maybe it is possible that root port has broken BRIDGE_CONTROL register and did not put card into Hot Reset state?

On 2/17/22 14:04, Pali Rohár wrote:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Flipping either bit makes no difference, suspend/resume behaves the same and the link always recovers.

Ok, perfect! And what happens without suspend/resume (just in normal conditions)? E.g. during active usage of some PCIe card (wifi, sata, etc..).

PING? Also what lspci see for the root port and card itself during hot reset?

Quoted 7 lines

Quoted 1 line

If I recall, lspci showed the root port and card.

This is suspicious. Card should not respond to config read requests when is in hot reset state. Could you send output of lspci -vvxx of the root port and also card during this test? Maybe it is possible that root port has broken BRIDGE_CONTROL register and did not put card into Hot Reset state?

Yes, I could set the hardware up again and run more tests, it will take some time, but I can still do that.

But before I spend any more time running tests for you here, I have to admit, it seems to me running all those tests is completely off-topic in context of these two bugfixes here.

So maybe it would make sense to stop the discussion here and move it to separate thread ?

I have to admit, I also don't quite understand what it is that you're trying to find out with all those tests.

From: Marek Vasut <[email protected]>

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

The current asynchronous external abort hook implementation restarts the instruction which finally triggered the fault, which can be a different instruction than the read/write instruction which started the faulting access. Usually the instruction which finally triggers the fault is one which has some data dependency on the result of the read/write. In case of read, the read value after fixup is undefined, while a read value of faulting read should be all Fs.

It is possible to enforce the fault using 'isb' instruction placed right after the read/write instruction which started the faulting access. Add custom register accessors which perform the read/write followed immediately by 'isb'.

This way, the fault always happens on the 'isb' and in case of read, which is located one instruction before the 'isb', it is now possible to fix up the return value of the read in the asynchronous external abort hook and make that read return all Fs.

Signed-off-by Marek Vasut <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc Krzysztof Wilczyński <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Yoshihiro Shimoda <[email protected]>

Cc [email protected]

---
V2: Rebase on 1/2
V3: - Add .text.fixup on all three ldr/str/isb instructions and call
fixup_exception() in the abort handler to trigger the fixup.
- Propagate return value from read/write accessors, in case the access fails, return PCIBIOS_SET_FAILED, else PCIBIOS_SUCCESSFUL.

---
 drivers/pci/controller/pcie-rcar-host.c | 53 +++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 4 deletions(-)

drivers/pci/controller/pcie-rcar-host.c +29 -0

index 7d38a9c50093..b2e521ee95eb 100644
@@ -114,6 +114,51 @@ static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 	return val >> shift;
 }
 
+#ifdef CONFIG_ARM
+#define __rcar_pci_rw_reg_workaround(instr)				\
+		"1:	" instr " %1, [%2]\n"				\
+		"2:	isb\n"						\
+		"3:	.pushsection .text.fixup,\"ax\"\n"		\
+		"	.align	2\n"					\
+		"4:	mov	%0, #" __stringify(PCIBIOS_SET_FAILED) "\n" \
+		"	b	3b\n"					\
+		"	.popsection\n"					\
+		"	.pushsection __ex_table,\"a\"\n"		\
+		"	.align	3\n"					\
+		"	.long	1b, 4b\n"				\
+		"	.long	1b, 4b\n"				\
+		"	.popsection\n"
+#endif
+
+int rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
+{
+	int error = PCIBIOS_SUCCESSFUL;
+#ifdef CONFIG_ARM
+	asm volatile(
+		__rcar_pci_rw_reg_workaround("str")
+	: "+r"(error):"r"(val), "r"(pcie->base + reg) : "memory");
+#else
+	rcar_pci_write_reg(pcie, val, reg);
+#endif
+	return error;
+}
+

drivers/pci/controller/pcie-rcar-host.c +10 -4

+#else
+	*val = rcar_pci_read_reg(pcie, reg);
+#endif
+	return error;
+}
+
 /* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
 static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		unsigned char access_type, struct pci_bus *bus,

@@ -185,14 +230,14 @@ static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (access_type == RCAR_PCI_ACCESS_READ)
-		*data = rcar_pci_read_reg(pcie, PCIECDR);
+		ret = rcar_pci_read_reg_workaround(pcie, data, PCIECDR);
 	else
-		rcar_pci_write_reg(pcie, *data, PCIECDR);
+		ret = rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
 
 	/* Disable the configuration access */
 	rcar_pci_write_reg(pcie, 0, PCIECCTLR);
 
-	return PCIBIOS_SUCCESSFUL;
+	return ret;
 }
 
 static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,

@@ -1097,7 +1142,7 @@ static struct platform_driver rcar_pcie_driver = {
 static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
-	return !!rcar_pcie_wakeup(pcie_dev, pcie_base);
+	return !fixup_exception(regs);
 }
 
 static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst = {

Signature · 1 line

2.34.1

From: Marek Vasut <[email protected]>

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

The current asynchronous external abort hook implementation restarts the instruction which finally triggered the fault, which can be a different instruction than the read/write instruction which started the faulting access. Usually the instruction which finally triggers the fault is one which has some data dependency on the result of the read/write. In case of read, the read value after fixup is undefined, while a read value of faulting read should be all Fs.

It is possible to enforce the fault using 'isb' instruction placed right after the read/write instruction which started the faulting access. Add custom register accessors which perform the read/write followed immediately by 'isb'.

This way, the fault always happens on the 'isb' and in case of read, which is located one instruction before the 'isb', it is now possible to fix up the return value of the read in the asynchronous external abort hook and make that read return all Fs.

Signed-off-by Marek Vasut <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc Krzysztof Wilczyński <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Wolfram Sang <[email protected]>

Cc Yoshihiro Shimoda <[email protected]>

Cc [email protected]

---
V2: Rebase on 1/2
V3: - Add .text.fixup on all three ldr/str/isb instructions and call
fixup_exception() in the abort handler to trigger the fixup.
- Propagate return value from read/write accessors, in case the access fails, return PCIBIOS_SET_FAILED, else PCIBIOS_SUCCESSFUL.

---
 drivers/pci/controller/pcie-rcar-host.c | 53 +++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 4 deletions(-)

drivers/pci/controller/pcie-rcar-host.c +49 -4

index 7d38a9c50093..b2e521ee95eb 100644
@@ -114,6 +114,51 @@ static u32 rcar_read_conf(struct rcar_pcie *pcie, int where)
 	return val >> shift;
 }
 
+#ifdef CONFIG_ARM
+#define __rcar_pci_rw_reg_workaround(instr)				\
+		"1:	" instr " %1, [%2]\n"				\
+		"2:	isb\n"						\
+		"3:	.pushsection .text.fixup,\"ax\"\n"		\
+		"	.align	2\n"					\
+		"4:	mov	%0, #" __stringify(PCIBIOS_SET_FAILED) "\n" \
+		"	b	3b\n"					\
+		"	.popsection\n"					\
+		"	.pushsection __ex_table,\"a\"\n"		\
+		"	.align	3\n"					\
+		"	.long	1b, 4b\n"				\
+		"	.long	1b, 4b\n"				\
+		"	.popsection\n"
+#endif
+
+int rcar_pci_write_reg_workaround(struct rcar_pcie *pcie, u32 val, unsigned int reg)
+{
+	int error = PCIBIOS_SUCCESSFUL;
+#ifdef CONFIG_ARM
+	asm volatile(
+		__rcar_pci_rw_reg_workaround("str")
+	: "+r"(error):"r"(val), "r"(pcie->base + reg) : "memory");
+#else
+	rcar_pci_write_reg(pcie, val, reg);
+#endif
+	return error;
+}
+
+int rcar_pci_read_reg_workaround(struct rcar_pcie *pcie, u32 *val, unsigned int reg)
+{
+	int error = PCIBIOS_SUCCESSFUL;
+#ifdef CONFIG_ARM
+	asm volatile(
+		__rcar_pci_rw_reg_workaround("ldr")
+	: "+r"(error), "=r"(*val) : "r"(pcie->base + reg) : "memory");
+
+	if (error != PCIBIOS_SUCCESSFUL)
+		*val = 0xffffffff;
+#else
+	*val = rcar_pci_read_reg(pcie, reg);
+#endif
+	return error;
+}
+
 /* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
 static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		unsigned char access_type, struct pci_bus *bus,

@@ -185,14 +230,14 @@ static int rcar_pcie_config_access(struct rcar_pcie_host *host,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (access_type == RCAR_PCI_ACCESS_READ)
-		*data = rcar_pci_read_reg(pcie, PCIECDR);
+		ret = rcar_pci_read_reg_workaround(pcie, data, PCIECDR);
 	else
-		rcar_pci_write_reg(pcie, *data, PCIECDR);
+		ret = rcar_pci_write_reg_workaround(pcie, *data, PCIECDR);
 
 	/* Disable the configuration access */
 	rcar_pci_write_reg(pcie, 0, PCIECCTLR);
 
-	return PCIBIOS_SUCCESSFUL;
+	return ret;
 }
 
 static int rcar_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,

@@ -1097,7 +1142,7 @@ static struct platform_driver rcar_pcie_driver = {
 static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
 {
-	return !!rcar_pcie_wakeup(pcie_dev, pcie_base);
+	return !fixup_exception(regs);
 }
 
 static const struct of_device_id rcar_pcie_abort_handler_of_match[] __initconst = {

Signature · 1 line

2.34.1

Quoted 18 lines

In case the controller is transitioning to L1 in rcar_pcie_config_access(), any read/write access to PCIECDR triggers asynchronous external abort. This is because the transition to L1 link state must be manually finished by the driver. The PCIe IP can transition back from L1 state to L0 on its own.

The current asynchronous external abort hook implementation restarts the instruction which finally triggered the fault, which can be a different instruction than the read/write instruction which started the faulting access. Usually the instruction which finally triggers the fault is one which has some data dependency on the result of the read/write. In case of read, the read value after fixup is undefined, while a read value of faulting read should be all Fs.

It is possible to enforce the fault using 'isb' instruction placed right after the read/write instruction which started the faulting access. Add custom register accessors which perform the read/write followed immediately by 'isb'.

Quoted 4 lines

This way, the fault always happens on the 'isb' and in case of read, which is located one instruction before the 'isb', it is now possible to fix up the return value of the read in the asynchronous external abort hook and make that read return all Fs.

I'm looking at this again and I do not think that this is reliable.

Asynchronous aborts are by definition asynchronous. Placing isb looks like a hack to decrease probability that asynchronous abort would be triggered at wrong time.

Marek: Cannot you change the code to trigger proper synchronous abort for this operation? If this is ARMv7 system, what about trying to change memory mapping where is the accessing address to strongly-ordered? Writing to strongly-ordered ARMv7 mapping could not report asynchronous aborts anymore, but I'm not sure.

Marek: Are you sure that also ldr instruction is causing asynchronous abort? This is strange as normally load from memory mapped config space could not finish earlier than data from config space are fetched. Normally these load instructions should cause synchronous abort on data errors.