On Tue, 4 Jul 2017 09:02:06 +0100 Ard Biesheuvel wrote:

On 4 July 2017 at 07:58, Jisheng Zhang [off-list ref] wrote:

quoted

On Mon, 3 Jul 2017 08:27:04 -0500 wrote:
 

quoted

[+cc Jingoo, Joao]

On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote:  

quoted

On 3 July 2017 at 00:18, Bjorn Helgaas [off-list ref] wrote:  

quoted

On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote:  

quoted

This driver is required to work around several hardware bugs
in the PCIe controller.

NB: Revision 1 does not support legacy interrupts, or IO space.  

I had to apply these manually because of conflicts in Kconfig and
Makefile.  What are these based on?  Easiest for me is if you base
them on the current -rc1 tag.
 

quoted

Signed-off-by: Marc Gonzalez <redacted>
---
 drivers/pci/host/Kconfig      |   8 +++
 drivers/pci/host/Makefile     |   1 +
 drivers/pci/host/pcie-tango.c | 164 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h       |   2 +
 4 files changed, 175 insertions(+)
 create mode 100644 drivers/pci/host/pcie-tango.c
 

[..]  

quoted

quoted

+     /*
+      * QUIRK #2
+      * Unfortunately, config and mem spaces are muxed.
+      * Linux does not support such a setting, since drivers are free
+      * to access mem space directly, at any time.
+      * Therefore, we can only PRAY that config and mem space accesses
+      * NEVER occur concurrently.
+      */
+     writel_relaxed(1, pcie->mux);
+     ret = pci_generic_config_read(bus, devfn, where, size, val);
+     writel_relaxed(0, pcie->mux);  

I'm very hesitant about this.  When people stress this, we're going to
get reports of data corruption.  Even with the disclaimer below, I
don't feel good about this.  Adding the driver is an implicit claim
that we support the device, but we know it can't be made reliable.  

I noticed that the Synopsys driver suffers from a similar issue: in
dw_pcie_rd_other_conf(), it happily reprograms the outbound I/O window
to perform a config space access, and switches it back to I/O space
afterwards (unless it has more than 2 viewports, in which case it uses
dedicated windows for I/O space and config space)  

That doesn't sound good.  Jingoo, Joao?  I remember some discussion
about this, but not the details.

I/O accesses use wrappers (inb(), etc), so there's at least the
possibility of a mutex to serialize them with respect to config
accesses.
 

IIRC, for 2 viewports, we don't need to worry about the config space
access, because config space access is serialized by pci_lock; We
do have race between config space and io space. But the accessing config
space and io space at the same time is rare.  

Being 'rare' is not sufficient, unfortunately. In the current
situation, I/O space accesses may occur when the outbound window is
directed to the config space of a potentially completely unrelated
device. This is bad.

Yep, I agree with you.

quoted

And the PCIe EPs which
has io space are rare too, supporting these EPs are not the potential
target of those platforms with 2 viewports.
 

I am not sure EP mode is relevant here. What I do know is that boards

I mean those PCIe EP cards which have IO space, but that doesn't matter.

like the Marvell 8040 based MacchiatoBin uses this IP in RC mode, and
exposes config, MMIO and IO space windows using only 2 viewports. Note
that this is essentially a bug in the DT description, given that its
version of the IP supports 8 viewports. But the driver needs to be
fixed as well.

To fix for 2 viewports situation, we need to serialize access of the io
and config space. In internal repo, we can achieve it by modifying the
io access helper functions such as inl/outl, but this won't be accepted
by the mainline IMHO. Except fixing the HW, any elegant solution?

Suggestions are appreciated.

Thanks,
Jisheng