| Message ID | E1Zenfg-0004d5-Dg@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 23, 2015 at 06:17:32PM +0100, Russell King wrote: > The idea that you can arbitarily read 32-bits from PCI configuration > space, modify a sub-field (like the command register) and write it > back without consequence is deeply flawed. > > Status registers (such as the status register, PCIe device status > register, etc) contain status bits which are read, write-one-to-clear. > > What this means is that reading 32-bits from the command register, > modifying the command register, and then writing it back has the effect > of clearing any status bits that were indicating at that time. Same for > the PCIe device control register clearing bits in the PCIe device status > register. > > Since the Armada chips support byte, 16-bit and 32-bit accesses to the > registers (unless otherwise stated) and the PCI configuration data > register does not specify otherwise, it seems logical that the chip can > indeed generate the proper configuration access cycles down to byte > level. > > Testing with an ASM1062 PCIe to SATA mini-PCIe card on Armada 388. > PCIe capability at 0x80, DevCtl at 0x88, DevSta at 0x8a. The Armada 388 is a pretty new SoC. It would be good to test this on a much older device as well, e.g. Kirkwood. I will add it to my TODO list, but it anybody else gets there first, please let me know. Andrew
> Testing with an ASM1062 PCIe to SATA mini-PCIe card on Armada 388. > PCIe capability at 0x80, DevCtl at 0x88, DevSta at 0x8a. > > Before: > /# setpci -s 1:0.0 0x88.l - DevSta: CorrErr+ > 00012810 > /# setpci -s 1:0.0 0x88.w=0x2810 - Write DevCtl only > /# setpci -s 1:0.0 0x88.l - CorrErr cleared - FAIL > 00002810 > > After: > /# setpci -s 1:0.0 0x88.l - DevSta: CorrErr+ > 00012810 > /# setpci -s 1:0.0 0x88.w=0x2810 - check DevCtl only write > /# setpci -s 1:0.0 0x88.l - CorErr remains set > 00012810 > /# setpci -s 1:0.0 0x88.w=0x281f - check DevCtl write works > /# setpci -s 1:0.0 0x88.l - devctl field updated > 0001281f > /# setpci -s 1:0.0 0x8a.w=0xffff - clear DevSta > /# setpci -s 1:0.0 0x88.l - CorrErr now cleared > 0000281f > /# setpci -s 1:0.0 0x88.w=0x2810 - restore DevCtl > /# setpci -s 1:0.0 0x88.l - check > 00002810 Hi Russell Can you give me some hints how to test this in my Kirkwood board. root@dir665:~# lspci -nvvvv 00:01.0 0604: 11ab:6281 (rev 02) (prog-if 00 [Normal decode]) Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx- Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 I/O behind bridge: 00010000-00010fff Memory behind bridge: e0000000-e00fffff Prefetchable memory behind bridge: 00000000-000fffff Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR- BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B- PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- 01:00.0 0200: 11ab:2a40 Subsystem: 11ab:2a40 Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Interrupt: pin A routed to IRQ 83 Region 0: Memory at e0000000 (64-bit, non-prefetchable) [disabled] [size=64K] Region 2: Memory at e0010000 (64-bit, non-prefetchable) [disabled] [size=64K] Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=1 PME- Capabilities: [5c] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [e0] Express (v1) Legacy Endpoint, MSI 00 DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 unlimited ClockPM+ Surprise- LLActRep- BwNot- LnkCtl: ASPM Disabled; RCB 128 bytes Disabled- CommClk- ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 1f, GenCap- CGenEn- ChkCap- ChkEn- Capabilities: [130 v1] Device Serial Number 00-00-00-00-00-00-00-00 root@dir665:~# setpci -s 1:0.0 0x88.l 00000000 Nothing there, so your test does not work directly. I tried root@dir665:~# setpci -s 1:0.0 0xe8.l 00102000 root@dir665:~# setpci -s 1:0.0 0xe8.w=0x2000 root@dir665:~# setpci -s 1:0.0 0xe8.l 00102000 but that is not producing the FAIL you had. Thanks Andrew
> > I tried > > > > root@dir665:~# setpci -s 1:0.0 0xe8.l > > 00102000 > > root@dir665:~# setpci -s 1:0.0 0xe8.w=0x2000 > > root@dir665:~# setpci -s 1:0.0 0xe8.l > > 00102000 > > > > but that is not producing the FAIL you had. > > The only bit you have set in your DevSta register is: > > #define PCI_EXP_DEVSTA_AUXPD 0x0010 /* AUX Power Detected */ > > which is not a RW1C bit. So i cannot actually test this on my hardware :-( I guess the best i can do is apply the patch, load the wifi driver, and ensure i can still join a network. Andrew
On Fri, Sep 25, 2015 at 12:23:22AM +0200, Andrew Lunn wrote: > > Testing with an ASM1062 PCIe to SATA mini-PCIe card on Armada 388. > > PCIe capability at 0x80, DevCtl at 0x88, DevSta at 0x8a. From this, the PCIe DevCtl register is at an offset of 8 bytes from the start of the PCIe capability. > Capabilities: [e0] Express (v1) Legacy Endpoint, MSI 00 which, here, starts at 0xe0. So... > DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited > ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- > DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- > RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- > MaxPayload 128 bytes, MaxReadReq 512 bytes This register is at 0xe8, and: > DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend- is at 0xea. > root@dir665:~# setpci -s 1:0.0 0x88.l > 00000000 > > Nothing there, so your test does not work directly. As is expected, because PCI configuration addresses between 0x40..0xff are freely assignable by the vendor to place whatever they want in that space - and the capabilities form a linked list. > I tried > > root@dir665:~# setpci -s 1:0.0 0xe8.l > 00102000 > root@dir665:~# setpci -s 1:0.0 0xe8.w=0x2000 > root@dir665:~# setpci -s 1:0.0 0xe8.l > 00102000 > > but that is not producing the FAIL you had. The only bit you have set in your DevSta register is: #define PCI_EXP_DEVSTA_AUXPD 0x0010 /* AUX Power Detected */ which is not a RW1C bit. I don't know why I get the CorrErr bit set here, but it being set is very useful to test for correct behaviour. I don't yet know how to cause PCIe errors, I just know that I end up with that bit set each time I reboot the board I have here.
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index b6a096bc9422..0d9f3eae4315 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -254,15 +254,22 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port, struct pci_bus *bus, u32 devfn, int where, int size, u32 *val) { + void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF; + mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where), PCIE_CONF_ADDR_OFF); - *val = mvebu_readl(port, PCIE_CONF_DATA_OFF); - - if (size == 1) - *val = (*val >> (8 * (where & 3))) & 0xff; - else if (size == 2) - *val = (*val >> (8 * (where & 3))) & 0xffff; + switch (size) { + case 1: + *val = readb_relaxed(conf_data + (where & 3)); + break; + case 2: + *val = readw_relaxed(conf_data + (where & 2)); + break; + case 4: + *val = readl_relaxed(conf_data); + break; + } return PCIBIOS_SUCCESSFUL; } @@ -271,22 +278,24 @@ static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port, struct pci_bus *bus, u32 devfn, int where, int size, u32 val) { - u32 _val, shift = 8 * (where & 3); + void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF; mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where), PCIE_CONF_ADDR_OFF); - _val = mvebu_readl(port, PCIE_CONF_DATA_OFF); - if (size == 4) - _val = val; - else if (size == 2) - _val = (_val & ~(0xffff << shift)) | ((val & 0xffff) << shift); - else if (size == 1) - _val = (_val & ~(0xff << shift)) | ((val & 0xff) << shift); - else + switch (size) { + case 1: + writeb(val, conf_data + (where & 3)); + break; + case 2: + writew(val, conf_data + (where & 2)); + break; + case 4: + writel(val, conf_data); + break; + default: return PCIBIOS_BAD_REGISTER_NUMBER; - - mvebu_writel(port, _val, PCIE_CONF_DATA_OFF); + } return PCIBIOS_SUCCESSFUL; }
The idea that you can arbitarily read 32-bits from PCI configuration space, modify a sub-field (like the command register) and write it back without consequence is deeply flawed. Status registers (such as the status register, PCIe device status register, etc) contain status bits which are read, write-one-to-clear. What this means is that reading 32-bits from the command register, modifying the command register, and then writing it back has the effect of clearing any status bits that were indicating at that time. Same for the PCIe device control register clearing bits in the PCIe device status register. Since the Armada chips support byte, 16-bit and 32-bit accesses to the registers (unless otherwise stated) and the PCI configuration data register does not specify otherwise, it seems logical that the chip can indeed generate the proper configuration access cycles down to byte level. Testing with an ASM1062 PCIe to SATA mini-PCIe card on Armada 388. PCIe capability at 0x80, DevCtl at 0x88, DevSta at 0x8a. Before: /# setpci -s 1:0.0 0x88.l - DevSta: CorrErr+ 00012810 /# setpci -s 1:0.0 0x88.w=0x2810 - Write DevCtl only /# setpci -s 1:0.0 0x88.l - CorrErr cleared - FAIL 00002810 After: /# setpci -s 1:0.0 0x88.l - DevSta: CorrErr+ 00012810 /# setpci -s 1:0.0 0x88.w=0x2810 - check DevCtl only write /# setpci -s 1:0.0 0x88.l - CorErr remains set 00012810 /# setpci -s 1:0.0 0x88.w=0x281f - check DevCtl write works /# setpci -s 1:0.0 0x88.l - devctl field updated 0001281f /# setpci -s 1:0.0 0x8a.w=0xffff - clear DevSta /# setpci -s 1:0.0 0x88.l - CorrErr now cleared 0000281f /# setpci -s 1:0.0 0x88.w=0x2810 - restore DevCtl /# setpci -s 1:0.0 0x88.l - check 00002810 Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/pci/host/pci-mvebu.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-)