I have a Dell notebook with an PCIe ExpressCard slot.
I also have a PCIe ExpressCard SATA controller (uses sata_sil24 driver).
I would like to be able to hot plug/unplug the controller card at will.
But alas, Linux doesn't cope with it *unless* I boot the kernel with
the card initially inserted.
1. Booting Linux kernel (latest 2.6.23) without the card inserted
means that the card will never be detected, regardless of how many
times subsequently the card is inserted/removed/whatever.
2. Booting Linux kernel *with* the card inserted means that it is
detected and used, and can be unplugged/replugged as I please,
with intervening suspend/resume (RAM or disk) cycles not interfering.
3. Booting Linux kernel without the card inserted, and then doing
a suspend-to-disk poweroff, inserting the card, and powering on again,
the card's BIOS extension runs as normal. But on resume from the
suspend-to-disk, the running kernel again never sees the card,
even after removing/reinserting/whatever.
4. All of this leads me to believe that the kernel must be doing some
kind of once-only scan of hardware at boot time, and never repeating
it afterwards. Loading/unloading all of the PCI/PCIe hotplug stuff
has no effect on this, so it must be broken elsewhere.
5. It is not likely to be a BIOS thing, because it still fails on
power-on (with card inserted) after a suspend-to-disk, which appears
to the BIOS exactly the same as any other power-on.
6. But it's probably a "kernel relies on BIOS data structure read
at boot time" issue, based on the observations above.
Suggestions? Is this a known defect?
Maybe with a known fix lurking in a git tree somewhere?
00:00.0 Host bridge: Intel Corporation Mobile 945GM/PM/GMS/940GML and 945GT Express Memory Controller Hub (rev 03)
00:01.0 PCI bridge: Intel Corporation Mobile 945GM/PM/GMS/940GML and 945GT Express PCI Express Root Port (rev 03)
00:1b.0 Audio device: Intel Corporation 82801G (ICH7 Family) High Definition Audio Controller (rev 01)
00:1c.0 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 1 (rev 01)
00:1c.1 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 2 (rev 01)
00:1c.3 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 4 (rev 01)
00:1d.0 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #1 (rev 01)
00:1d.1 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #2 (rev 01)
00:1d.2 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #3 (rev 01)
00:1d.3 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #4 (rev 01)
00:1d.7 USB Controller: Intel Corporation 82801G (ICH7 Family) USB2 EHCI Controller (rev 01)
00:1e.0 PCI bridge: Intel Corporation 82801 Mobile PCI Bridge (rev e1)
00:1f.0 ISA bridge: Intel Corporation 82801GBM (ICH7-M) LPC Interface Bridge (rev 01)
00:1f.2 IDE interface: Intel Corporation 82801GBM/GHM (ICH7 Family) Serial ATA Storage Controller IDE (rev 01)
00:1f.3 SMBus: Intel Corporation 82801G (ICH7 Family) SMBus Controller (rev 01)
01:00.0 VGA compatible controller: ATI Technologies Inc Radeon Mobility X1400
03:00.0 Ethernet controller: Broadcom Corporation BCM4401-B0 100Base-TX (rev 02)
03:01.0 FireWire (IEEE 1394): Ricoh Co Ltd Unknown device 0832
03:01.1 Generic system peripheral [0805]: Ricoh Co Ltd R5C822 SD/SDIO/MMC/MS/MSPro Host Adapter (rev 19)
03:01.2 System peripheral: Ricoh Co Ltd Unknown device 0843 (rev 01)
03:01.3 System peripheral: Ricoh Co Ltd R5C592 Memory Stick Bus Host Adapter (rev 0a)
03:01.4 System peripheral: Ricoh Co Ltd xD-Picture Card Controller (rev 05)
0c:00.0 Network controller: Intel Corporation PRO/Wireless 3945ABG Network Connection (rev 02)
0d:00.0 Mass storage controller: Silicon Image, Inc. SiI 3132 Serial ATA Raid II Controller (rev 01)
Mark Lord wrote:
> I have a Dell notebook with an PCIe ExpressCard slot.
> I also have a PCIe ExpressCard SATA controller (uses sata_sil24 driver).
>
> I would like to be able to hot plug/unplug the controller card at will.
> But alas, Linux doesn't cope with it *unless* I boot the kernel with
> the card initially inserted.
>
> 1. Booting Linux kernel (latest 2.6.23) without the card inserted
> means that the card will never be detected, regardless of how many
> times subsequently the card is inserted/removed/whatever.
>
> 2. Booting Linux kernel *with* the card inserted means that it is
> detected and used, and can be unplugged/replugged as I please,
> with intervening suspend/resume (RAM or disk) cycles not interfering.
>
> 3. Booting Linux kernel without the card inserted, and then doing
> a suspend-to-disk poweroff, inserting the card, and powering on again,
> the card's BIOS extension runs as normal. But on resume from the
> suspend-to-disk, the running kernel again never sees the card,
> even after removing/reinserting/whatever.
>
> 4. All of this leads me to believe that the kernel must be doing some
> kind of once-only scan of hardware at boot time, and never repeating
> it afterwards. Loading/unloading all of the PCI/PCIe hotplug stuff
> has no effect on this, so it must be broken elsewhere.
>
> 5. It is not likely to be a BIOS thing, because it still fails on
> power-on (with card inserted) after a suspend-to-disk, which appears
> to the BIOS exactly the same as any other power-on.
>
> 6. But it's probably a "kernel relies on BIOS data structure read
> at boot time" issue, based on the observations above.
Actually, I must now take back some of that.
Most of these tests were done a month or two ago.
With 2.6.23.1 running, I just now redid all of the tests.
Now it seems that pciehp fails to notice a newly inserted card
only after a suspend/resume cycle with the slot empty.
I can now get it to work again by just doing:
1. remove the card, so the slot is empty.
2. rmmod pciehp; modprobe pciehp
3. insert the card again -- it works!
So we just need to fix an issue or two with suspend/resume (RAM) in pciehp.
Cheers
On Tue, 16 Oct 2007 11:21:36 -0400
Mark Lord <[email protected]> wrote:
> Mark Lord wrote:
> > I have a Dell notebook with an PCIe ExpressCard slot.
> > I also have a PCIe ExpressCard SATA controller (uses sata_sil24 driver).
> >
> > I would like to be able to hot plug/unplug the controller card at will.
> > But alas, Linux doesn't cope with it *unless* I boot the kernel with
> > the card initially inserted.
> >
> > 1. Booting Linux kernel (latest 2.6.23) without the card inserted
> > means that the card will never be detected, regardless of how many
> > times subsequently the card is inserted/removed/whatever.
> >
> > 2. Booting Linux kernel *with* the card inserted means that it is
> > detected and used, and can be unplugged/replugged as I please,
> > with intervening suspend/resume (RAM or disk) cycles not interfering.
> >
> > 3. Booting Linux kernel without the card inserted, and then doing
> > a suspend-to-disk poweroff, inserting the card, and powering on again,
> > the card's BIOS extension runs as normal. But on resume from the
> > suspend-to-disk, the running kernel again never sees the card,
> > even after removing/reinserting/whatever.
> >
> > 4. All of this leads me to believe that the kernel must be doing some
> > kind of once-only scan of hardware at boot time, and never repeating
> > it afterwards. Loading/unloading all of the PCI/PCIe hotplug stuff
> > has no effect on this, so it must be broken elsewhere.
> >
> > 5. It is not likely to be a BIOS thing, because it still fails on
> > power-on (with card inserted) after a suspend-to-disk, which appears
> > to the BIOS exactly the same as any other power-on.
> >
> > 6. But it's probably a "kernel relies on BIOS data structure read
> > at boot time" issue, based on the observations above.
>
> Actually, I must now take back some of that.
>
> Most of these tests were done a month or two ago.
> With 2.6.23.1 running, I just now redid all of the tests.
>
> Now it seems that pciehp fails to notice a newly inserted card
> only after a suspend/resume cycle with the slot empty.
>
> I can now get it to work again by just doing:
> 1. remove the card, so the slot is empty.
> 2. rmmod pciehp; modprobe pciehp
> 3. insert the card again -- it works!
>
> So we just need to fix an issue or two with suspend/resume (RAM) in pciehp.
>
> Cheers
>
Hi Mark,
So, just to make sure I understand, your reproducer for the failing case is:
1. Boot laptop with no card.
2. Load pciehp
3. Suspend laptop (slot is still empty)
4. Resume laptop (slot is still empty)
5. insert card - card is not detected.
Can you tell me which Dell laptop you have, and also send me the dmesg
output of the failing case after loading pciehp with pciehp_debug=1.
Thanks,
Kristen
00:00.0 Host bridge: Intel Corporation Mobile 945GM/PM/GMS/940GML and 945GT Express Memory Controller Hub (rev 03)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ >SERR- <PERR-
Latency: 0
Capabilities: [e0] Vendor Specific Information
00:01.0 PCI bridge: Intel Corporation Mobile 945GM/PM/GMS/940GML and 945GT Express PCI Express Root Port (rev 03) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR+ <PERR-
Latency: 0, Cache Line Size: 64 bytes
Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
I/O behind bridge: 0000e000-0000efff
Memory behind bridge: efd00000-efefffff
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-
Capabilities: [88] Subsystem: Dell Unknown device 01cd
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: 00000000 Data: 0000
Capabilities: [a0] Express Root Port (Slot+) IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag-
Device: Latency L0s <64ns, L1 <1us
Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
Device: RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed 2.5Gb/s, Width x16, ASPM L0s L1, Port 2
Link: Latency L0s <256ns, L1 <4us
Link: ASPM L1 Enabled RCB 64 bytes CommClk+ ExtSynch-
Link: Speed 2.5Gb/s, Width x16
Slot: AtnBtn- PwrCtrl- MRL- AtnInd- PwrInd- HotPlug- Surpise-
Slot: Number 1, PowerLimit 75.000000
Slot: Enabled AtnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq-
Slot: AttnInd Off, PwrInd On, Power-
Root: Correctable- Non-Fatal- Fatal- PME-
Capabilities: [100] Virtual Channel
Capabilities: [140] Unknown (5)
00:1b.0 Audio device: Intel Corporation 82801G (ICH7 Family) High Definition Audio Controller (rev 01)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 20
Region 0: Memory at efffc000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [50] Power Management version 2
Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [60] Message Signalled Interrupts: Mask- 64bit+ Queue=0/0 Enable-
Address: 0000000000000000 Data: 0000
Capabilities: [70] Express Unknown type IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag-
Device: Latency L0s <64ns, L1 <1us
Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
Device: RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop+
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed unknown, Width x0, ASPM unknown, Port 0
Link: Latency L0s <64ns, L1 <1us
Link: ASPM Disabled CommClk- ExtSynch-
Link: Speed unknown, Width x0
Capabilities: [100] Virtual Channel
Capabilities: [130] Unknown (5)
00:1c.0 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 1 (rev 01) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0, Cache Line Size: 64 bytes
Bus: primary=00, secondary=0b, subordinate=0b, sec-latency=0
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
Capabilities: [40] Express Root Port (Slot+) IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag-
Device: Latency L0s unlimited, L1 unlimited
Device: Errors: Correctable- Non-Fatal- Fatal+ Unsupported-
Device: RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed 2.5Gb/s, Width x1, ASPM L0s L1, Port 1
Link: Latency L0s <1us, L1 <4us
Link: ASPM Disabled RCB 64 bytes CommClk- ExtSynch-
Link: Speed 2.5Gb/s, Width x0
Slot: AtnBtn- PwrCtrl- MRL- AtnInd- PwrInd- HotPlug+ Surpise+
Slot: Number 2, PowerLimit 6.500000
Slot: Enabled AtnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+
Slot: AttnInd Unknown, PwrInd Unknown, Power-
Root: Correctable- Non-Fatal- Fatal- PME-
Capabilities: [80] Message Signalled Interrupts: Mask- 64bit- Queue=0/0 Enable-
Address: 00000000 Data: 0000
Capabilities: [90] Subsystem: Dell Unknown device 01cd
Capabilities: [a0] 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: [100] Virtual Channel
Capabilities: [180] Unknown (5)
00:1c.1 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 2 (rev 01) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0, Cache Line Size: 64 bytes
Bus: primary=00, secondary=0c, subordinate=0c, sec-latency=0
Memory behind bridge: efc00000-efcfffff
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
Capabilities: [40] Express Root Port (Slot+) IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag-
Device: Latency L0s unlimited, L1 unlimited
Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
Device: RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed 2.5Gb/s, Width x1, ASPM L0s L1, Port 2
Link: Latency L0s <256ns, L1 <4us
Link: ASPM L1 Enabled RCB 64 bytes CommClk+ ExtSynch-
Link: Speed 2.5Gb/s, Width x1
Slot: AtnBtn- PwrCtrl- MRL- AtnInd- PwrInd- HotPlug+ Surpise+
Slot: Number 3, PowerLimit 6.500000
Slot: Enabled AtnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+
Slot: AttnInd Unknown, PwrInd Unknown, Power-
Root: Correctable- Non-Fatal- Fatal- PME-
Capabilities: [80] Message Signalled Interrupts: Mask- 64bit- Queue=0/0 Enable-
Address: 00000000 Data: 0000
Capabilities: [90] Subsystem: Dell Unknown device 01cd
Capabilities: [a0] 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: [100] Virtual Channel
Capabilities: [180] Unknown (5)
00:1c.3 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 4 (rev 01) (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0, Cache Line Size: 64 bytes
Bus: primary=00, secondary=0d, subordinate=0e, sec-latency=0
I/O behind bridge: 0000d000-0000dfff
Memory behind bridge: efa00000-efbfffff
Prefetchable memory behind bridge: 00000000e0000000-00000000e01fffff
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
Capabilities: [40] Express Root Port (Slot+) IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag-
Device: Latency L0s unlimited, L1 unlimited
Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
Device: RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed 2.5Gb/s, Width x1, ASPM L0s L1, Port 4
Link: Latency L0s <1us, L1 <4us
Link: ASPM Disabled RCB 64 bytes CommClk- ExtSynch-
Link: Speed 2.5Gb/s, Width x1
Slot: AtnBtn- PwrCtrl- MRL- AtnInd- PwrInd- HotPlug+ Surpise+
Slot: Number 5, PowerLimit 6.500000
Slot: Enabled AtnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+
Slot: AttnInd Unknown, PwrInd Unknown, Power-
Root: Correctable- Non-Fatal- Fatal- PME-
Capabilities: [80] Message Signalled Interrupts: Mask- 64bit- Queue=0/0 Enable-
Address: 00000000 Data: 0000
Capabilities: [90] Subsystem: Dell Unknown device 01cd
Capabilities: [a0] 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: [100] Virtual Channel
Capabilities: [180] Unknown (5)
00:1d.0 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #1 (rev 01) (prog-if 00 [UHCI])
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin A routed to IRQ 19
Region 4: I/O ports at bf80 [size=32]
00:1d.1 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #2 (rev 01) (prog-if 00 [UHCI])
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin B routed to IRQ 20
Region 4: I/O ports at bf60 [size=32]
00:1d.2 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #3 (rev 01) (prog-if 00 [UHCI])
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin C routed to IRQ 21
Region 4: I/O ports at bf40 [size=32]
00:1d.3 USB Controller: Intel Corporation 82801G (ICH7 Family) USB UHCI #4 (rev 01) (prog-if 00 [UHCI])
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin D routed to IRQ 22
Region 4: I/O ports at bf20 [size=32]
00:1d.7 USB Controller: Intel Corporation 82801G (ICH7 Family) USB2 EHCI Controller (rev 01) (prog-if 20 [EHCI])
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin A routed to IRQ 19
Region 0: Memory at ffa80000 (32-bit, non-prefetchable) [size=1K]
Capabilities: [50] Power Management version 2
Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [58] Debug port
00:1e.0 PCI bridge: Intel Corporation 82801 Mobile PCI Bridge (rev e1) (prog-if 01 [Subtractive decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Bus: primary=00, secondary=03, subordinate=03, sec-latency=32
Memory behind bridge: ef900000-ef9fffff
Secondary status: 66MHz- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
Capabilities: [50] Subsystem: Dell Unknown device 01cd
00:1f.0 ISA bridge: Intel Corporation 82801GBM (ICH7-M) LPC Interface Bridge (rev 01)
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Capabilities: [e0] Vendor Specific Information
00:1f.2 IDE interface: Intel Corporation 82801GBM/GHM (ICH7 Family) Serial ATA Storage Controller IDE (rev 01) (prog-if 80 [Master])
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin B routed to IRQ 17
Region 0: I/O ports at 01f0 [size=8]
Region 1: I/O ports at 03f4 [size=1]
Region 2: I/O ports at 0170 [size=8]
Region 3: I/O ports at 0374 [size=1]
Region 4: I/O ports at bfa0 [size=16]
Capabilities: [70] 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-
00:1f.3 SMBus: Intel Corporation 82801G (ICH7 Family) SMBus Controller (rev 01)
Subsystem: Dell Unknown device 01cd
Control: I/O+ Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Interrupt: pin B routed to IRQ 5
Region 4: I/O ports at 10c0 [size=32]
01:00.0 VGA compatible controller: ATI Technologies Inc Radeon Mobility X1400 (prog-if 00 [VGA])
Subsystem: Dell Unknown device 2002
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ >SERR- <PERR-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 4
Region 0: Memory at d0000000 (32-bit, prefetchable) [size=256M]
Region 1: I/O ports at ee00 [size=256]
Region 2: Memory at efdf0000 (32-bit, non-prefetchable) [size=64K]
Expansion ROM at efe00000 [disabled] [size=128K]
Capabilities: [50] 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: [58] Express Legacy Endpoint IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag+
Device: Latency L0s <4us, L1 unlimited
Device: AtnBtn- AtnInd- PwrInd-
Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
Device: RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed 2.5Gb/s, Width x16, ASPM L0s L1, Port 0
Link: Latency L0s <64ns, L1 <1us
Link: ASPM L1 Enabled RCB 64 bytes CommClk+ ExtSynch-
Link: Speed 2.5Gb/s, Width x16
03:00.0 Ethernet controller: Broadcom Corporation BCM4401-B0 100Base-TX (rev 02)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 64
Interrupt: pin A routed to IRQ 17
Region 0: Memory at ef9fe000 (32-bit, non-prefetchable) [size=8K]
Capabilities: [40] Power Management version 2
Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
Status: D0 PME-Enable- DSel=0 DScale=2 PME-
03:01.0 FireWire (IEEE 1394): Ricoh Co Ltd Unknown device 0832 (prog-if 10 [OHCI])
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 64 (500ns min, 1000ns max)
Interrupt: pin A routed to IRQ 18
Region 0: Memory at ef9fd800 (32-bit, non-prefetchable) [size=2K]
Capabilities: [dc] Power Management version 2
Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
Status: D0 PME-Enable- DSel=0 DScale=2 PME+
03:01.1 Generic system peripheral [0805]: Ricoh Co Ltd R5C822 SD/SDIO/MMC/MS/MSPro Host Adapter (rev 19) (prog-if 01)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 64
Interrupt: pin B routed to IRQ 23
Region 0: Memory at ef9fd400 (32-bit, non-prefetchable) [size=256]
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=2 PME-
03:01.2 System peripheral: Ricoh Co Ltd Unknown device 0843 (rev 01)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0
Interrupt: pin B routed to IRQ 9
Region 0: Memory at ef9fd500 (32-bit, non-prefetchable) [size=256]
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=2 PME-
03:01.3 System peripheral: Ricoh Co Ltd R5C592 Memory Stick Bus Host Adapter (rev 0a)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Interrupt: pin B routed to IRQ 9
Region 0: Memory at ef9fd600 (32-bit, non-prefetchable) [size=256]
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=2 PME-
03:01.4 System peripheral: Ricoh Co Ltd xD-Picture Card Controller (rev 05)
Subsystem: Dell Unknown device 01cd
Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Interrupt: pin B routed to IRQ 9
Region 0: Memory at ef9fd700 (32-bit, non-prefetchable) [size=256]
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=2 PME-
0c:00.0 Network controller: Intel Corporation PRO/Wireless 3945ABG Network Connection (rev 02)
Subsystem: Intel Corporation Unknown device 1000
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 5
Region 0: Memory at efcff000 (32-bit, non-prefetchable) [size=4K]
Capabilities: [c8] 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: [d0] Message Signalled Interrupts: Mask- 64bit+ Queue=0/0 Enable-
Address: 0000000000000000 Data: 0000
Capabilities: [e0] Express Legacy Endpoint IRQ 0
Device: Supported: MaxPayload 128 bytes, PhantFunc 0, ExtTag-
Device: Latency L0s <512ns, L1 unlimited
Device: AtnBtn- AtnInd- PwrInd-
Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
Device: RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
Device: MaxPayload 128 bytes, MaxReadReq 128 bytes
Link: Supported Speed 2.5Gb/s, Width x1, ASPM L0s L1, Port 0
Link: Latency L0s <128ns, L1 <64us
Link: ASPM L1 Enabled RCB 64 bytes CommClk+ ExtSynch-
Link: Speed 2.5Gb/s, Width x1
Capabilities: [100] Advanced Error Reporting
Capabilities: [140] Device Serial Number 6f-5e-72-ff-ff-d2-19-00
On Tue, 16 Oct 2007 14:39:33 -0400
Mark Lord <[email protected]> wrote:
> I also checked my modprobe.d/ options, and I am using pciehp_force=1.
> Without that flag, none of this ever works.
OK - I suspected something like this. Most Dell computers don't support
ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single
one, they explicitly disable it because they have not validated it or
they have and something didn't work right. I'll take a look at what you've
got, but be aware that you are forcing pciehp to load and operate on a system
where they've certainly either not tested it, or tested it and something
bad happened.
Kristen Carlson Accardi wrote:
> On Tue, 16 Oct 2007 14:39:33 -0400
> Mark Lord <[email protected]> wrote:
>
>> I also checked my modprobe.d/ options, and I am using pciehp_force=1.
>> Without that flag, none of this ever works.
>
> OK - I suspected something like this. Most Dell computers don't support
> ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single
> one, they explicitly disable it because they have not validated it or
> they have and something didn't work right. I'll take a look at what you've
> got, but be aware that you are forcing pciehp to load and operate on a system
> where they've certainly either not tested it, or tested it and something
> bad happened.
Perhaps. But this one works perfectly, except for two driver bugs:
1. Driver does not notice already-inserted cards after modprobe.
2. Driver fails to function after suspend/resume until reloaded.
Both of those are fixable in the kernel.
Cheers
Mark Lord wrote:
> Kristen Carlson Accardi wrote:
>> On Tue, 16 Oct 2007 14:39:33 -0400
>> Mark Lord <[email protected]> wrote:
>>
>>> I also checked my modprobe.d/ options, and I am using pciehp_force=1.
>>> Without that flag, none of this ever works.
>>
>> OK - I suspected something like this. Most Dell computers don't support
>> ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single
>> one, they explicitly disable it because they have not validated it or
>> they have and something didn't work right. I'll take a look at what
>> you've
>> got, but be aware that you are forcing pciehp to load and operate on a
>> system
>> where they've certainly either not tested it, or tested it and something
>> bad happened.
>
> Perhaps. But this one works perfectly, except for two driver bugs:
>
> 1. Driver does not notice already-inserted cards after modprobe.
> 2. Driver fails to function after suspend/resume until reloaded.
>
> Both of those are fixable in the kernel.
Ahh.. point 2 in particular suffers from "suspend/resume" not implemented.
Or rather, implemented as a pair of "do nothing" functions.
Cheers
Mark Lord wrote:
> Mark Lord wrote:
>> Kristen Carlson Accardi wrote:
>>> On Tue, 16 Oct 2007 14:39:33 -0400
>>> Mark Lord <[email protected]> wrote:
>>>
>>>> I also checked my modprobe.d/ options, and I am using pciehp_force=1.
>>>> Without that flag, none of this ever works.
>>>
>>> OK - I suspected something like this. Most Dell computers don't support
>>> ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single
>>> one, they explicitly disable it because they have not validated it or
>>> they have and something didn't work right. I'll take a look at what
>>> you've
>>> got, but be aware that you are forcing pciehp to load and operate on
>>> a system
>>> where they've certainly either not tested it, or tested it and something
>>> bad happened.
>>
>> Perhaps. But this one works perfectly, except for two driver bugs:
>>
>> 1. Driver does not notice already-inserted cards after modprobe.
>> 2. Driver fails to function after suspend/resume until reloaded.
>>
>> Both of those are fixable in the kernel.
>
> Ahh.. point 2 in particular suffers from "suspend/resume" not implemented.
> Or rather, implemented as a pair of "do nothing" functions.
This patch below seems to fix point 1 on my system,
causing pciehp to become aware of already-inserted cards on module load.
It's not perfect, but I believe it does show the kind of functionality
that's missing from the driver.
The resume() function will need something similar, to poll the slots
on resume and call pciehp_enable_slot() or pciehp_disable_slot()
as appropriate.
I suspect this is broken even on machines that do have ACPI BIOS
support.
Cheers
Not for kernel inclusion (yet), but..
Signed-off-by: Mark Lord <[email protected]>
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 15:22:46.000000000 -0400
@@ -475,6 +475,9 @@
rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
if (rc)
goto err_out_free_ctrl_slot;
+ } else {
+ extern int pciehp_enable_slot(struct slot *p_slot);
+ pciehp_enable_slot(t_slot);
}
return 0;
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 15:22:44.000000000 -0400
@@ -37,7 +37,7 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_enable_slot(struct slot *p_slot);
+ int pciehp_enable_slot(struct slot *p_slot);
static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
On Tue, 16 Oct 2007 15:31:29 -0400
Mark Lord <[email protected]> wrote:
> Mark Lord wrote:
> > Mark Lord wrote:
> >> Kristen Carlson Accardi wrote:
> >>> On Tue, 16 Oct 2007 14:39:33 -0400
> >>> Mark Lord <[email protected]> wrote:
> >>>
> >>>> I also checked my modprobe.d/ options, and I am using pciehp_force=1.
> >>>> Without that flag, none of this ever works.
> >>>
> >>> OK - I suspected something like this. Most Dell computers don't support
> >>> ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single
> >>> one, they explicitly disable it because they have not validated it or
> >>> they have and something didn't work right. I'll take a look at what
> >>> you've
> >>> got, but be aware that you are forcing pciehp to load and operate on
> >>> a system
> >>> where they've certainly either not tested it, or tested it and something
> >>> bad happened.
> >>
> >> Perhaps. But this one works perfectly, except for two driver bugs:
> >>
> >> 1. Driver does not notice already-inserted cards after modprobe.
> >> 2. Driver fails to function after suspend/resume until reloaded.
> >>
> >> Both of those are fixable in the kernel.
> >
> > Ahh.. point 2 in particular suffers from "suspend/resume" not implemented.
> > Or rather, implemented as a pair of "do nothing" functions.
>
> This patch below seems to fix point 1 on my system,
> causing pciehp to become aware of already-inserted cards on module load.
>
> It's not perfect, but I believe it does show the kind of functionality
> that's missing from the driver.
No - it's not broken. Powering off the slot if it is not occupied is the
right thing to do - the controller when it is working properly will detect
the presence of a new adapter and interrupt.
I'll try to duplicate your problem on a piece of hardware that has proper
firmware support and validated hardware and then we'll go from there.
We could very well have software problems, especially with ExpressCard
since most pciehp use is for servers, but we should make sure we aren't
writing workarounds for broken hardware first.
On Tue, 16 Oct 2007 15:31:29 -0400
Mark Lord <[email protected]> wrote:
> Mark Lord wrote:
> > Mark Lord wrote:
> >> Kristen Carlson Accardi wrote:
> >>> On Tue, 16 Oct 2007 14:39:33 -0400
> >>> Mark Lord <[email protected]> wrote:
> >>>
> >>>> I also checked my modprobe.d/ options, and I am using pciehp_force=1.
> >>>> Without that flag, none of this ever works.
> >>>
> >>> OK - I suspected something like this. Most Dell computers don't support
> >>> ExpressCard hotplug using Native PCIe -- in fact, I've not seen a single
> >>> one, they explicitly disable it because they have not validated it or
> >>> they have and something didn't work right. I'll take a look at what
> >>> you've
> >>> got, but be aware that you are forcing pciehp to load and operate on
> >>> a system
> >>> where they've certainly either not tested it, or tested it and something
> >>> bad happened.
> >>
> >> Perhaps. But this one works perfectly, except for two driver bugs:
> >>
> >> 1. Driver does not notice already-inserted cards after modprobe.
> >> 2. Driver fails to function after suspend/resume until reloaded.
> >>
> >> Both of those are fixable in the kernel.
> >
> > Ahh.. point 2 in particular suffers from "suspend/resume" not implemented.
> > Or rather, implemented as a pair of "do nothing" functions.
>
I tried to reproduce this on a Lenovo T61, which does have proper firmware
support for _OSC, and also has been validated, and the driver which is
in 2.6.23-git8 seems to work fine, even across suspend resume. I suspect
that your system just doesn't support pcie hotplug properly.
You might try getting a BIOS update from Dell.
On Tue, 16 Oct 2007 14:39:33 -0400
Mark Lord <[email protected]> wrote:
> Another thing: if a card is already in the slot before pciehp is loaded
> (under any circumstances), then pciehp does *not* see the card until I
> unplug/replug it.
One other thing to note here, the proper operation of hot plug dictates that
if you boot your laptop with no adapter in the slot, you must first load
the driver in order for the new adapter interrupt to be captured. So,
if you insert a card without the driver loaded, it will not be enumerated
by PCI until you remove it and the reinsert it.
Kristen Carlson Accardi wrote:
>...
> I tried to reproduce this on a Lenovo T61, which does have proper firmware
> support for _OSC, and also has been validated, and the driver which is
> in 2.6.23-git8 seems to work fine, even across suspend resume. I suspect
> that your system just doesn't support pcie hotplug properly.
No, the hardware seems to work perfectly.
We just have a software issue. We *know* it's only software
because rmmod+modprobe fixes things, without any hardware intervention.
I believe the code is leaning too heavily on the BIOS for stuff,
and like lots of other parts of the kernel we'll need an alternate
strategy for when things aren't "perfect".
> You might try getting a BIOS update from Dell.
The machine already has the latest BIOS, thanks.
Cheers
Kristen Carlson Accardi wrote:
> On Tue, 16 Oct 2007 14:39:33 -0400
> Mark Lord <[email protected]> wrote:
>
>> Another thing: if a card is already in the slot before pciehp is loaded
>> (under any circumstances), then pciehp does *not* see the card until I
>> unplug/replug it.
You mean that the existing code does not handle it properly.
The hardware can see the card just fine, as reflected in the slot
status bits.
> One other thing to note here, the proper operation of hot plug dictates that
> if you boot your laptop with no adapter in the slot, you must first load
> the driver in order for the new adapter interrupt to be captured. So,
> if you insert a card without the driver loaded, it will not be enumerated
> by PCI until you remove it and the reinsert it.
Bull puckey.
If there's already a card in the slot when we turn on the power,
the Linux kernel should find and use that card.
This is flawed, and needs fixing.
Cheers
On Tue, 16 Oct 2007 16:39:25 -0400
Mark Lord <[email protected]> wrote:
> Kristen Carlson Accardi wrote:
> >...
> > I tried to reproduce this on a Lenovo T61, which does have proper firmware
> > support for _OSC, and also has been validated, and the driver which is
> > in 2.6.23-git8 seems to work fine, even across suspend resume. I suspect
> > that your system just doesn't support pcie hotplug properly.
>
> No, the hardware seems to work perfectly.
>
> We just have a software issue. We *know* it's only software
> because rmmod+modprobe fixes things, without any hardware intervention.
This reinitializes the controller, which is probably why things work
better for you (which does of course touch hardware...).
>
> I believe the code is leaning too heavily on the BIOS for stuff,
> and like lots of other parts of the kernel we'll need an alternate
> strategy for when things aren't "perfect".
the pitfall for forcing pciehp when the BIOS hasn't provided OSC is that
you don't know for sure that you really have gained control of hot plug
operation properly. You can obviously try it, using the provide forcing
option as you have done, but the behavior is not predictable.
On Tue, Oct 16, 2007 at 04:39:25PM -0400, Mark Lord wrote:
> Kristen Carlson Accardi wrote:
> >...
> > I tried to reproduce this on a Lenovo T61, which does have proper firmware
> > support for _OSC, and also has been validated, and the driver which is
> > in 2.6.23-git8 seems to work fine, even across suspend resume. I suspect
> > that your system just doesn't support pcie hotplug properly.
>
> No, the hardware seems to work perfectly.
>
> We just have a software issue. We *know* it's only software
> because rmmod+modprobe fixes things, without any hardware intervention.
>
> I believe the code is leaning too heavily on the BIOS for stuff,
> and like lots of other parts of the kernel we'll need an alternate
> strategy for when things aren't "perfect".
>
> > You might try getting a BIOS update from Dell.
>
> The machine already has the latest BIOS, thanks.
Dell laptops are known to have broken BIOS acpi code in regards to pci
express hotplug issues. Seriously, we rely on acpi here for a lot of
this, and unless it is properly set up, it will not work.
And this is broken because Windows can't even handle this kind of thing,
so it isn't tested :(
So a short note to Dell might be helpful, although in the end, I think
this looks like a kernel issue as you have proven with your patch.
thanks,
greg k-h
Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
in conjunction with modparam of pciehp_force=1.
The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
which lack ACPI BIOS support for PCIe hotplug:
1. The driver does not recognise cards that were inserted prior
to the driver being modprobe'd.
2. The driver stops functioning after a suspend/resume (RAM) cycle,
and needs to be rmmod'd and modprobe'd to get it working again.
This patch addresses both issues.
Issue 1 is fixed by modifying pciehp_probe() to either enable or disable
the slot based on whether or not a card is detected at module init time.
Issue 2 is fixed by implementing pciehp_resume() to do the same
after having it first reinitialize the slot event registers.
The code to reinitialize those registers has been broken away from
pcie_init() so that it can be shared with pciehp_resume().
I'm not certain that locking contraints are correct in pciehp_resume(),
so it would be quite useful for the PCIe hotplug folks to look this all
over and provide suggestions/fixes as needed.
There are also a few minor cosmetic changes to satisfy checkpatch.pl.
pciehp.h | 3
pciehp_core.c | 31 ++++++--
pciehp_ctrl.c | 4 -
pciehp_hpc.c | 207 +++++++++++++++++++++++++++++++++-------------------------
4 files changed, 144 insertions(+), 101 deletions(-)
Signed-off-by: Mark Lord <[email protected]>
---
--- old/drivers/pci/hotplug/pciehp.h 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 17:53:48.000000000 -0400
@@ -161,6 +161,9 @@
extern int pciehp_unconfigure_device(struct slot *p_slot);
extern void pciehp_queue_pushbutton_work(struct work_struct *work);
int pcie_init(struct controller *ctrl, struct pcie_device *dev);
+int pciehp_enable_slot(struct slot *p_slot);
+int pciehp_disable_slot(struct slot *p_slot);
+int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev);
static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
{
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 17:48:24.000000000 -0400
@@ -470,17 +470,14 @@
t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
- t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */
- if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
- rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
- if (rc)
- goto err_out_free_ctrl_slot;
- }
-
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &value);
+ if (value)
+ pciehp_enable_slot(t_slot);
+ else
+ pciehp_disable_slot(t_slot);
return 0;
-err_out_free_ctrl_slot:
- cleanup_slots(ctrl);
err_out_release_ctlr:
ctrl->hpc_ops->release_ctlr(ctrl);
err_out_free_ctrl:
@@ -508,7 +505,23 @@
static int pciehp_resume (struct pcie_device *dev)
{
+ struct pci_dev *pdev = dev->port;
+ struct controller *ctrl = pci_get_drvdata(pdev);
+ struct slot *t_slot;
+ u8 status;
+
printk("%s ENTRY\n", __FUNCTION__);
+
+ pcie_init_enable_events(ctrl, dev);
+
+ t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
+
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &status);
+ if (status)
+ pciehp_enable_slot(t_slot);
+ else
+ pciehp_disable_slot(t_slot);
return 0;
}
#endif
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 16:53:35.000000000 -0400
@@ -37,8 +37,6 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_enable_slot(struct slot *p_slot);
-static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
{
@@ -520,7 +518,7 @@
case INT_PRESENCE_OFF:
if (!HP_SUPR_RM(ctrl->ctrlcap))
break;
- dbg("Surprise Removal\n");
+ dbg("Surprise Event\n");
update_slot_info(p_slot);
handle_surprise_event(p_slot);
break;
--- old/drivers/pci/hotplug/pciehp_hpc.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 17:53:27.000000000 -0400
@@ -1163,100 +1163,23 @@
}
#endif
-
-
-int pcie_init(struct controller * ctrl, struct pcie_device *dev)
+int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev)
{
int rc;
u16 temp_word;
- u16 cap_reg;
u16 intr_enable = 0;
u32 slot_cap;
- int cap_base;
- u16 slot_status, slot_ctrl;
+ u16 slot_status;
struct pci_dev *pdev;
DBG_ENTER_ROUTINE
-
- pdev = dev->port;
- ctrl->pci_dev = pdev; /* save pci_dev in context */
-
- dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
- __FUNCTION__, pdev->vendor, pdev->device);
-
- if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
- dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
- ctrl->cap_base = cap_base;
-
- dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
-
- rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
- if (rc) {
- err("%s: Cannot read CAPREG register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: CAPREG offset %x cap_reg %x\n",
- __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
-
- if (((cap_reg & SLOT_IMPL) == 0) || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
- && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port or the port is not connected to a slot\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
+ pdev = dev->port;
rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
if (rc) {
err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
goto abort_free_ctlr;
}
- dbg("%s: SLOTCAP offset %x slot_cap %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
-
- if (!(slot_cap & HP_CAP)) {
- dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- /* For debugging purpose */
- rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
- if (rc) {
- err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
-
- rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
- if (rc) {
- err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
-
- for ( rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
- if (pci_resource_len(pdev, rc) > 0)
- dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
- (unsigned long long)pci_resource_start(pdev, rc),
- (unsigned long long)pci_resource_len(pdev, rc));
-
- info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n", pdev->vendor, pdev->device,
- pdev->subsystem_vendor, pdev->subsystem_device);
-
- mutex_init(&ctrl->crit_sect);
- mutex_init(&ctrl->ctrl_lock);
- spin_lock_init(&ctrl->lock);
-
- /* setup wait queue */
- init_waitqueue_head(&ctrl->queue);
-
- /* return PCI Controller Info */
- ctrl->slot_device_offset = 0;
- ctrl->num_slots = 1;
- ctrl->first_slot = slot_cap >> 19;
- ctrl->ctrlcap = slot_cap & 0x0000007f;
/* Mask Hot-plug Interrupt Enable */
rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
@@ -1267,7 +1190,7 @@
dbg("%s: SLOTCTRL %x value read %x\n",
__FUNCTION__, ctrl->cap_base + SLOTCTRL, temp_word);
- temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE) | 0x00;
+ temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE)|0x00;
rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
if (rc) {
@@ -1330,14 +1253,14 @@
if (ATTN_BUTTN(slot_cap))
intr_enable = intr_enable | ATTN_BUTTN_ENABLE;
-
+
if (POWER_CTRL(slot_cap))
intr_enable = intr_enable | PWR_FAULT_DETECT_ENABLE;
-
+
if (MRL_SENS(slot_cap))
intr_enable = intr_enable | MRL_DETECT_ENABLE;
- temp_word = (temp_word & ~intr_enable) | intr_enable;
+ temp_word = (temp_word & ~intr_enable) | intr_enable;
if (pciehp_poll_mode) {
temp_word = (temp_word & ~HP_INTR_ENABLE) | 0x0;
@@ -1345,7 +1268,7 @@
temp_word = (temp_word & ~HP_INTR_ENABLE) | HP_INTR_ENABLE;
}
- /* Unmask Hot-plug Interrupt Enable for the interrupt notification mechanism case */
+ /* Unmask hp Interrupt Enable for intr notification mechanism case */
rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
if (rc) {
err("%s: Cannot write to SLOTCTRL register\n", __FUNCTION__);
@@ -1356,14 +1279,14 @@
err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
goto abort_disable_intr;
}
-
+
temp_word = 0x1F; /* Clear all events */
rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
if (rc) {
err("%s: Cannot write to SLOTSTATUS register\n", __FUNCTION__);
goto abort_disable_intr;
}
-
+
if (pciehp_force) {
dbg("Bypassing BIOS check for pciehp use on %s\n",
pci_name(ctrl->pci_dev));
@@ -1373,8 +1296,6 @@
goto abort_disable_intr;
}
- ctrl->hpc_ops = &pciehp_hpc_ops;
-
DBG_LEAVE_ROUTINE
return 0;
@@ -1398,3 +1319,111 @@
DBG_LEAVE_ROUTINE
return -1;
}
+
+int pcie_init(struct controller *ctrl, struct pcie_device *dev)
+{
+ int rc;
+ u16 cap_reg;
+ u32 slot_cap;
+ int cap_base;
+ u16 slot_status, slot_ctrl;
+ struct pci_dev *pdev;
+
+ DBG_ENTER_ROUTINE
+
+ pdev = dev->port;
+ ctrl->pci_dev = pdev; /* save pci_dev in context */
+
+ dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
+ __FUNCTION__, pdev->vendor, pdev->device);
+
+ cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+ if (cap_base == 0) {
+ dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+
+ ctrl->cap_base = cap_base;
+
+ dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
+
+ rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
+ if (rc) {
+ err("%s: Cannot read CAPREG register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: CAPREG offset %x cap_reg %x\n",
+ __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
+
+ if (((cap_reg & SLOT_IMPL) == 0)
+ || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
+ && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
+ dbg("%s : This is not a root port"
+ " or the port is not connected to a slot\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+
+ rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
+ if (rc) {
+ err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: SLOTCAP offset %x slot_cap %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
+
+ if (!(slot_cap & HP_CAP)) {
+ dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ /* For debugging purpose */
+ rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
+ if (rc) {
+ err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
+
+ rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
+ if (rc) {
+ err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
+
+ for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
+ if (pci_resource_len(pdev, rc) > 0)
+ dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
+ (unsigned long long)pci_resource_start(pdev, rc),
+ (unsigned long long)pci_resource_len(pdev, rc));
+
+ info("HPC vendor_id %x device_id %x ss_vid %x"
+ " ss_did %x\n", pdev->vendor, pdev->device,
+ pdev->subsystem_vendor, pdev->subsystem_device);
+
+ mutex_init(&ctrl->crit_sect);
+ mutex_init(&ctrl->ctrl_lock);
+ spin_lock_init(&ctrl->lock);
+
+ /* setup wait queue */
+ init_waitqueue_head(&ctrl->queue);
+
+ /* return PCI Controller Info */
+ ctrl->slot_device_offset = 0;
+ ctrl->num_slots = 1;
+ ctrl->first_slot = slot_cap >> 19;
+ ctrl->ctrlcap = slot_cap & 0x0000007f;
+
+ rc = pcie_init_enable_events(ctrl, dev);
+ if (rc)
+ goto abort_free_ctlr;;
+
+ ctrl->hpc_ops = &pciehp_hpc_ops;
+
+ DBG_LEAVE_ROUTINE
+ return 0;
+abort_free_ctlr:
+ DBG_LEAVE_ROUTINE
+ return -1;
+}
On Tue, 16 Oct 2007 17:57:03 -0400
Mark Lord <[email protected]> wrote:
> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> in conjunction with modparam of pciehp_force=1.
Please resubmit, breaking this patch into 3 separate patches
1 for your first issue you wish to address, 2 for the second, and 3 for
cosmetic changes.
Thanks,
Kristen
>
> The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
> which lack ACPI BIOS support for PCIe hotplug:
>
> 1. The driver does not recognise cards that were inserted prior
> to the driver being modprobe'd.
>
> 2. The driver stops functioning after a suspend/resume (RAM) cycle,
> and needs to be rmmod'd and modprobe'd to get it working again.
>
> This patch addresses both issues.
>
> Issue 1 is fixed by modifying pciehp_probe() to either enable or disable
> the slot based on whether or not a card is detected at module init time.
>
> Issue 2 is fixed by implementing pciehp_resume() to do the same
> after having it first reinitialize the slot event registers.
>
> The code to reinitialize those registers has been broken away from
> pcie_init() so that it can be shared with pciehp_resume().
>
> I'm not certain that locking contraints are correct in pciehp_resume(),
> so it would be quite useful for the PCIe hotplug folks to look this all
> over and provide suggestions/fixes as needed.
>
> There are also a few minor cosmetic changes to satisfy checkpatch.pl.
>
> pciehp.h | 3
> pciehp_core.c | 31 ++++++--
> pciehp_ctrl.c | 4 -
> pciehp_hpc.c | 207 +++++++++++++++++++++++++++++++++-------------------------
> 4 files changed, 144 insertions(+), 101 deletions(-)
>
> Signed-off-by: Mark Lord <[email protected]>
> ---
>
> --- old/drivers/pci/hotplug/pciehp.h 2007-10-12 12:43:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 17:53:48.000000000 -0400
> @@ -161,6 +161,9 @@
> extern int pciehp_unconfigure_device(struct slot *p_slot);
> extern void pciehp_queue_pushbutton_work(struct work_struct *work);
> int pcie_init(struct controller *ctrl, struct pcie_device *dev);
> +int pciehp_enable_slot(struct slot *p_slot);
> +int pciehp_disable_slot(struct slot *p_slot);
> +int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev);
>
> static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
> {
> --- old/drivers/pci/hotplug/pciehp_core.c 2007-10-12 12:43:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 17:48:24.000000000 -0400
> @@ -470,17 +470,14 @@
>
> t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
>
> - t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */
> - if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
> - rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
> - if (rc)
> - goto err_out_free_ctrl_slot;
> - }
> -
> + /* Check if slot is occupied */
> + t_slot->hpc_ops->get_adapter_status(t_slot, &value);
> + if (value)
> + pciehp_enable_slot(t_slot);
> + else
> + pciehp_disable_slot(t_slot);
> return 0;
>
> -err_out_free_ctrl_slot:
> - cleanup_slots(ctrl);
> err_out_release_ctlr:
> ctrl->hpc_ops->release_ctlr(ctrl);
> err_out_free_ctrl:
> @@ -508,7 +505,23 @@
>
> static int pciehp_resume (struct pcie_device *dev)
> {
> + struct pci_dev *pdev = dev->port;
> + struct controller *ctrl = pci_get_drvdata(pdev);
> + struct slot *t_slot;
> + u8 status;
> +
> printk("%s ENTRY\n", __FUNCTION__);
> +
> + pcie_init_enable_events(ctrl, dev);
> +
> + t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
> +
> + /* Check if slot is occupied */
> + t_slot->hpc_ops->get_adapter_status(t_slot, &status);
> + if (status)
> + pciehp_enable_slot(t_slot);
> + else
> + pciehp_disable_slot(t_slot);
> return 0;
> }
> #endif
> --- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-12 12:43:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 16:53:35.000000000 -0400
> @@ -37,8 +37,6 @@
> #include "pciehp.h"
>
> static void interrupt_event_handler(struct work_struct *work);
> -static int pciehp_enable_slot(struct slot *p_slot);
> -static int pciehp_disable_slot(struct slot *p_slot);
>
> static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
> {
> @@ -520,7 +518,7 @@
> case INT_PRESENCE_OFF:
> if (!HP_SUPR_RM(ctrl->ctrlcap))
> break;
> - dbg("Surprise Removal\n");
> + dbg("Surprise Event\n");
> update_slot_info(p_slot);
> handle_surprise_event(p_slot);
> break;
> --- old/drivers/pci/hotplug/pciehp_hpc.c 2007-10-12 12:43:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 17:53:27.000000000 -0400
> @@ -1163,100 +1163,23 @@
> }
> #endif
>
> -
> -
> -int pcie_init(struct controller * ctrl, struct pcie_device *dev)
> +int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev)
> {
> int rc;
> u16 temp_word;
> - u16 cap_reg;
> u16 intr_enable = 0;
> u32 slot_cap;
> - int cap_base;
> - u16 slot_status, slot_ctrl;
> + u16 slot_status;
> struct pci_dev *pdev;
>
> DBG_ENTER_ROUTINE
> -
> - pdev = dev->port;
> - ctrl->pci_dev = pdev; /* save pci_dev in context */
> -
> - dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
> - __FUNCTION__, pdev->vendor, pdev->device);
> -
> - if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
> - dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
> - goto abort_free_ctlr;
> - }
> -
> - ctrl->cap_base = cap_base;
> -
> - dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
> -
> - rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
> - if (rc) {
> - err("%s: Cannot read CAPREG register\n", __FUNCTION__);
> - goto abort_free_ctlr;
> - }
> - dbg("%s: CAPREG offset %x cap_reg %x\n",
> - __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
> -
> - if (((cap_reg & SLOT_IMPL) == 0) || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
> - && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
> - dbg("%s : This is not a root port or the port is not connected to a slot\n", __FUNCTION__);
> - goto abort_free_ctlr;
> - }
>
> + pdev = dev->port;
> rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
> if (rc) {
> err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
> goto abort_free_ctlr;
> }
> - dbg("%s: SLOTCAP offset %x slot_cap %x\n",
> - __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
> -
> - if (!(slot_cap & HP_CAP)) {
> - dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
> - goto abort_free_ctlr;
> - }
> - /* For debugging purpose */
> - rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
> - if (rc) {
> - err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
> - goto abort_free_ctlr;
> - }
> - dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
> - __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
> -
> - rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
> - if (rc) {
> - err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
> - goto abort_free_ctlr;
> - }
> - dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
> - __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
> -
> - for ( rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
> - if (pci_resource_len(pdev, rc) > 0)
> - dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
> - (unsigned long long)pci_resource_start(pdev, rc),
> - (unsigned long long)pci_resource_len(pdev, rc));
> -
> - info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n", pdev->vendor, pdev->device,
> - pdev->subsystem_vendor, pdev->subsystem_device);
> -
> - mutex_init(&ctrl->crit_sect);
> - mutex_init(&ctrl->ctrl_lock);
> - spin_lock_init(&ctrl->lock);
> -
> - /* setup wait queue */
> - init_waitqueue_head(&ctrl->queue);
> -
> - /* return PCI Controller Info */
> - ctrl->slot_device_offset = 0;
> - ctrl->num_slots = 1;
> - ctrl->first_slot = slot_cap >> 19;
> - ctrl->ctrlcap = slot_cap & 0x0000007f;
>
> /* Mask Hot-plug Interrupt Enable */
> rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
> @@ -1267,7 +1190,7 @@
>
> dbg("%s: SLOTCTRL %x value read %x\n",
> __FUNCTION__, ctrl->cap_base + SLOTCTRL, temp_word);
> - temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE) | 0x00;
> + temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE)|0x00;
>
> rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
> if (rc) {
> @@ -1330,14 +1253,14 @@
>
> if (ATTN_BUTTN(slot_cap))
> intr_enable = intr_enable | ATTN_BUTTN_ENABLE;
> -
> +
> if (POWER_CTRL(slot_cap))
> intr_enable = intr_enable | PWR_FAULT_DETECT_ENABLE;
> -
> +
> if (MRL_SENS(slot_cap))
> intr_enable = intr_enable | MRL_DETECT_ENABLE;
>
> - temp_word = (temp_word & ~intr_enable) | intr_enable;
> + temp_word = (temp_word & ~intr_enable) | intr_enable;
>
> if (pciehp_poll_mode) {
> temp_word = (temp_word & ~HP_INTR_ENABLE) | 0x0;
> @@ -1345,7 +1268,7 @@
> temp_word = (temp_word & ~HP_INTR_ENABLE) | HP_INTR_ENABLE;
> }
>
> - /* Unmask Hot-plug Interrupt Enable for the interrupt notification mechanism case */
> + /* Unmask hp Interrupt Enable for intr notification mechanism case */
> rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
> if (rc) {
> err("%s: Cannot write to SLOTCTRL register\n", __FUNCTION__);
> @@ -1356,14 +1279,14 @@
> err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
> goto abort_disable_intr;
> }
> -
> +
> temp_word = 0x1F; /* Clear all events */
> rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
> if (rc) {
> err("%s: Cannot write to SLOTSTATUS register\n", __FUNCTION__);
> goto abort_disable_intr;
> }
> -
> +
> if (pciehp_force) {
> dbg("Bypassing BIOS check for pciehp use on %s\n",
> pci_name(ctrl->pci_dev));
> @@ -1373,8 +1296,6 @@
> goto abort_disable_intr;
> }
>
> - ctrl->hpc_ops = &pciehp_hpc_ops;
> -
> DBG_LEAVE_ROUTINE
> return 0;
>
> @@ -1398,3 +1319,111 @@
> DBG_LEAVE_ROUTINE
> return -1;
> }
> +
> +int pcie_init(struct controller *ctrl, struct pcie_device *dev)
> +{
> + int rc;
> + u16 cap_reg;
> + u32 slot_cap;
> + int cap_base;
> + u16 slot_status, slot_ctrl;
> + struct pci_dev *pdev;
> +
> + DBG_ENTER_ROUTINE
> +
> + pdev = dev->port;
> + ctrl->pci_dev = pdev; /* save pci_dev in context */
> +
> + dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
> + __FUNCTION__, pdev->vendor, pdev->device);
> +
> + cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP);
> + if (cap_base == 0) {
> + dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> +
> + ctrl->cap_base = cap_base;
> +
> + dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
> +
> + rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
> + if (rc) {
> + err("%s: Cannot read CAPREG register\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> + dbg("%s: CAPREG offset %x cap_reg %x\n",
> + __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
> +
> + if (((cap_reg & SLOT_IMPL) == 0)
> + || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
> + && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
> + dbg("%s : This is not a root port"
> + " or the port is not connected to a slot\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> +
> + rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
> + if (rc) {
> + err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> + dbg("%s: SLOTCAP offset %x slot_cap %x\n",
> + __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
> +
> + if (!(slot_cap & HP_CAP)) {
> + dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> + /* For debugging purpose */
> + rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
> + if (rc) {
> + err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> + dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
> + __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
> +
> + rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
> + if (rc) {
> + err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
> + goto abort_free_ctlr;
> + }
> + dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
> + __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
> +
> + for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
> + if (pci_resource_len(pdev, rc) > 0)
> + dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
> + (unsigned long long)pci_resource_start(pdev, rc),
> + (unsigned long long)pci_resource_len(pdev, rc));
> +
> + info("HPC vendor_id %x device_id %x ss_vid %x"
> + " ss_did %x\n", pdev->vendor, pdev->device,
> + pdev->subsystem_vendor, pdev->subsystem_device);
> +
> + mutex_init(&ctrl->crit_sect);
> + mutex_init(&ctrl->ctrl_lock);
> + spin_lock_init(&ctrl->lock);
> +
> + /* setup wait queue */
> + init_waitqueue_head(&ctrl->queue);
> +
> + /* return PCI Controller Info */
> + ctrl->slot_device_offset = 0;
> + ctrl->num_slots = 1;
> + ctrl->first_slot = slot_cap >> 19;
> + ctrl->ctrlcap = slot_cap & 0x0000007f;
> +
> + rc = pcie_init_enable_events(ctrl, dev);
> + if (rc)
> + goto abort_free_ctlr;;
> +
> + ctrl->hpc_ops = &pciehp_hpc_ops;
> +
> + DBG_LEAVE_ROUTINE
> + return 0;
> +abort_free_ctlr:
> + DBG_LEAVE_ROUTINE
> + return -1;
> +}
>
On Tue, Oct 16, 2007 at 05:57:03PM -0400, Mark Lord wrote:
> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> in conjunction with modparam of pciehp_force=1.
>
> The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
> which lack ACPI BIOS support for PCIe hotplug:
Wait, Dell explicitly says that pci hotplug of express cards is not
supported and is broken on these laptops. This is because the version
of Windows they support on these machines also does not support
hotplugging these devices.
The current code works just fine on hardware that actually supports this
kind of functionality, as per the proper specs and requirements for this
feature.
So why try to go through these gyrations for hardware that is explicitly
broken?
thanks,
greg k-h
Kristen Carlson Accardi wrote:
> On Tue, 16 Oct 2007 17:57:03 -0400
> Mark Lord <[email protected]> wrote:
>
>> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
>> in conjunction with modparam of pciehp_force=1.
>
> Please resubmit, breaking this patch into 3 separate patches
> 1 for your first issue you wish to address, 2 for the second, and 3 for
> cosmetic changes.
It'll have to be the other way around, then.
The first patch will be one to fix the broken whitespace
and lines > 80 characters that already exist in the code
being touched. Otherwise *none* of the patches pass checkpatch.pl paranoia.
Subsequent patches will then follow for the two issues.
Cheers
Greg KH wrote:
> On Tue, Oct 16, 2007 at 05:57:03PM -0400, Mark Lord wrote:
>> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
>> in conjunction with modparam of pciehp_force=1.
>>
>> The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
>> which lack ACPI BIOS support for PCIe hotplug:
>
> Wait, Dell explicitly says that pci hotplug of express cards is not
> supported and is broken on these laptops. This is because the version
> of Windows they support on these machines also does not support
> hotplugging these devices.
>
> The current code works just fine on hardware that actually supports this
> kind of functionality, as per the proper specs and requirements for this
> feature.
>
> So why try to go through these gyrations for hardware that is explicitly
> broken?
Because it is NOT broken. It works perfectly.
There's just a couple of issues in the existing *working* Linux driver
that need fixing, that's all.
Cheers
On Tue, Oct 16, 2007 at 02:01:16PM -0700, Kristen Carlson Accardi wrote:
> >
> > I believe the code is leaning too heavily on the BIOS for stuff,
> > and like lots of other parts of the kernel we'll need an alternate
> > strategy for when things aren't "perfect".
>
> the pitfall for forcing pciehp when the BIOS hasn't provided OSC is that
> you don't know for sure that you really have gained control of hot plug
> operation properly. You can obviously try it, using the provide forcing
> option as you have done, but the behavior is not predictable.
The bigger concern is whether this likely to break things on systems
that *do* correctly implement ACPI support for PCIe hotplug?
- Ted
On Tue, 16 Oct 2007 15:03:54 -0700
Greg KH <[email protected]> wrote:
> On Tue, Oct 16, 2007 at 05:57:03PM -0400, Mark Lord wrote:
> > Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> > in conjunction with modparam of pciehp_force=1.
> >
> > The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
> > which lack ACPI BIOS support for PCIe hotplug:
>
> Wait, Dell explicitly says that pci hotplug of express cards is not
> supported and is broken on these laptops. This is because the version
> of Windows they support on these machines also does not support
> hotplugging these devices.
Just to add to why sometimes vendors disable PCIe hotplug by not providing
_OSC - sometimes there are problems with the hardware itself, and so
due to schedule and other reasons, vendors will decide to just disable
the support by not providing OSC, since really according to spec we shouldn't
be running the PCIe hot plug driver if OSC doesn't exist or fails. This
can be anything from "Well, we don't have time to test it" to actual hardware
errata. At any rate, we don't know the reasons. I think when we force these
things, you run the danger that things will break on you in some strange
ways.
Just out of curiosity, did you try using ACPI based hotplug for this
laptop? Sometimes vendors will let you do that because then they can include
hardware workarounds in the AML.
Original single patch is now broken out into tiny pieces for easier review.
Also, valuable feedback from Ted has been incorporated,
to avoid any possible side effects on regular use of
the PCIe hotplug stuff (when used without pciehp_force=1 mod parm).
* * *
Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
in conjunction with the modparam of pciehp_force=1.
The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
which lack ACPI BIOS support for PCIe hotplug:
1. The driver does not recognise cards that were inserted prior
to the driver being modprobe'd.
2. The driver stops functioning after a suspend/resume (RAM) cycle,
and needs to be rmmod'd and modprobe'd to get it working again.
This patch series addresses those issues, resulting in a completely
functional PCIe Hotplug driver for Dell notebooks, and probably others
as well, which may lack ACPI BIOS support for this.
There are four patches in this series:
01_pciehp_cosmetic_fixes.patch
-- cosmetic fixes, mostly to keep checkpatch.pl happy
02_pciehp_handle_preinserted_card.patch
-- fixes problem number 1 (above).
03_pciehp_split_pcie_init.patch
-- preparation for the resume patch.
04_pciehp_resume.patch
-- fixes problem number 2 (above).
diffstat summary for 01_pciehp_cosmetic_fixes.patch only:
drivers/pci/hotplug/pciehp_core.c | 6 -
drivers/pci/hotplug/pciehp_ctrl.c | 2
drivers/pci/hotplug/pciehp_hpc.c | 119 ++++++++++++++--------------
3 files changed, 65 insertions(+), 62 deletions(-)
diffstat summary for the other three patches combined:
drivers/pci/hotplug/pciehp.h | 3
drivers/pci/hotplug/pciehp_core.c | 21 ++-
drivers/pci/hotplug/pciehp_ctrl.c | 2
drivers/pci/hotplug/pciehp_hpc.c | 194 ++++++++++++++++------------
4 files changed, 134 insertions(+), 86 deletions(-)
Cheers
--
Mark Lord <[email protected]>
Whitespace and other cosmetic fixes so that checkpatch.pl
is happy with the remainder of patches in this series.
Signed-off-by: Mark Lord <[email protected]>
---
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:14:03.000000000 -0400
@@ -470,9 +470,11 @@
t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
- t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &value);
if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
- rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
+ /* Power off slot if not occupied*/
+ rc = t_slot->hpc_ops->power_off_slot(t_slot);
if (rc)
goto err_out_free_ctrl_slot;
}
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:12:54.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:08:18.000000000 -0400
@@ -520,7 +520,7 @@
case INT_PRESENCE_OFF:
if (!HP_SUPR_RM(ctrl->ctrlcap))
break;
- dbg("Surprise Removal\n");
+ dbg("Surprise Event\n");
update_slot_info(p_slot);
handle_surprise_event(p_slot);
break;
--- old/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:12:54.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:13:32.000000000 -0400
@@ -160,10 +160,10 @@
/* Link Width Encoding */
#define LNK_X1 0x01
#define LNK_X2 0x02
-#define LNK_X4 0x04
+#define LNK_X4 0x04
#define LNK_X8 0x08
#define LNK_X12 0x0C
-#define LNK_X16 0x10
+#define LNK_X16 0x10
#define LNK_X32 0x20
/*Field definitions of Link Status Register */
@@ -289,7 +289,7 @@
u16 slot_ctrl;
unsigned long flags;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
mutex_lock(&ctrl->ctrl_lock);
@@ -299,7 +299,7 @@
goto out;
}
- if ((slot_status & CMD_COMPLETED) == CMD_COMPLETED ) {
+ if ((slot_status & CMD_COMPLETED) == CMD_COMPLETED) {
/* After 1 sec and CMD_COMPLETED still not set, just
proceed forward to issue the next command according
to spec. Just print out the error message */
@@ -332,7 +332,7 @@
retval = pcie_wait_cmd(ctrl);
out:
mutex_unlock(&ctrl->ctrl_lock);
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -341,7 +341,7 @@
u16 lnk_status;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, LNKSTATUS, &lnk_status);
if (retval) {
@@ -350,14 +350,14 @@
}
dbg("%s: lnk_status = %x\n", __FUNCTION__, lnk_status);
- if ( (lnk_status & LNK_TRN) || (lnk_status & LNK_TRN_ERR) ||
+ if ((lnk_status & LNK_TRN) || (lnk_status & LNK_TRN_ERR) ||
!(lnk_status & NEG_LINK_WD)) {
err("%s : Link Training Error occurs \n", __FUNCTION__);
retval = -1;
return retval;
}
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -368,8 +368,8 @@
u16 slot_ctrl;
u8 atten_led_state;
int retval = 0;
-
- DBG_ENTER_ROUTINE
+
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
if (retval) {
@@ -400,7 +400,7 @@
break;
}
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return 0;
}
@@ -410,8 +410,8 @@
u16 slot_ctrl;
u8 pwr_state;
int retval = 0;
-
- DBG_ENTER_ROUTINE
+
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
if (retval) {
@@ -428,14 +428,14 @@
*status = 1;
break;
case 1:
- *status = 0;
+ *status = 0;
break;
default:
*status = 0xFF;
break;
}
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -446,7 +446,7 @@
u16 slot_status;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
if (retval) {
@@ -454,9 +454,9 @@
return retval;
}
- *status = (((slot_status & MRL_STATE) >> 5) == 0) ? 0 : 1;
+ *status = (((slot_status & MRL_STATE) >> 5) == 0) ? 0 : 1;
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return 0;
}
@@ -467,7 +467,7 @@
u8 card_state;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
if (retval) {
@@ -477,7 +477,7 @@
card_state = (u8)((slot_status & PRSN_STATE) >> 6);
*status = (card_state == 1) ? 1 : 0;
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return 0;
}
@@ -488,7 +488,7 @@
u8 pwr_fault;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
if (retval) {
@@ -496,7 +496,7 @@
return retval;
}
pwr_fault = (u8)((slot_status & PWR_FAULT_DETECTED) >> 1);
-
+
DBG_LEAVE_ROUTINE
return pwr_fault;
}
@@ -572,7 +572,7 @@
rc = pcie_write_cmd(slot, slot_cmd, cmd_mask);
dbg("%s: SLOTCTRL %x write cmd %x\n",
__FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_cmd);
-
+
DBG_LEAVE_ROUTINE
return rc;
}
@@ -583,7 +583,7 @@
struct controller *ctrl = slot->ctrl;
u16 slot_cmd;
u16 cmd_mask;
-
+
DBG_ENTER_ROUTINE
slot_cmd = 0x0100;
@@ -629,7 +629,7 @@
struct controller *ctrl = slot->ctrl;
u16 slot_cmd;
u16 cmd_mask;
-
+
DBG_ENTER_ROUTINE
slot_cmd = 0x0200;
@@ -649,7 +649,7 @@
static void hpc_release_ctlr(struct controller *ctrl)
{
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
if (pciehp_poll_mode)
del_timer(&ctrl->poll_timer);
@@ -674,7 +674,7 @@
u16 slot_status;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
dbg("%s: slot->hp_slot %x\n", __FUNCTION__, slot->hp_slot);
@@ -731,7 +731,7 @@
u16 cmd_mask;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
dbg("%s: slot->hp_slot %x\n", __FUNCTION__, slot->hp_slot);
@@ -825,7 +825,7 @@
}
dbg("%s: pciehp_readw(SLOTSTATUS) with value %x\n",
__FUNCTION__, slot_status);
-
+
/* Clear command complete interrupt caused by this write */
temp_word = 0x1f;
rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
@@ -835,10 +835,10 @@
return IRQ_NONE;
}
}
-
+
if (intr_loc & CMD_COMPLETED) {
- /*
- * Command Complete Interrupt Pending
+ /*
+ * Command Complete Interrupt Pending
*/
ctrl->cmd_busy = 0;
wake_up_interruptible(&ctrl->queue);
@@ -892,7 +892,7 @@
__FUNCTION__);
return IRQ_NONE;
}
-
+
/* Clear command complete interrupt caused by this write */
temp_word = 0x1F;
rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
@@ -904,7 +904,7 @@
dbg("%s: pciehp_writew(SLOTSTATUS) with value %x\n",
__FUNCTION__, temp_word);
}
-
+
return IRQ_HANDLED;
}
@@ -915,7 +915,7 @@
u32 lnk_cap;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readl(ctrl, LNKCAP, &lnk_cap);
if (retval) {
@@ -934,7 +934,7 @@
*value = lnk_speed;
dbg("Max link speed = %d\n", lnk_speed);
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -945,7 +945,7 @@
u32 lnk_cap;
int retval = 0;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readl(ctrl, LNKCAP, &lnk_cap);
if (retval) {
@@ -985,7 +985,7 @@
*value = lnk_wdth;
dbg("Max link width = %d\n", lnk_wdth);
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -996,7 +996,7 @@
int retval = 0;
u16 lnk_status;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, LNKSTATUS, &lnk_status);
if (retval) {
@@ -1015,7 +1015,7 @@
*value = lnk_speed;
dbg("Current link speed = %d\n", lnk_speed);
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -1026,14 +1026,14 @@
int retval = 0;
u16 lnk_status;
- DBG_ENTER_ROUTINE
+ DBG_ENTER_ROUTINE
retval = pciehp_readw(ctrl, LNKSTATUS, &lnk_status);
if (retval) {
err("%s: Cannot read LNKSTATUS register\n", __FUNCTION__);
return retval;
}
-
+
switch ((lnk_status & 0x03F0) >> 4){
case 0:
lnk_wdth = PCIE_LNK_WIDTH_RESRV;
@@ -1066,7 +1066,7 @@
*value = lnk_wdth;
dbg("Current link width = %d\n", lnk_wdth);
- DBG_LEAVE_ROUTINE
+ DBG_LEAVE_ROUTINE
return retval;
}
@@ -1085,12 +1085,12 @@
.get_cur_bus_speed = hpc_get_cur_lnk_speed,
.get_max_lnk_width = hpc_get_max_lnk_width,
.get_cur_lnk_width = hpc_get_cur_lnk_width,
-
+
.query_power_fault = hpc_query_power_fault,
.green_led_on = hpc_set_green_led_on,
.green_led_off = hpc_set_green_led_off,
.green_led_blink = hpc_set_green_led_blink,
-
+
.release_ctlr = hpc_release_ctlr,
.check_lnk_status = hpc_check_lnk_status,
};
@@ -1163,9 +1163,7 @@
}
#endif
-
-
-int pcie_init(struct controller * ctrl, struct pcie_device *dev)
+int pcie_init(struct controller *ctrl, struct pcie_device *dev)
{
int rc;
u16 temp_word;
@@ -1177,7 +1175,7 @@
struct pci_dev *pdev;
DBG_ENTER_ROUTINE
-
+
pdev = dev->port;
ctrl->pci_dev = pdev; /* save pci_dev in context */
@@ -1201,9 +1199,11 @@
dbg("%s: CAPREG offset %x cap_reg %x\n",
__FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
- if (((cap_reg & SLOT_IMPL) == 0) || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
+ if (((cap_reg & SLOT_IMPL) == 0)
+ || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
&& ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port or the port is not connected to a slot\n", __FUNCTION__);
+ dbg("%s : This is not a root port"
+ " or the port is not connected to a slot\n", __FUNCTION__);
goto abort_free_ctlr;
}
@@ -1242,7 +1242,8 @@
(unsigned long long)pci_resource_start(pdev, rc),
(unsigned long long)pci_resource_len(pdev, rc));
- info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n", pdev->vendor, pdev->device,
+ info("HPC vendor_id %x device_id %x ss_vid %x"
+ " ss_did %x\n", pdev->vendor, pdev->device,
pdev->subsystem_vendor, pdev->subsystem_device);
mutex_init(&ctrl->crit_sect);
@@ -1267,7 +1268,7 @@
dbg("%s: SLOTCTRL %x value read %x\n",
__FUNCTION__, ctrl->cap_base + SLOTCTRL, temp_word);
- temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE) | 0x00;
+ temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE)|0x00;
rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
if (rc) {
@@ -1330,14 +1331,14 @@
if (ATTN_BUTTN(slot_cap))
intr_enable = intr_enable | ATTN_BUTTN_ENABLE;
-
+
if (POWER_CTRL(slot_cap))
intr_enable = intr_enable | PWR_FAULT_DETECT_ENABLE;
-
+
if (MRL_SENS(slot_cap))
intr_enable = intr_enable | MRL_DETECT_ENABLE;
- temp_word = (temp_word & ~intr_enable) | intr_enable;
+ temp_word = (temp_word & ~intr_enable) | intr_enable;
if (pciehp_poll_mode) {
temp_word = (temp_word & ~HP_INTR_ENABLE) | 0x0;
@@ -1345,7 +1346,7 @@
temp_word = (temp_word & ~HP_INTR_ENABLE) | HP_INTR_ENABLE;
}
- /* Unmask Hot-plug Interrupt Enable for the interrupt notification mechanism case */
+ /* Unmask Hotplug Intr Enable for intr notification mechanism case */
rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
if (rc) {
err("%s: Cannot write to SLOTCTRL register\n", __FUNCTION__);
@@ -1356,14 +1357,14 @@
err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
goto abort_disable_intr;
}
-
+
temp_word = 0x1F; /* Clear all events */
rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
if (rc) {
err("%s: Cannot write to SLOTSTATUS register\n", __FUNCTION__);
goto abort_disable_intr;
}
-
+
if (pciehp_force) {
dbg("Bypassing BIOS check for pciehp use on %s\n",
pci_name(ctrl->pci_dev));
Fix pciehp_probe() to deal with pre-inserted ExpressCard cards,
but only when pciehp_force==1. Otherwise behaviour is unmodified.
Signed-off-by: Mark Lord <[email protected]>
---
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:14:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:16:36.000000000 -0400
@@ -37,7 +37,6 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_enable_slot(struct slot *p_slot);
static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
--- old/drivers/pci/hotplug/pciehp.h 2007-10-12 12:43:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 21:16:06.000000000 -0400
@@ -161,6 +161,7 @@
extern int pciehp_unconfigure_device(struct slot *p_slot);
extern void pciehp_queue_pushbutton_work(struct work_struct *work);
int pcie_init(struct controller *ctrl, struct pcie_device *dev);
+int pciehp_enable_slot(struct slot *p_slot);
static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
{
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:14:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:15:56.000000000 -0400
@@ -477,7 +477,8 @@
rc = t_slot->hpc_ops->power_off_slot(t_slot);
if (rc)
goto err_out_free_ctrl_slot;
- }
+ } else if (pciehp_force)
+ pciehp_enable_slot(t_slot);
return 0;
Split out the hotplug hardware initialization code from pcie_init()
into pcie_init_enable_events(), without changing any functionality.
Signed-off-by: Mark Lord <[email protected]>
---
--- old/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:14:44.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c 2007-10-16 21:21:11.000000000 -0400
@@ -1163,101 +1163,23 @@
}
#endif
-int pcie_init(struct controller *ctrl, struct pcie_device *dev)
+int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev)
{
int rc;
u16 temp_word;
- u16 cap_reg;
u16 intr_enable = 0;
u32 slot_cap;
- int cap_base;
- u16 slot_status, slot_ctrl;
+ u16 slot_status;
struct pci_dev *pdev;
DBG_ENTER_ROUTINE
pdev = dev->port;
- ctrl->pci_dev = pdev; /* save pci_dev in context */
-
- dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
- __FUNCTION__, pdev->vendor, pdev->device);
-
- if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
- dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
- ctrl->cap_base = cap_base;
-
- dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
-
- rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
- if (rc) {
- err("%s: Cannot read CAPREG register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: CAPREG offset %x cap_reg %x\n",
- __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
-
- if (((cap_reg & SLOT_IMPL) == 0)
- || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
- && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port"
- " or the port is not connected to a slot\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
if (rc) {
err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
goto abort_free_ctlr;
}
- dbg("%s: SLOTCAP offset %x slot_cap %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
-
- if (!(slot_cap & HP_CAP)) {
- dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- /* For debugging purpose */
- rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
- if (rc) {
- err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
-
- rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
- if (rc) {
- err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
-
- for ( rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
- if (pci_resource_len(pdev, rc) > 0)
- dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
- (unsigned long long)pci_resource_start(pdev, rc),
- (unsigned long long)pci_resource_len(pdev, rc));
-
- info("HPC vendor_id %x device_id %x ss_vid %x"
- " ss_did %x\n", pdev->vendor, pdev->device,
- pdev->subsystem_vendor, pdev->subsystem_device);
-
- mutex_init(&ctrl->crit_sect);
- mutex_init(&ctrl->ctrl_lock);
- spin_lock_init(&ctrl->lock);
-
- /* setup wait queue */
- init_waitqueue_head(&ctrl->queue);
-
- /* return PCI Controller Info */
- ctrl->slot_device_offset = 0;
- ctrl->num_slots = 1;
- ctrl->first_slot = slot_cap >> 19;
- ctrl->ctrlcap = slot_cap & 0x0000007f;
/* Mask Hot-plug Interrupt Enable */
rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
@@ -1346,7 +1268,7 @@
temp_word = (temp_word & ~HP_INTR_ENABLE) | HP_INTR_ENABLE;
}
- /* Unmask Hotplug Intr Enable for intr notification mechanism case */
+ /* Unmask hp Interrupt Enable for intr notification mechanism case */
rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
if (rc) {
err("%s: Cannot write to SLOTCTRL register\n", __FUNCTION__);
@@ -1374,8 +1296,6 @@
goto abort_disable_intr;
}
- ctrl->hpc_ops = &pciehp_hpc_ops;
-
DBG_LEAVE_ROUTINE
return 0;
@@ -1399,3 +1319,111 @@
DBG_LEAVE_ROUTINE
return -1;
}
+
+int pcie_init(struct controller *ctrl, struct pcie_device *dev)
+{
+ int rc;
+ u16 cap_reg;
+ u32 slot_cap;
+ int cap_base;
+ u16 slot_status, slot_ctrl;
+ struct pci_dev *pdev;
+
+ DBG_ENTER_ROUTINE
+
+ pdev = dev->port;
+ ctrl->pci_dev = pdev; /* save pci_dev in context */
+
+ dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
+ __FUNCTION__, pdev->vendor, pdev->device);
+
+ cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+ if (cap_base == 0) {
+ dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+
+ ctrl->cap_base = cap_base;
+
+ dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
+
+ rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
+ if (rc) {
+ err("%s: Cannot read CAPREG register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: CAPREG offset %x cap_reg %x\n",
+ __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
+
+ if (((cap_reg & SLOT_IMPL) == 0)
+ || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
+ && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
+ dbg("%s : This is not a root port"
+ " or the port is not connected to a slot\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+
+ rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
+ if (rc) {
+ err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: SLOTCAP offset %x slot_cap %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
+
+ if (!(slot_cap & HP_CAP)) {
+ dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ /* For debugging purpose */
+ rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
+ if (rc) {
+ err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
+
+ rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
+ if (rc) {
+ err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
+ goto abort_free_ctlr;
+ }
+ dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
+
+ for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
+ if (pci_resource_len(pdev, rc) > 0)
+ dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
+ (unsigned long long)pci_resource_start(pdev, rc),
+ (unsigned long long)pci_resource_len(pdev, rc));
+
+ info("HPC vendor_id %x device_id %x ss_vid %x"
+ " ss_did %x\n", pdev->vendor, pdev->device,
+ pdev->subsystem_vendor, pdev->subsystem_device);
+
+ mutex_init(&ctrl->crit_sect);
+ mutex_init(&ctrl->ctrl_lock);
+ spin_lock_init(&ctrl->lock);
+
+ /* setup wait queue */
+ init_waitqueue_head(&ctrl->queue);
+
+ /* return PCI Controller Info */
+ ctrl->slot_device_offset = 0;
+ ctrl->num_slots = 1;
+ ctrl->first_slot = slot_cap >> 19;
+ ctrl->ctrlcap = slot_cap & 0x0000007f;
+
+ rc = pcie_init_enable_events(ctrl, dev);
+ if (rc)
+ goto abort_free_ctlr;;
+
+ ctrl->hpc_ops = &pciehp_hpc_ops;
+
+ DBG_LEAVE_ROUTINE
+ return 0;
+abort_free_ctlr:
+ DBG_LEAVE_ROUTINE
+ return -1;
+}
Make use of the previously split out pcie_init_enable_events() function
to reinitialize the hotplug hardware on resume from suspend,
but only when pciehp_force==1. Otherwise behaviour is unmodified.
Signed-off-by: Mark Lord <[email protected]>
---
--- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:17:27.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:28:36.000000000 -0400
@@ -37,7 +37,6 @@
#include "pciehp.h"
static void interrupt_event_handler(struct work_struct *work);
-static int pciehp_disable_slot(struct slot *p_slot);
static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
{
--- old/drivers/pci/hotplug/pciehp.h 2007-10-16 21:17:27.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 21:28:42.000000000 -0400
@@ -162,6 +162,8 @@
extern void pciehp_queue_pushbutton_work(struct work_struct *work);
int pcie_init(struct controller *ctrl, struct pcie_device *dev);
int pciehp_enable_slot(struct slot *p_slot);
+int pciehp_disable_slot(struct slot *p_slot);
+int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev);
static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
{
--- old/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:17:27.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:30:19.000000000 -0400
@@ -512,6 +512,24 @@
static int pciehp_resume (struct pcie_device *dev)
{
printk("%s ENTRY\n", __FUNCTION__);
+ if (pciehp_force) {
+ struct pci_dev *pdev = dev->port;
+ struct controller *ctrl = pci_get_drvdata(pdev);
+ struct slot *t_slot;
+ u8 status;
+
+ /* reinitialize the chipset's event detection logic */
+ pcie_init_enable_events(ctrl, dev);
+
+ t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
+
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &status);
+ if (status)
+ pciehp_enable_slot(t_slot);
+ else
+ pciehp_disable_slot(t_slot);
+ }
return 0;
}
#endif
On Tue, Oct 16, 2007 at 09:54:08PM -0400, Mark Lord wrote:
> - t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */
> + /* Check if slot is occupied */
> + t_slot->hpc_ops->get_adapter_status(t_slot, &value);
> if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
> - rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
> + /* Power off slot if not occupied*/
> + rc = t_slot->hpc_ops->power_off_slot(t_slot);
I'd argue these comments fall under "stating the bleedin' obvious", but
that's Kristen's call.
> case INT_PRESENCE_OFF:
> if (!HP_SUPR_RM(ctrl->ctrlcap))
> break;
> - dbg("Surprise Removal\n");
> + dbg("Surprise Event\n");
> update_slot_info(p_slot);
> handle_surprise_event(p_slot);
> break;
That doesn't seem like an obviously correct change to me. Can you
explain?
> - if (((cap_reg & SLOT_IMPL) == 0) || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
> + if (((cap_reg & SLOT_IMPL) == 0)
> + || (((cap_reg & DEV_PORT_TYPE) != 0x0040)
> && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
> - dbg("%s : This is not a root port or the port is not connected to a slot\n", __FUNCTION__);
> + dbg("%s : This is not a root port"
> + " or the port is not connected to a slot\n", __FUNCTION__);
> goto abort_free_ctlr;
Normal style would be more like ...
if (((cap_reg & SLOT_IMPL) == 0) ||
(((cap_reg & DEV_PORT_TYPE) != 0x0040) &&
((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
dbg("%s : This is not a root port or the port is not "
"connected to a slot\n", __FUNCTION__);
> - info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n", pdev->vendor, pdev->device,
> + info("HPC vendor_id %x device_id %x ss_vid %x"
> + " ss_did %x\n", pdev->vendor, pdev->device,
> pdev->subsystem_vendor, pdev->subsystem_device);
Why did you choose to break the format string?
info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
pdev->vendor, pdev->device,
pdev->subsystem_vendor, pdev->subsystem_device);
> - temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE) | 0x00;
> + temp_word = (temp_word & ~HP_INTR_ENABLE & ~CMD_CMPL_INTR_ENABLE)|0x00;
Just delete the | 0x00?
> - temp_word = (temp_word & ~intr_enable) | intr_enable;
> + temp_word = (temp_word & ~intr_enable) | intr_enable;
*boggle*
temp_word |= intr_enable;
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, Oct 16, 2007 at 09:54:42PM -0400, Mark Lord wrote:
> if (rc)
> goto err_out_free_ctrl_slot;
> - }
> + } else if (pciehp_force)
> + pciehp_enable_slot(t_slot);
>
I find the construct if () { ... } else ...; to be a bit jarring. How
about adding the extra braces?
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Tue, Oct 16, 2007 at 09:54:08PM -0400, Mark Lord wrote:
>> - t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */
>> + /* Check if slot is occupied */
>> + t_slot->hpc_ops->get_adapter_status(t_slot, &value);
>> if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
>> - rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
>> + /* Power off slot if not occupied*/
>> + rc = t_slot->hpc_ops->power_off_slot(t_slot);
>
> I'd argue these comments fall under "stating the bleedin' obvious", but
> that's Kristen's call.
Hey, they're original to the file. I'm just keeping checkpatch.pl happy here.
Ditto for everything else you commented on.
>> case INT_PRESENCE_OFF:
>> if (!HP_SUPR_RM(ctrl->ctrlcap))
>> break;
>> - dbg("Surprise Removal\n");
>> + dbg("Surprise Event\n");
>> update_slot_info(p_slot);
>> handle_surprise_event(p_slot);
>> break;
>
> That doesn't seem like an obviously correct change to me. Can you
> explain?
Yeah. You clipped the top line: case INT_PRESENCE_ON:
So that code can be run for both hot plug and hot remove operations.
..
>> - temp_word = (temp_word & ~intr_enable) | intr_enable;
>> + temp_word = (temp_word & ~intr_enable) | intr_enable;
>
> *boggle*
Dig out your text editor, and notice the excess whitespace at the end of the line,
along with similar stuff on most other lines in this patch.
This is all just stuff that checkpatch.pl complains about when
the later patches are applied, so patch 01_... serves to reduce
the noise level from checkpatch.pl for the *real* patches which follow.
-ml
On Wed, Oct 17, 2007 at 09:09:24AM -0400, Mark Lord wrote:
> > I'd argue these comments fall under "stating the bleedin' obvious", but
> > that's Kristen's call.
>
> Hey, they're original to the file. I'm just keeping checkpatch.pl happy here.
> Ditto for everything else you commented on.
I can see that they were there before. I just think the appropriate
patch is to delete them rather than to move them
> >> case INT_PRESENCE_OFF:
> >> if (!HP_SUPR_RM(ctrl->ctrlcap))
> >> break;
> >> - dbg("Surprise Removal\n");
> >> + dbg("Surprise Event\n");
> >> update_slot_info(p_slot);
> >> handle_surprise_event(p_slot);
> >> break;
> >
> > That doesn't seem like an obviously correct change to me. Can you
> > explain?
>
> Yeah. You clipped the top line: case INT_PRESENCE_ON:
> So that code can be run for both hot plug and hot remove operations.
I didn't clip it -- diff did when it inserted three lines of context.
> ..
> >> - temp_word = (temp_word & ~intr_enable) | intr_enable;
> >> + temp_word = (temp_word & ~intr_enable) | intr_enable;
> >
> > *boggle*
>
> Dig out your text editor, and notice the excess whitespace at the end of the line,
> along with similar stuff on most other lines in this patch.
You, however, did clip the important line.
> > temp_word |= intr_enable;
which is what I was saying the above is equivalent to.
> This is all just stuff that checkpatch.pl complains about when
> the later patches are applied, so patch 01_... serves to reduce
> the noise level from checkpatch.pl for the *real* patches which follow.
I know, but if you're going to remove whitespace from a line, you might
as well change the line to look like idiomatic C rather than something
an Ada programmer wrote.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Matthew Wilcox wrote:
> On Wed, Oct 17, 2007 at 09:09:24AM -0400, Mark Lord wrote:
>
>> ..
>>>> - temp_word = (temp_word & ~intr_enable) | intr_enable;
>>>> + temp_word = (temp_word & ~intr_enable) | intr_enable;
>>> *boggle*
>> Dig out your text editor, and notice the excess whitespace at the end of the line,
>> along with similar stuff on most other lines in this patch.
>
> You, however, did clip the important line.
>
>>> temp_word |= intr_enable;
>
> which is what I was saying the above is equivalent to.
Gag-me. That *is* bad. But the PCIe folks seem to be extra sensitive
about the code, so I'm trying to change as little as possible here.
That line was just the result of doing this in vim: :%s/[ ]*$//g
I agree there's a lot of funny looking code in there,
but that's not what this particular patch series is about.
You could submit a follow-up patch to repair that stuff, if you like.
Mmm.. I wonder how clever gcc is with a line like that.. ? ;)
Cheers!
On Tue, 16 Oct 2007 21:53:24 -0400
Mark Lord <[email protected]> wrote:
> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> in conjunction with the modparam of pciehp_force=1.
>
> The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
> which lack ACPI BIOS support for PCIe hotplug:
You just sent four patches all of which are identified as "Fix PCIe hotplug
for non-ACPI ExpressCard slots (version 2)". Please do not do this.
I went through the (tiresome) exercise of inventing unique names for them
and then discovered that the first patch gets just a couple of rejects against
the mainline tree....
Hunk #1 succeeded at 129 (offset -31 lines).
Hunk #2 FAILED at 258.
Hunk #3 succeeded at 262 (offset -37 lines).
Hunk #4 FAILED at 295.
Hunk #5 FAILED at 304.
Hunk #6 FAILED at 313.
Hunk #7 FAILED at 331.
Hunk #8 FAILED at 363.
Hunk #9 FAILED at 373.
Hunk #10 FAILED at 391.
Hunk #11 FAILED at 409.
Hunk #12 FAILED at 417.
Hunk #13 FAILED at 430.
Hunk #14 FAILED at 440.
Hunk #15 FAILED at 451.
Hunk #16 FAILED at 459.
Hunk #17 FAILED at 535.
Hunk #18 FAILED at 546.
Hunk #19 FAILED at 592.
Hunk #20 FAILED at 612.
Hunk #21 FAILED at 637.
Hunk #22 FAILED at 694.
Hunk #23 succeeded at 734 (offset -91 lines).
Hunk #24 succeeded at 744 (offset -91 lines).
Hunk #25 succeeded at 801 (offset -91 lines).
Hunk #26 succeeded at 813 (offset -91 lines).
Hunk #27 FAILED at 824.
Hunk #28 FAILED at 843.
Hunk #29 FAILED at 854.
Hunk #30 FAILED at 894.
Hunk #31 FAILED at 905.
Hunk #32 FAILED at 924.
Hunk #33 FAILED at 935.
Hunk #34 FAILED at 975.
Hunk #35 succeeded at 988 (offset -97 lines).
Hunk #36 FAILED at 1066.
Hunk #37 FAILED at 1078.
Hunk #38 FAILED at 1102.
Hunk #39 FAILED at 1145.
Hunk #40 FAILED at 1171.
Hunk #41 succeeded at 1235 (offset -96 lines).
Hunk #42 FAILED at 1250.
Hunk #43 succeeded at 1264 (offset -93 lines).
34 out of 43 hunks FAILED -- saving rejects to file drivers/pci/hotplug/pciehp_hpc.c.rej
Andrew Morton wrote:
> On Tue, 16 Oct 2007 21:53:24 -0400
> Mark Lord <[email protected]> wrote:
>
>> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
>> in conjunction with the modparam of pciehp_force=1.
>>
>> The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
>> which lack ACPI BIOS support for PCIe hotplug:
>
> You just sent four patches all of which are identified as "Fix PCIe hotplug
> for non-ACPI ExpressCard slots (version 2)". Please do not do this.
Eh? They were labelled as 1/4, 2/4, 3/4, and 4/4.
But I'll make the rest of the subject line unique as well on resubmit, then.
Thanks.
So far, two postings, and zero comments from anyone on the actual code.
Cheers
On Wed, 17 Oct 2007 18:59:43 -0400
Mark Lord <[email protected]> wrote:
> Andrew Morton wrote:
> > On Tue, 16 Oct 2007 21:53:24 -0400
> > Mark Lord <[email protected]> wrote:
> >
> >> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> >> in conjunction with the modparam of pciehp_force=1.
> >>
> >> The PCIe Hotplug driver has two shortcomings when used on Dell notebooks
> >> which lack ACPI BIOS support for PCIe hotplug:
> >
> > You just sent four patches all of which are identified as "Fix PCIe hotplug
> > for non-ACPI ExpressCard slots (version 2)". Please do not do this.
>
> Eh? They were labelled as 1/4, 2/4, 3/4, and 4/4.
Please, review http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt -
a lot of experiecne has gone into that...
When preparing patches, always think "how will this appear to someone who
is reading it in the git tree a year from now".
Obviously stuff like "[patch 2/4]" won't be there, because it is
meaningless once the patch hits the git tree. Also text such as "the previous
patch" and "my patch from yesterday" and "John Doe's comments" and lots of
other stuff which is appropriate to an email conversation is _not_
appropriate to a permanent git commit.
> But I'll make the rest of the subject line unique as well on resubmit, then.
>
Thanks. That's much better than having me invent the subject, because when
you choose the title, everyone agrees on what the patch is _called_ (as
long as the title isn't so poorly chosen that I have to fix it).
If the patch title is well-chosen then it becomes a nice google search key,
so if someone wants to find out what we were thinking when we did a
particular patch two years ago, they can just google for the title and go
and read all the email discussion.
>
> So far, two postings, and zero comments from anyone on the actual code.
Actual code? That sounds hard ;)
On Tue, 16 Oct 2007 21:54:42 -0400
Mark Lord <[email protected]> wrote:
> Fix pciehp_probe() to deal with pre-inserted ExpressCard cards,
> but only when pciehp_force==1. Otherwise behaviour is unmodified.
I think it would be ok to try allowing the slot to be enabled when not
using pciehp_force mode. We can wrap it later if it proves to break
things, however, see my comment below:
>
> Signed-off-by: Mark Lord <[email protected]>
> ---
> --- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:14:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:16:36.000000000 -0400
> @@ -37,7 +37,6 @@
> #include "pciehp.h"
>
> static void interrupt_event_handler(struct work_struct *work);
> -static int pciehp_enable_slot(struct slot *p_slot);
> static int pciehp_disable_slot(struct slot *p_slot);
>
> static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
> --- old/drivers/pci/hotplug/pciehp.h 2007-10-12 12:43:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 21:16:06.000000000 -0400
> @@ -161,6 +161,7 @@
> extern int pciehp_unconfigure_device(struct slot *p_slot);
> extern void pciehp_queue_pushbutton_work(struct work_struct *work);
> int pcie_init(struct controller *ctrl, struct pcie_device *dev);
> +int pciehp_enable_slot(struct slot *p_slot);
>
> static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
> {
> --- old/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:14:44.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:15:56.000000000 -0400
> @@ -477,7 +477,8 @@
> rc = t_slot->hpc_ops->power_off_slot(t_slot);
> if (rc)
> goto err_out_free_ctrl_slot;
> - }
> + } else if (pciehp_force)
> + pciehp_enable_slot(t_slot);
Here it seems like what you want to do just go ahead and try to call
pciehp_enable_slot always, but check the return value. If an adapter is
not present, it will return -ENODEV, and then you can check to see if
you have the ability to power off the slot, and try to power it off.
Please fix CodingStyle issues too.
>
> return 0;
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems? Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> Pcihpd-discuss mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/pcihpd-discuss
>
On Tue, 16 Oct 2007 21:55:30 -0400
Mark Lord <[email protected]> wrote:
> Make use of the previously split out pcie_init_enable_events() function
> to reinitialize the hotplug hardware on resume from suspend,
> but only when pciehp_force==1. Otherwise behaviour is unmodified.
OK - definitely in this case the right thing to do is not use this code
unless you are forcing pciehp, thanks.
I think I'd be careful when you rename this patch - non-ACPI
ExpressCard slots is not what you want to say, as this fix is very
specific for your machine. We have no idea if this will fix any other
problems that occur when people force pciehp - and in many cases this
either may not be needed, or may not be enough. I only care about the
name because I don't want people mislead when they are reading git commits.
>
> Signed-off-by: Mark Lord <[email protected]>
> ---
> --- old/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:17:27.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_ctrl.c 2007-10-16 21:28:36.000000000 -0400
> @@ -37,7 +37,6 @@
> #include "pciehp.h"
>
> static void interrupt_event_handler(struct work_struct *work);
> -static int pciehp_disable_slot(struct slot *p_slot);
>
> static int queue_interrupt_event(struct slot *p_slot, u32 event_type)
> {
> --- old/drivers/pci/hotplug/pciehp.h 2007-10-16 21:17:27.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp.h 2007-10-16 21:28:42.000000000 -0400
> @@ -162,6 +162,8 @@
> extern void pciehp_queue_pushbutton_work(struct work_struct *work);
> int pcie_init(struct controller *ctrl, struct pcie_device *dev);
> int pciehp_enable_slot(struct slot *p_slot);
> +int pciehp_disable_slot(struct slot *p_slot);
> +int pcie_init_enable_events(struct controller *ctrl, struct pcie_device *dev);
>
> static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
> {
> --- old/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:17:27.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_core.c 2007-10-16 21:30:19.000000000 -0400
> @@ -512,6 +512,24 @@
> static int pciehp_resume (struct pcie_device *dev)
> {
> printk("%s ENTRY\n", __FUNCTION__);
> + if (pciehp_force) {
> + struct pci_dev *pdev = dev->port;
> + struct controller *ctrl = pci_get_drvdata(pdev);
> + struct slot *t_slot;
> + u8 status;
> +
> + /* reinitialize the chipset's event detection logic */
> + pcie_init_enable_events(ctrl, dev);
> +
> + t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
> +
> + /* Check if slot is occupied */
> + t_slot->hpc_ops->get_adapter_status(t_slot, &status);
> + if (status)
> + pciehp_enable_slot(t_slot);
> + else
> + pciehp_disable_slot(t_slot);
> + }
> return 0;
> }
> #endif
>
Kristen Carlson Accardi wrote:
> On Tue, 16 Oct 2007 21:54:42 -0400
> Mark Lord <[email protected]> wrote:
>
>> Fix pciehp_probe() to deal with pre-inserted ExpressCard cards,
>> but only when pciehp_force==1. Otherwise behaviour is unmodified.
>
> I think it would be ok to try allowing the slot to be enabled when not
> using pciehp_force mode. We can wrap it later if it proves to break
> things, however, see my comment below:
Could you test that on ACPI-supported hardware and report back, please.
...
> Here it seems like what you want to do just go ahead and try to call
> pciehp_enable_slot always, but check the return value. If an adapter is
> not present, it will return -ENODEV, and then you can check to see if
> you have the ability to power off the slot, and try to power it off.
Okay, will do.
> Please fix CodingStyle issues too.
????
Kristen Carlson Accardi wrote:
> On Tue, 16 Oct 2007 21:55:30 -0400
> Mark Lord <[email protected]> wrote:
>
>> Make use of the previously split out pcie_init_enable_events() function
>> to reinitialize the hotplug hardware on resume from suspend,
>> but only when pciehp_force==1. Otherwise behaviour is unmodified.
>
> OK - definitely in this case the right thing to do is not use this code
> unless you are forcing pciehp, thanks.
Yeah, that was Ted's suggestion, and it makes sense.
> I think I'd be careful when you rename this patch - non-ACPI
> ExpressCard slots is not what you want to say, as this fix is very
> specific for your machine.
No, from reading through the driver it seems that any machine
that lacks ACPI BIOS support for PCIe slots will suffer from
the exact same problems.
That's a large class of machines, not just my tiny little individual
one here (of which *millions* have been manufactured and sold).
But, whatever.