Re: [PATCH] x86/pci: make pci_mem_start to be aligned only -v4

[Ivan Kokshaysky]

On Mon, Apr 20, 2009 at 05:09:32PM -0700, Yinghai Lu wrote:
> also it seems logical is wrong.
> 
> we should make sure if one pci resource support 64 from pci_read_bases() instead of 
> pcibios_allocate_resources.

pci_read_bases() is already providing all necessary information.

> thinking about: if pci bridge on bus 0 (aka first peer root bus), some device under
> bridge doesn't get allocated resource from BIOS. and those bridge/device does support pref mem64
> then your patch will not getIORESORCE_MEM64 set before pcibios_pci64_verify...

Yes, that's my point - even if host bridge, p2p bridge and device are 64-bit,
there is absolutely no guarantee that after moving the BARs above 4G
the device will work correctly.

> Correct logic should be
> record all device if support 64bit pref mem (with pci_read_bases), and make sure bridge to be
>  consistent with that of device under if.

Your view is very x86 centric, please don't forget that drivers/pci
code is used by other architectures as well:
- limiting 32-bit allocations to 0xffffffff simply breaks non-x86
  architectures. Alpha doesn't even boot with your patch;
- there are lots of devices with 64-bit non-prefetchable memory BARs,
  you don't seem to care about that.

And your patch doesn't work even on x86:

00:01.0 PCI bridge: Intel Corporation 82945G/GZ/P/PL PCI Express Root Port (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-
        Latency: 0, Cache Line Size: 16 bytes
        Bus: primary=00, secondary=04, subordinate=04, sec-latency=0
        I/O behind bridge: 0000e000-0000efff
        Memory behind bridge: cdf00000-cfffffff
        Prefetchable memory behind bridge: 00000000d0000000-00000000dfffffff
        Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
        BridgeCtl: Parity- SERR+ NoISA- VGA+ MAbort- >Reset- FastB2B-
                PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
        Capabilities: [88] Subsystem: Intel Corporation Device 0000
        Capabilities: [80] Power Management version 2
                Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
                Status: D0 PME-Enable- DSel=0 DScale=0 PME-
        Capabilities: [90] Message Signalled Interrupts: Mask- 64bit- Queue=0/0 Enable+
                Address: fee0300c  Data: 4159
        Capabilities: [a0] Express (v1) Root Port (Slot+), MSI 00
                DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
                        ExtTag- RBE- FLReset-
                DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
                        RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
                        MaxPayload 128 bytes, MaxReadReq 128 bytes
                DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
                LnkCap: Port #2, Speed 2.5GT/s, Width x16, ASPM L0s L1, Latency L0 <256ns, L1 <4us
                        ClockPM- Suprise- LLActRep- BwNot-
                LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk+
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x16, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
                SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surpise-
                        Slot #  0, PowerLimit 75.000000; Interlock- NoCompl-
                SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
                        Control: AttnInd Off, PwrInd On, Power- Interlock-
                SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
                        Changed: MRL- PresDet+ LinkState-
                RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
                RootCap: CRSVisible-
                RootSta: PME ReqID 0000, PMEStatus- PMEPending-
00: 86 80 71 27 07 05 10 00 02 00 04 06 04 00 01 00
10: 00 00 00 00 00 00 00 00 00 04 04 00 e0 e0 00 00
20: f0 cd f0 cf 01 d0 f1 df 00 00 00 00 00 00 00 00
30: 00 00 00 00 88 00 00 00 00 00 00 00 0b 01 0a 00

As one can see, the prefetchable base register (0x24) is 0xd001, the
bit 0 indicates 64-bitness. Which is not true, as i82945G/GZ/P/PL
only supports 32-bit addressing (please check the datasheet).

Also, your patch can't handle transparent bridges. And it doesn't
bode well for bus sizing code.

> and that is my patch doing: pci: don't assume pref memio are 64bit -v2

Your patch is all wrong, sorry.

Ivan.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/