From: Ard Biesheuvel <ard.biesheuvel@linaro.org> To: Jisheng Zhang <jszhang@marvell.com> Cc: Bjorn Helgaas <helgaas@kernel.org>, Joao Pinto <Joao.Pinto@synopsys.com>, Mason <slash.tmp@free.fr>, Marc Zyngier <marc.zyngier@arm.com>, linux-pci <linux-pci@vger.kernel.org>, Thibaud Cornic <thibaud_cornic@sigmadesigns.com>, LKML <linux-kernel@vger.kernel.org>, Jingoo Han <jingoohan1@gmail.com>, Thomas Gleixner <tglx@linutronix.de>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Marc Gonzalez <marc_gonzalez@sigmadesigns.com> Subject: Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Date: Tue, 4 Jul 2017 09:02:06 +0100 [thread overview] Message-ID: <CAKv+Gu9gi1W2Ex1hchMdob9PqkQ94uYspQW31W2+RG_sgsEV4Q@mail.gmail.com> (raw) In-Reply-To: <20170704145840.69e53f58@xhacker> On 4 July 2017 at 07:58, Jisheng Zhang <jszhang@marvell.com> wrote: > On Mon, 3 Jul 2017 08:27:04 -0500 wrote: > >> [+cc Jingoo, Joao] >> >> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote: >> > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote: >> > >> 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. >> > > >> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> >> > >> --- >> > >> 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 >> > >> >> > [..] >> > >> + /* >> > >> + * 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. > 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 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.
WARNING: multiple messages have this Message-ID (diff)
From: ard.biesheuvel@linaro.org (Ard Biesheuvel) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Date: Tue, 4 Jul 2017 09:02:06 +0100 [thread overview] Message-ID: <CAKv+Gu9gi1W2Ex1hchMdob9PqkQ94uYspQW31W2+RG_sgsEV4Q@mail.gmail.com> (raw) In-Reply-To: <20170704145840.69e53f58@xhacker> On 4 July 2017 at 07:58, Jisheng Zhang <jszhang@marvell.com> wrote: > On Mon, 3 Jul 2017 08:27:04 -0500 wrote: > >> [+cc Jingoo, Joao] >> >> On Mon, Jul 03, 2017 at 10:35:50AM +0100, Ard Biesheuvel wrote: >> > On 3 July 2017 at 00:18, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > > On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote: >> > >> 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. >> > > >> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> >> > >> --- >> > >> 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 >> > >> >> > [..] >> > >> + /* >> > >> + * 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. > 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 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.
next prev parent reply other threads:[~2017-07-04 8:02 UTC|newest] Thread overview: 109+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-20 8:12 [PATCH v9 0/3] Tango PCIe controller support Marc Gonzalez 2017-06-20 8:12 ` Marc Gonzalez 2017-06-20 8:12 ` Marc Gonzalez 2017-06-20 8:14 ` [PATCH v9 1/3] PCI: Add DT binding for tango PCIe controller Marc Gonzalez 2017-06-20 8:14 ` Marc Gonzalez 2017-06-20 8:14 ` Marc Gonzalez 2017-06-20 8:17 ` [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Marc Gonzalez 2017-06-20 8:17 ` Marc Gonzalez 2017-06-20 8:17 ` Marc Gonzalez 2017-07-02 23:18 ` Bjorn Helgaas 2017-07-02 23:18 ` Bjorn Helgaas 2017-07-02 23:18 ` Bjorn Helgaas 2017-07-03 9:35 ` Ard Biesheuvel 2017-07-03 9:35 ` Ard Biesheuvel 2017-07-03 13:27 ` Bjorn Helgaas 2017-07-03 13:27 ` Bjorn Helgaas 2017-07-03 13:27 ` Bjorn Helgaas 2017-07-04 6:58 ` Jisheng Zhang 2017-07-04 6:58 ` Jisheng Zhang 2017-07-04 6:58 ` Jisheng Zhang 2017-07-04 7:16 ` Jisheng Zhang 2017-07-04 7:16 ` Jisheng Zhang 2017-07-04 8:02 ` Ard Biesheuvel [this message] 2017-07-04 8:02 ` Ard Biesheuvel 2017-07-04 8:19 ` Jisheng Zhang 2017-07-04 8:19 ` Jisheng Zhang 2017-07-04 8:19 ` Jisheng Zhang 2017-07-04 9:38 ` Ard Biesheuvel 2017-07-04 9:38 ` Ard Biesheuvel 2017-07-05 13:53 ` Joao Pinto 2017-07-05 13:53 ` Joao Pinto 2017-07-03 9:54 ` Marc Gonzalez 2017-07-03 9:54 ` Marc Gonzalez 2017-07-03 9:54 ` Marc Gonzalez 2017-07-03 13:13 ` Marc Gonzalez 2017-07-03 13:13 ` Marc Gonzalez 2017-07-03 15:30 ` Marc Gonzalez 2017-07-03 15:30 ` Marc Gonzalez 2017-07-03 15:30 ` Marc Gonzalez 2017-07-04 7:09 ` Peter Zijlstra 2017-07-04 7:09 ` Peter Zijlstra 2017-07-04 13:08 ` Mason 2017-07-04 13:08 ` Mason 2017-07-04 14:27 ` Peter Zijlstra 2017-07-04 14:27 ` Peter Zijlstra 2017-07-04 15:18 ` Mason 2017-07-04 15:18 ` Mason 2017-07-03 13:40 ` Bjorn Helgaas 2017-07-03 13:40 ` Bjorn Helgaas 2017-07-03 13:40 ` Bjorn Helgaas 2017-07-03 14:34 ` Marc Gonzalez 2017-07-03 14:34 ` Marc Gonzalez 2017-07-03 14:34 ` Marc Gonzalez 2017-07-04 15:58 ` Bjorn Helgaas 2017-07-04 15:58 ` Bjorn Helgaas 2017-07-04 15:58 ` Bjorn Helgaas 2017-07-04 23:42 ` Mason 2017-07-04 23:42 ` Mason 2017-07-03 18:11 ` Russell King - ARM Linux 2017-07-03 18:11 ` Russell King - ARM Linux 2017-07-03 18:44 ` Ard Biesheuvel 2017-07-03 18:44 ` Ard Biesheuvel 2017-07-04 15:15 ` Bjorn Helgaas 2017-07-04 15:15 ` Bjorn Helgaas 2017-07-04 15:15 ` Bjorn Helgaas 2017-07-04 18:17 ` Russell King - ARM Linux 2017-07-04 18:17 ` Russell King - ARM Linux 2017-07-04 23:59 ` Mason 2017-07-04 23:59 ` Mason 2017-07-05 5:21 ` Greg Kroah-Hartman 2017-07-05 5:21 ` Greg Kroah-Hartman 2017-07-05 12:33 ` Mark Brown 2017-07-05 12:33 ` Mark Brown 2017-06-20 8:18 ` [PATCH v9 3/3] PCI: Add tango MSI controller support Marc Gonzalez 2017-06-20 8:18 ` Marc Gonzalez 2017-06-20 8:18 ` Marc Gonzalez 2017-07-04 20:24 ` [PATCH v9 0/3] Tango PCIe " Bjorn Helgaas 2017-07-04 20:24 ` Bjorn Helgaas 2017-07-04 20:24 ` Bjorn Helgaas 2017-07-04 22:55 ` Mason 2017-07-04 22:55 ` Mason 2017-07-04 22:55 ` Mason 2017-07-05 18:03 ` Bjorn Helgaas 2017-07-05 18:03 ` Bjorn Helgaas 2017-07-05 18:03 ` Bjorn Helgaas 2017-07-05 20:39 ` Mason 2017-07-05 20:39 ` Mason 2017-07-05 20:39 ` Mason 2017-07-05 21:34 ` Bjorn Helgaas 2017-07-05 21:34 ` Bjorn Helgaas 2017-07-05 21:34 ` Bjorn Helgaas 2017-07-05 21:34 ` Bjorn Helgaas 2017-07-05 21:59 ` Mason 2017-07-05 21:59 ` Mason 2017-07-05 21:59 ` Mason 2017-07-06 3:39 ` Bjorn Helgaas 2017-07-06 3:39 ` Bjorn Helgaas 2017-07-06 3:39 ` Bjorn Helgaas 2017-07-06 3:39 ` Bjorn Helgaas 2017-07-06 12:26 ` Mason 2017-07-06 12:26 ` Mason 2017-07-06 12:40 ` Marc Zyngier 2017-07-06 12:40 ` Marc Zyngier 2017-07-06 12:40 ` Marc Zyngier 2017-07-06 19:46 ` Bjorn Helgaas 2017-07-06 19:46 ` Bjorn Helgaas 2017-07-06 19:46 ` Bjorn Helgaas 2017-07-07 9:55 ` Marc Gonzalez 2017-07-07 9:55 ` Marc Gonzalez
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=CAKv+Gu9gi1W2Ex1hchMdob9PqkQ94uYspQW31W2+RG_sgsEV4Q@mail.gmail.com \ --to=ard.biesheuvel@linaro.org \ --cc=Joao.Pinto@synopsys.com \ --cc=helgaas@kernel.org \ --cc=jingoohan1@gmail.com \ --cc=jszhang@marvell.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=marc_gonzalez@sigmadesigns.com \ --cc=slash.tmp@free.fr \ --cc=tglx@linutronix.de \ --cc=thibaud_cornic@sigmadesigns.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.