* Re: Fn Keys wrong function
From: Martin Pitt @ 2013-10-10 4:05 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <CAJ2vxsXA7=VUQ_=KJgJivCW_96u7PDAzjK04g34Kp1azRejtnw@mail.gmail.com>
Hello Guillermo,
Guillermo Dominguez Duarte [2013-10-09 16:32 -0700]:
> Is this the place to report that in my computer, Fn keys that are
> suposed to bright up the screen and change the display output are now
> making my computer sleep and hibernate respectively?
It's a good enough place :) Please tell us which udev version you are
running, install the "evtest" tool, run it, press these two keys, and
copy & paste the whole output.
After that, please run
udevadm info --export-db > /tmp/udev.txt
and attach /tmp/udev.txt, too.
Thanks,
Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
^ permalink raw reply
* pre-configuring PCIe switch
From: Rajat Jain @ 2013-10-10 19:19 UTC (permalink / raw)
To: linux-pci@vger.kernel.org, linux-hotplug@vger.kernel.org
Hello,
For the purposes of hot-plug, I want to configure my PCI Express's switch's downstream port capabilities before enumeration. (I am currently interested in setting "Slot Implemented", "hot-plug capable" and "physical slot number". But in future, this may also come in handy to configure some downstream ports as non-transparent). These registers seem to be only writable via an external EEPROM (which is not populated), or an I2C bus. Thus PCI quirks cannot be used. What is the best way to achieve this?
I'm thinking of configuring over I2C before the PCI enumeration begins. My question is how do I ensure that the I2C subsystem is ready to be used, before the PCI subsystem is scanned by the kernel? (This is a Powerpc system, no ACPI or BIOS) Is it OK to use I2C bus at such an early stage in boot up, for purposes like this?
Thanks,
Rajat
^ permalink raw reply
* Re: Fn Keys wrong function
From: Guillermo D @ 2013-10-18 2:16 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <CAJ2vxsXA7=VUQ_=KJgJivCW_96u7PDAzjK04g34Kp1azRejtnw@mail.gmail.com>
Hi
I found the way to configure the Fn keys scan codes to the correct key
code. Except for the Fn key that turns wireless on/off.
When I try to apply the mapping, I get this error:
EVIOCGKEYCODE for scan code 0x158: Invalid argument
I did exactly the same with the other scan codes, but only this scan
code gives me this error.
Help please
On Wed, Oct 9, 2013 at 4:32 PM, Guillermo Dominguez Duarte
<guillermod84@gmail.com> wrote:
> Hi guys.
> Is this the place to report that in my computer, Fn keys that are
> suposed to bright up the screen and change the display output are now
> making my computer sleep and hibernate respectively?
>
> This dind't happen until one of the kernel updates. I am not sure which one
>
> Thanks
^ permalink raw reply
* RE: Enhancing pciehp (was: pcielw An alternate pcie hotplug driver)
From: Rajat Jain @ 2013-10-19 1:07 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: ebiederm@xmission.com, linux-pci@vger.kernel.org,
linux-hotplug@vger.kernel.org, Greg KH, Guenter Roeck,
tom.l.nguyen@intel.com, kristen.c.accardi@intel.com,
rajesh.shah@intel.com, rajatjain.linux@gmail.com
In-Reply-To: <CAErSpo4Yg-kUz3LHaWPVmzE1rGhdiqeJWuHwrM=-hxhP_zYH_g@mail.gmail.com>
Hello,
I'm working on enhancing the pciehp driver to allow link state changes to drive the hot-plug and un-plug. The idea was discussed here:
http://www.spinics.net/lists/linux-pci/msg25733.html
However, this is to invite ideas on how to handle a particular corner case I see. Let's say a PCIe link comes up and the pciehp driver calls pciehp_enable_slot() to start scanning and adding the devices on the slot. However, what to do if before that completes, the link goes down due to some reason? Can the pciehp driver call pciehp_disable_slot() before the call to pciehp_enable_slot() returns (with or without error)?
Essentially even without link state to driver the hot-plug, this problem can also be thought of as a problem that exists today. For e.g. for the slots that support surprise removal, the pciehp will initiate hot-plug and unplug based on the presence detection (as soon as card is inserted or removed). Now if a user puts in a card very momentarily, long enough for the hot-add process to get started (pciehp_enable_slot()) but removes it before it can be completed, the same problem can be envisioned. As the code exists today, I believe it would call pciehp_disable_slot() even before pciehp_enable_slot() returns. Is that the designed / correct behavior?
Should I be following the same for link state change driven hot-plug?
Thanks,
Rajat
^ permalink raw reply
* Re: Enhancing pciehp
From: Eric W. Biederman @ 2013-10-19 1:22 UTC (permalink / raw)
To: Rajat Jain
Cc: Bjorn Helgaas, linux-pci@vger.kernel.org,
linux-hotplug@vger.kernel.org, Greg KH, Guenter Roeck,
tom.l.nguyen@intel.com, kristen.c.accardi@intel.com,
rajesh.shah@intel.com, rajatjain.linux@gmail.com
In-Reply-To: <c713940b329e4c6584e173cb49aaa043@BLUPR05MB118.namprd05.prod.outlook.com>
Rajat Jain <rajatjain@juniper.net> writes:
> Hello,
>
> I'm working on enhancing the pciehp driver to allow link state changes to drive the hot-plug and un-plug. The idea was discussed here:
>
> http://www.spinics.net/lists/linux-pci/msg25733.html
>
> However, this is to invite ideas on how to handle a particular corner case I see. Let's say a PCIe link comes up and the pciehp driver calls pciehp_enable_slot() to start scanning and adding the devices on the slot. However, what to do if before that completes, the link goes down due to some reason? Can the pciehp driver call pciehp_disable_slot() before the call to pciehp_enable_slot() returns (with or without error)?
>
> Essentially even without link state to driver the hot-plug, this problem can also be thought of as a problem that exists today. For e.g. for the slots that support surprise removal, the pciehp will initiate hot-plug and unplug based on the presence detection (as soon as card is inserted or removed). Now if a user puts in a card very momentarily, long enough for the hot-add process to get started (pciehp_enable_slot()) but removes it before it can be completed, the same problem can be envisioned. As the code exists today, I believe it would call pciehp_disable_slot() even before pciehp_enable_slot() returns. Is that the designed / correct behavior?
>
> Should I be following the same for link state change driven hot-plug?
The way I handled this problem is made the enable and disable all run
through the same single threaded work queue.
The code does need to be single threaded on a per hotplug slot basis
(aka enable and disable concurrently on the same slot is nonsense).
I think there is some locking in the device tree that you might be able
to depend on to help single thread this. At the time I was working on
this I did not see any locking or anything else that would serialize the
access so I went for the very cheap solution of an interrupt handler
that queued work in a single threaded work queue.
The code to be correct and robuse must handle the case of where what it
thought was going on add/remove is time late information and is actually
incorrect at the time of the hotplug scan.
This all gets even more interesting when you are plugging in a tree of
hotplug devices with hotplug slots of their own.
From an implementation point of view I have observed devices in the wild
with imperfect PCIe busses that due to bus noise would occassionally
flap their presence bit for no particularly good reason. It is a
hardware but so you don't need to handle it beyond not letting that kind
of activity crash the kernel but it does happen.
Eric
^ permalink raw reply
* Re: How important is the MBR partition offset of grub-mkrescue ?
From: Andrey Borzenkov @ 2013-11-04 12:08 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: linux-hotplug, scdbackup
In-Reply-To: <1974651407333528403@scdbackup.webframe.org>
В Sun, 03 Nov 2013 18:16:09 +0100
"Thomas Schmitt" <scdbackup@gmx.net> пишет:
> Hi,
>
> i believe to have found the udev rules in Debian 6 which
> govern the population of /dev/disk/by-label.
>
> File
> /lib/udev/rules.d/60-persistent-storage.rules
> has
> # probe filesystem metadata of disks
> KERNEL!="sr*", IMPORT{program}="/sbin/blkid -o udev -p $tempnode"
> ...
> ENV{ID_FS_LABEL_ENC}="?*", ENV{ID_FS_USAGE}="filesystem|other", \
> SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
>
> I understand that if blkid sets variable ID_FS_LABEL_ENC to
> a non-empty value, then this will become the link name in ./by-label.
> The link target is the device that is being examined by the rule.
>
> From a USB stick with partition start LBA 1, i get:
>
> $ /sbin/blkid -o udev -p /dev/sdb
> ID_PART_TABLE_TYPE=dos
> ID_FS_LABEL=epidemic-4.1-b1-1-ts-amd64
> ID_FS_LABEL_ENC=epidemic-4.1-b1-1-ts-amd64
> ID_FS_TYPE=iso9660
> ID_FS_USAGE=filesystem
> $ /sbin/blkid -o udev -p /dev/sdb1
> $
>
> So why the hell does /dev/sdb1 become link target ?
> Its ID_FS_LABEL_ENC must be empty.
>
> Any idea how to get a verbous log of these decisions ?
>
I confirm this. The culprit is this rule in 60-persistent-storage.rules:
# for partitions import parent information
ENV{DEVTYPE}="partition", IMPORT{parent}="ID_*"
I'm not really sure how exactly to fix it. I.e. normally it is assumed
that device is either partitioned or not. Situation when we have
filesystem on a whole disk *and* individual partitions ... not sure.
I'm interested in which information actually needs to be imported from
parent. May be it should be less aggressive.
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: How important is the MBR partition offset of grub-mkrescue ?
From: Thomas Schmitt @ 2013-11-04 15:44 UTC (permalink / raw)
To: linux-hotplug
In-Reply-To: <20131104160843.5c0cabd2@opensuse.site>
Hi,
Andrey Borzenkov wrote:
> I confirm this. The culprit is this rule in 60-persistent-storage.rules:
>
> # for partitions import parent information
> ENV{DEVTYPE}="partition", IMPORT{parent}="ID_*"
>
> I'm not really sure how exactly to fix it. I.e. normally it is assumed
> that device is either partitioned or not. Situation when we have
> filesystem on a whole disk *and* individual partitions ... not sure.
>
> I'm interested in which information actually needs to be imported from
> parent. May be it should be less aggressive.
Background:
I started this discussion on grub-devel because the problem
shows up with bootable ISO 9660 images produced by grub-mkrescue.
These images are suitable for booting from CD/DVD and from
USB stick. They begin by an MBR which marks a single partition
beginning at LBA 1. The ISO filesystem has to begin at LBA 0,
because else it would not be bootable from CD/DVD.
So the partition is not mountable, intentionally. It just protects
the ISO filesystem from partition editors and alike, but leaves
the MBR unclaimed in order to please some boot firmware.
Nevertheless the partition shows up as link in /dev/disk/by-label
under the name of the ISO filesystem Volume Id.
After Andrey Borzenkov pointed me to IMPORT{parent}, i added
statements to reset the blkid ID_FS_* properties:
ENV{DEVTYPE}="partition", IMPORT{parent}="ID_*", \
ENV{ID_FS_LABEL}="" , ENV{ID_FS_LABEL_ENC}="" , \
ENV{ID_FS_TYPE}="" , ENV{ID_FS_USAGE}=""
I hope this is not a totally stupid beginner's mistake.
At least it solved the problem on my system.
Now the link points to /dev/sdb rather than to /dev/sdb1.
So i assume that before the change, the partition link overwrote
the disk link.
Have a nice day :)
Thomas
^ permalink raw reply
* [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Rajat Jain @ 2013-11-05 22:33 UTC (permalink / raw)
To: linux-pci, linux-hotplug, Bjorn Helgaas, Kenji Kaneshige,
Yijing Wang
Cc: Guenter Roeck, Rajat Jain, Greg KH, Tom Nguyen, Kristen Accardi,
Rajat Jain
In case of a spurious "cmd completed", pcie_write_cmd() does not
clear it, but yet expects more "cmd completed" events to be generated.
This does not happen because the previous (spurious) event has not
been acknowledged. Fix that.
Signed-off-by: Rajat Jain <rajatjain@juniper.net>
Signed-off-by: Guenter Roeck <groeck@juniper.net>
---
This is how I saw it in action: my controller does not implement any
hot-plug elements (LED, power ctrl, EMI etc) but still supports Command
completed bit.
- During initialization,
pcie_disable_notification()
-> pcie_write_cmd()
-> writes to Slot control register
-> which causes PCI_EXP_SLTSTA_CC to get set, which is not
cleared, because IRQ is not generated (we just disabled
notifications).
- After some time,
pcie_enable_notification()
-> pcie_write_cmd()
-> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious.
-> Does not clear it, yet expects more command completed
events to be generated (never happens).
drivers/pci/hotplug/pciehp_hpc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5b8d749..ba8e06f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
}
if (slot_status & PCI_EXP_SLTSTA_CC) {
+ pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
if (!ctrl->no_cmd_complete) {
/*
* After 1 sec and CMD_COMPLETED still not set, just
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Bjorn Helgaas @ 2013-11-06 0:25 UTC (permalink / raw)
To: Rajat Jain
Cc: linux-pci, linux-hotplug, Kenji Kaneshige, Yijing Wang,
Guenter Roeck, Rajat Jain, Greg KH, Tom Nguyen, Kristen Accardi,
Rajat Jain
In-Reply-To: <52797238.8070304@gmail.com>
On Tue, Nov 05, 2013 at 02:33:28PM -0800, Rajat Jain wrote:
> In case of a spurious "cmd completed", pcie_write_cmd() does not
> clear it, but yet expects more "cmd completed" events to be generated.
> This does not happen because the previous (spurious) event has not
> been acknowledged. Fix that.
>
> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> ---
> This is how I saw it in action: my controller does not implement any
> hot-plug elements (LED, power ctrl, EMI etc) but still supports Command
> completed bit.
> - During initialization,
> pcie_disable_notification()
> -> pcie_write_cmd()
> -> writes to Slot control register
> -> which causes PCI_EXP_SLTSTA_CC to get set, which is not
> cleared, because IRQ is not generated (we just disabled
> notifications).
> - After some time,
> pcie_enable_notification()
> -> pcie_write_cmd()
> -> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious.
> -> Does not clear it, yet expects more command completed
> events to be generated (never happens).
I'm not sure this "cmd completed" is actually spurious. Spec section
7.8.10 is very clear that any write to Slot Control must cause a
hot-plug command to be generated (if the port is hot-plug capable).
Can you collect "lspci -vv" output for your controller? I assume
you're hitting this case in pcie_init() (added by 5808639bfa98
("pciehp: fix slow probing")):
/*
* Controller doesn't notify of command completion if the "No
* Command Completed Support" bit is set in Slot Capability
* register or the controller supports none of power
* controller, attention led, power led and EMI.
*/
if (NO_CMD_CMPL(ctrl) ||
!(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
ctrl->no_cmd_complete = 1;
and we're setting "no_cmd_complete = 1" for your controller, which
keeps us from waiting for completion in pcie_write_cmd().
I'm dubious about the assertion that a controller without power
control, attention LED, power LED, or EMI can't support command
completion. I don't see anything in the spec to that effect.
Since you're seeing PCI_EXP_SLTSTA_CC=1, your controller *should*
support Command Completion notification and PCI_EXP_SLTCAP_NCCS should
be 0 (per Table 7-20), so I wonder what happens on your system if you
change pcie_init() so it leaves "ctrl->no_cmd_complete = 0" instead?
Does it work correctly then?
I know we can't just drop the "!(POWER_CTRL(ctrl) | ...)" tests
because we don't want to reintroduce the problem fixed by
5808639bfa98, but I wonder if we can find a better fix that addresses
both problems.
Bjorn
>
> drivers/pci/hotplug/pciehp_hpc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 5b8d749..ba8e06f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
> }
>
> if (slot_status & PCI_EXP_SLTSTA_CC) {
> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
> if (!ctrl->no_cmd_complete) {
> /*
> * After 1 sec and CMD_COMPLETED still not set, just
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Rajat Jain @ 2013-11-06 2:38 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-hotplug, Kenji Kaneshige, Yijing Wang,
Guenter Roeck, Rajat Jain, Greg KH, Tom Nguyen, Kristen Accardi,
Rajat Jain
In-Reply-To: <20131106002505.GC3359@google.com>
Hello Bjorn,
On Tue, Nov 5, 2013 at 4:25 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Nov 05, 2013 at 02:33:28PM -0800, Rajat Jain wrote:
>> In case of a spurious "cmd completed", pcie_write_cmd() does not
>> clear it, but yet expects more "cmd completed" events to be generated.
>> This does not happen because the previous (spurious) event has not
>> been acknowledged. Fix that.
>>
>> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
>> Signed-off-by: Guenter Roeck <groeck@juniper.net>
>> ---
>> This is how I saw it in action: my controller does not implement any
>> hot-plug elements (LED, power ctrl, EMI etc) but still supports Command
>> completed bit.
>> - During initialization,
>> pcie_disable_notification()
>> -> pcie_write_cmd()
>> -> writes to Slot control register
>> -> which causes PCI_EXP_SLTSTA_CC to get set, which is not
>> cleared, because IRQ is not generated (we just disabled
>> notifications).
>> - After some time,
>> pcie_enable_notification()
>> -> pcie_write_cmd()
>> -> finds PCI_EXP_SLTSTA_CC is set, assumes it is spurious.
>> -> Does not clear it, yet expects more command completed
>> events to be generated (never happens).
>
> I'm not sure this "cmd completed" is actually spurious. Spec section
> 7.8.10 is very clear that any write to Slot Control must cause a
> hot-plug command to be generated (if the port is hot-plug capable).
I agree, and I think I am witnessing a "genuine" command completion
(caused by disabling of notifications).
>
> Can you collect "lspci -vv" output for your controller?
>
Sure:
PCI bridge: Integrated Device Technology, Inc. Device 807a (rev 02)
(prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSELúst >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Bus: primary\x02, secondaryP, subordinate_, sec-latency=0
I/O behind bridge: 00003000-00003fff
Memory behind bridge: 8c000000-8fffffff
Prefetchable memory behind bridge: 00000000b0600000-00000000b07fffff
Secondary status: 66MHz- FastB2B- ParErr- DEVSELúst >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [40] Express (v2) Downstream Port (Slot+), MSI 00
DevCap: MaxPayload 2048 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
ExtTag+ RBE+ FLReset-
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 128 bytes
DevSta: CorrErr+ UncorrErr- FatalErr- UnsuppReq+ AuxPwr- TransPend-
LnkCap: Port #3, Speed 5GT/s, Width x4, ASPM L0s L1, Latency
L0 <4us, L1 <4us
ClockPM- Surprise+ LLActRep+ BwNot+
LnkCtl: ASPM Disabled; Disabled- Retrain- CommClk-
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x0, TrErr- Train- SlotClk-
DLActive- BWMgmt- ABWMgmt-
SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise-
Slot #3, PowerLimit 0.000W; Interlock- NoCompl-
SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt+ HPIrq+ LinkChg+
Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt+ PresDet- Interlock-
Changed: MRL- PresDet- LinkState-
DevCap2: Completion Timeout: Not Supported, TimeoutDis-, LTR-,
OBFF Not Supported ARIFwd+
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-,
OBFF Disabled ARIFwd-
LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-,
Selectable De-emphasis: -6dB
Transmit Margin: Normal Operating Range,
EnterModifiedCompliance- ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB,
EqualizationComplete-, EqualizationPhase1-
EqualizationPhase2-, EqualizationPhase3-,
LinkEqualizationRequest-
Capabilities: [c0] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
Address: 00000000ff041740 Data: 0003
Capabilities: [100 v1] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt-
RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt-
RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt-
RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
AERCap: First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn-
Capabilities: [200 v1] Virtual Channel
Caps: LPEVC=0 RefClk\x100ns PATEntryBits=4
Arb: Fixed- WRR32- WRR64- WRR128-
Ctrl: ArbSelect=Fixed
Status: InProgress-
VC0: Caps: PATOffset\x04 MaxTimeSlots=1 RejSnoopTrans-
Arb: Fixed+ WRR32- WRR64- WRR128- TWRR128- WRR256-
Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VCÿ
Status: NegoPending- InProgress-
Port Arbitration Table <?>
Capabilities: [320 v1] Access Control Services
ACSCap: SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+ UpstreamFwd+
EgressCtrl+ DirectTrans+
ACSCtl: SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd-
EgressCtrl- DirectTrans-
Capabilities: [330 v1] #12
Kernel driver in use: pcieport
In addition, here is what the pciehp driver spews out:
Hotplug Controller:
Seg/Bus/Dev/Func/IRQ : 0000:02:03.0 IRQ 21
Vendor ID : 0x111d
Device ID : 0x807a
Subsystem ID : 0x0000
Subsystem Vendor ID : 0x0000
PCIe Cap offset : 0x40
PCI resource [7] : [io 0x13000-0x13fff]
PCI resource [8] : [mem 0xc0c000000-0xc0fffffff]
PCI resource [9] : [mem 0xc30600000-0xc307fffff 64bit pref]
Slot Capabilities : 0x00180040
Physical Slot Number : 3
Attention Button : no
Power Controller : no
MRL Sensor : no
Attention Indicator : no
Power Indicator : no
Hot-Plug Surprise : no
EMI Present : no
Command Completed : yes
Slot Status : 0x0010
Slot Control : 0x0000
Link Active Reporting supported
HPC vendor_id 111d device_id 807a ss_vid 0 ss_did 0
Registering domain:bus:dev\000:50:00 sun=3
Unexpected CMD_COMPLETED. Need to wait for command completed event.
Command not completed in 1000 msec
The last two output lines are part of the problem. Each controller
takes 1 second to initialized as the message shows.
> I assume
> you're hitting this case in pcie_init() (added by 5808639bfa98
> ("pciehp: fix slow probing")):
>
> /*
> * Controller doesn't notify of command completion if the "No
> * Command Completed Support" bit is set in Slot Capability
> * register or the controller supports none of power
> * controller, attention led, power led and EMI.
> */
> if (NO_CMD_CMPL(ctrl) ||
> !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
> ctrl->no_cmd_complete = 1;
>
> and we're setting "no_cmd_complete = 1" for your controller, which
> keeps us from waiting for completion in pcie_write_cmd().
>
> I'm dubious about the assertion that a controller without power
> control, attention LED, power LED, or EMI can't support command
> completion. I don't see anything in the spec to that effect.
I agree, specially since I am seeing this contrary behavior (to that
assertion) in action :-)
>
> Since you're seeing PCI_EXP_SLTSTA_CC=1, your controller *should*
> support Command Completion notification and PCI_EXP_SLTCAP_NCCS should
> be 0 (per Table 7-20), so I wonder what happens on your system if you
> change pcie_init() so it leaves "ctrl->no_cmd_complete = 0" instead?
> Does it work correctly then?
Yes, it work well after that (Both the error output lines go away as
well). And the 1 second delay per controller is also gone.
>
> I know we can't just drop the "!(POWER_CTRL(ctrl) | ...)" tests
> because we don't want to reintroduce the problem fixed by
> 5808639bfa98, but I wonder if we can find a better fix that addresses
> both problems.
Please let me know if any one has any suggestions? IMHO, this patch
should yet not cause the original problem to reappear because we are
clearing the bit *only* when we know that it is already set (and no
way to clear it because at least in my controller no subsequent
interrupts can be generated). But I do not have enough understanding,
and will be glad to get any pointers.
Thanks,
- Rajat
>
> Bjorn
>
>>
>> drivers/pci/hotplug/pciehp_hpc.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 5b8d749..ba8e06f 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
>> }
>>
>> if (slot_status & PCI_EXP_SLTSTA_CC) {
>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
>> if (!ctrl->no_cmd_complete) {
>> /*
>> * After 1 sec and CMD_COMPLETED still not set, just
>> --
>> 1.7.9.5
>>
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Rajat Jain @ 2013-11-07 21:53 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-hotplug, Kenji Kaneshige, Yijing Wang,
Guenter Roeck, Greg KH, Tom Nguyen, Kristen Accardi, Rajat Jain,
Rajat Jain
In-Reply-To: <CADTPrLRmCjcKDeapFNk5vT3SfwSqSgVvd=u2p7BjgihXzsyfmg@mail.gmail.com>
Hi Bjorn / list,
I think there are two independent problems that need to be addressed.
Problem #1: In any condition where (e.g. spurious interrupt) once the
execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC
bit is already set, and needs to be cleared. This does not happen
today and hence this patch was aimed at solving that. Please note that
this will not cause any problems to the systems that were fixed by
5808639bfa98, because this is equally applicable to any case.
>>>
>>> drivers/pci/hotplug/pciehp_hpc.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>>> index 5b8d749..ba8e06f 100644
>>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
>>> }
>>>
>>> if (slot_status & PCI_EXP_SLTSTA_CC) {
>>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
>>> if (!ctrl->no_cmd_complete) {
>>> /*
>>> * After 1 sec and CMD_COMPLETED still not set, just
>>> --
>>> 1.7.9.5
>>>
Problem #2: Looks like we have now 2 classes of systems:
a) The 5808639bfa98 was addressed towards systems that advertise the
"Command completion notification" capability, but do not notify of
command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits
are not written in SLOT_CTRL register (not implemented in
capabilities). Thus no command completion shall be generated for
pcie_disable_notifications() for e.g.
b) I have a system that where none of the above described HP elements
are present implemented, but the controller supports command
completion, and sends it out for every write of the slot control
register. Thus notification for command complete is generated for
pcie_disable_notifiction().
I don't believe there is a way to differentiate between these 2
classes of systems at init time, unless we try to generate a
notification and do or do not get one.
The PCIe specification section 7.8.10 seems to lean towards category
(b) of systems, but today this class of systems shall get penalized by
delay of 1 second (per controller) during probe. I can try and address
that, but admittedly I could not think of a better solution than
moving this code from pcie_init() to pcie_write_cmd().
> if (NO_CMD_CMPL(ctrl) ||
> !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
> ctrl->no_cmd_complete = 1;
Please advise.
Also, I'd appreciate if you could please let me know if there are any
comments on the patch for problem #1 above.
Thanks,
Rajat
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Bjorn Helgaas @ 2013-11-08 1:23 UTC (permalink / raw)
To: Rajat Jain
Cc: linux-pci, linux-hotplug, Kenji Kaneshige, Yijing Wang,
Guenter Roeck, Greg KH, Tom Nguyen, Kristen Accardi, Rajat Jain,
Rajat Jain
In-Reply-To: <CADTPrLQpaCDo_YCC2=MmT1iCrWRPqzDpnqQZrE8OMjRMc6WYmQ@mail.gmail.com>
On Thu, Nov 07, 2013 at 01:53:44PM -0800, Rajat Jain wrote:
> Hi Bjorn / list,
>
> I think there are two independent problems that need to be addressed.
>
> Problem #1: In any condition where (e.g. spurious interrupt) once the
> execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC
> bit is already set, and needs to be cleared. This does not happen
> today and hence this patch was aimed at solving that. Please note that
> this will not cause any problems to the systems that were fixed by
> 5808639bfa98, because this is equally applicable to any case.
Your patch might be the right thing to do, but something still niggles
at me, and I can't quite put my finger on it yet.
> >>>
> >>> drivers/pci/hotplug/pciehp_hpc.c | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> >>> index 5b8d749..ba8e06f 100644
> >>> --- a/drivers/pci/hotplug/pciehp_hpc.c
> >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> >>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
> >>> }
> >>>
> >>> if (slot_status & PCI_EXP_SLTSTA_CC) {
> >>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
> >>> if (!ctrl->no_cmd_complete) {
> >>> /*
> >>> * After 1 sec and CMD_COMPLETED still not set, just
> >>> --
> >>> 1.7.9.5
> >>>
>
> Problem #2: Looks like we have now 2 classes of systems:
>
> a) The 5808639bfa98 was addressed towards systems that advertise the
> "Command completion notification" capability, but do not notify of
> command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits
> are not written in SLOT_CTRL register (not implemented in
> capabilities). Thus no command completion shall be generated for
> pcie_disable_notifications() for e.g.
This seems pretty clearly out of spec. I read the 82801H spec for the
part used in the system in bz 10751, and the documentation seems to
comply with the spec. Possibly the part doesn't work as advertised,
but I think it's more likely there's some subtlety we're still missing.
I'm a little bit surprised that we'd be using pciehp on that Thinkpad R61
instead of acpiphp. The kernel in bz 10751 is so ancient that it might
not be negotiating correctly for control of pciehp.
> b) I have a system that where none of the above described HP elements
> are present implemented, but the controller supports command
> completion, and sends it out for every write of the slot control
> register. Thus notification for command complete is generated for
> pcie_disable_notifiction().
>
> I don't believe there is a way to differentiate between these 2
> classes of systems at init time, unless we try to generate a
> notification and do or do not get one.
>
> The PCIe specification section 7.8.10 seems to lean towards category
> (b) of systems, but today this class of systems shall get penalized by
> delay of 1 second (per controller) during probe.
Where does this delay happen on your system? I tried to work through
the path but I don't see it yet. Here's what I expect to happen on your
system:
pciehp_probe
pcie_init
readl(SLTCAP)
if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true
ctrl->no_cmd_complete = 1 # set (condition above is true)
writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC
pcie_disable_notification
pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE)
readw(SLTSTA) # CC = 0 (was cleared above)
writew(SLTCTL)
...
# no waiting here because no_cmd_complete = 1
...
hardware sets SLTSTA.CC
...
pcie_init_notification
pcie_enable_notification
pcie_write_cmd
readw(SLTSTA) # CC = 1 (was set by HW above)
if (SLTSTA.CC) # true
if (!NO_CMD_CMPL)) # true
dbg("Unexpected CMD_COMPLETED. ...")
ctrl->no_cmd_complete = 0
writew(SLTCTL)
...
# this time we wait because no_cmd_complete = 0
...
pcie_wait_cmd(..., poll = 1) # now no_cmd_complete = 0
pcie_poll_cmd
readw(SLTSTA) # CC = 1 on first read
if (SLTSTA.CC)
writew(SLTSTA, CC)
return 1
I would think you'd see the "Unexpected CMD_COMPLETED. Need to wait for
command completed event" message (if enabled), but I don't see where the
one second timeout would happen. It looks like we would call
pcie_poll_cmd(), but it would return immediately because SLTSTA.CC is
already set.
Bjorn
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Rajat Jain @ 2013-11-08 17:30 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-hotplug, Kenji Kaneshige, Yijing Wang,
Guenter Roeck, Greg KH, Tom Nguyen, Kristen Accardi, Rajat Jain,
Rajat Jain
In-Reply-To: <20131108012352.GE2955@google.com>
Hi,
On Thu, Nov 7, 2013 at 5:23 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Nov 07, 2013 at 01:53:44PM -0800, Rajat Jain wrote:
>> Hi Bjorn / list,
>>
>> I think there are two independent problems that need to be addressed.
>>
>> Problem #1: In any condition where (e.g. spurious interrupt) once the
>> execution reaches inside the if condition below, the PCI_EXP_SLTSTA_CC
>> bit is already set, and needs to be cleared. This does not happen
>> today and hence this patch was aimed at solving that. Please note that
>> this will not cause any problems to the systems that were fixed by
>> 5808639bfa98, because this is equally applicable to any case.
>
> Your patch might be the right thing to do, but something still niggles
> at me, and I can't quite put my finger on it yet.
>
>> >>>
>> >>> drivers/pci/hotplug/pciehp_hpc.c | 1 +
>> >>> 1 file changed, 1 insertion(+)
>> >>>
>> >>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> >>> index 5b8d749..ba8e06f 100644
>> >>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> >>> @@ -185,6 +185,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
>> >>> }
>> >>>
>> >>> if (slot_status & PCI_EXP_SLTSTA_CC) {
>> >>> + pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
>> >>> if (!ctrl->no_cmd_complete) {
>> >>> /*
>> >>> * After 1 sec and CMD_COMPLETED still not set, just
>> >>> --
>> >>> 1.7.9.5
>> >>>
>>
>> Problem #2: Looks like we have now 2 classes of systems:
>>
>> a) The 5808639bfa98 was addressed towards systems that advertise the
>> "Command completion notification" capability, but do not notify of
>> command completion in case MRL, Power Ctrl, Attn LED and Pwr LED bits
>> are not written in SLOT_CTRL register (not implemented in
>> capabilities). Thus no command completion shall be generated for
>> pcie_disable_notifications() for e.g.
>
> This seems pretty clearly out of spec. I read the 82801H spec for the
> part used in the system in bz 10751, and the documentation seems to
> comply with the spec. Possibly the part doesn't work as advertised,
> but I think it's more likely there's some subtlety we're still missing.
>
> I'm a little bit surprised that we'd be using pciehp on that Thinkpad R61
> instead of acpiphp. The kernel in bz 10751 is so ancient that it might
> not be negotiating correctly for control of pciehp.
>
>> b) I have a system that where none of the above described HP elements
>> are present implemented, but the controller supports command
>> completion, and sends it out for every write of the slot control
>> register. Thus notification for command complete is generated for
>> pcie_disable_notifiction().
>>
>> I don't believe there is a way to differentiate between these 2
>> classes of systems at init time, unless we try to generate a
>> notification and do or do not get one.
>>
>> The PCIe specification section 7.8.10 seems to lean towards category
>> (b) of systems, but today this class of systems shall get penalized by
>> delay of 1 second (per controller) during probe.
>
> Where does this delay happen on your system? I tried to work through
> the path but I don't see it yet. Here's what I expect to happen on your
> system:
>
> pciehp_probe
> pcie_init
> readl(SLTCAP)
> if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true
> ctrl->no_cmd_complete = 1 # set (condition above is true)
> writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC
> pcie_disable_notification
> pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE)
> readw(SLTSTA) # CC = 0 (was cleared above)
> writew(SLTCTL)
> ...
> # no waiting here because no_cmd_complete = 1
> ...
> hardware sets SLTSTA.CC
> ...
> pcie_init_notification
> pcie_enable_notification
> pcie_write_cmd
> readw(SLTSTA) # CC = 1 (was set by HW above)
> if (SLTSTA.CC) # true
> if (!NO_CMD_CMPL)) # true
> dbg("Unexpected CMD_COMPLETED. ...")
> ctrl->no_cmd_complete = 0
> writew(SLTCTL)
> ...
> # this time we wait because no_cmd_complete = 0
> ...
> pcie_wait_cmd(..., poll = 1) # now no_cmd_complete = 0
Actually pcie_wait_cmd() shall be called with poll=0. That is because
the following conditions are both false (we just enabled both the
interrupts while enabling notifications):
if (!(slot_ctrl & PCI_EXP_SLTCTL_HPIE) ||
!(slot_ctrl & PCI_EXP_SLTCTL_CCIE))
poll = 1;
Thus it does not poll, but waits for interrupt. Which does not happen
because the CC bit in the slot status register was not cleared. As a
result, we get the following messages on the console for each
controller (and 1 second delay):
=============Unexpected CMD_COMPLETED. Need to wait for command completed event.
Command not completed in 1000 msec
=============With my patch, we'd eliminate the 1 second delay and the second line
of output above. If we also want to get rid of the first line
"Unexpected CMD_COMPLETED..." also, that would probably have to be
solved by changing the behavior to assume that CC shall be generated
for every SLOTCTRL register write.
Please suggest.
Thanks,
Rajat
> pcie_poll_cmd
> readw(SLTSTA) # CC = 1 on first read
> if (SLTSTA.CC)
> writew(SLTSTA, CC)
> return 1
>
> I would think you'd see the "Unexpected CMD_COMPLETED. Need to wait for
> command completed event" message (if enabled),
> but I don't see where the
> one second timeout would happen. It looks like we would call
> pcie_poll_cmd(), but it would return immediately because SLTSTA.CC is
> already set.
>
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Bjorn Helgaas @ 2013-11-08 23:15 UTC (permalink / raw)
To: Rajat Jain
Cc: linux-pci@vger.kernel.org, linux hotplug mailing, Kenji Kaneshige,
Yijing Wang, Guenter Roeck, Greg KH, Tom Nguyen, Kristen Accardi,
Rajat Jain, Rajat Jain
In-Reply-To: <CADTPrLTYOXsGovL8HJ7=+K30OWVeSdMKTma_UuMV8ZRFB6CkYA@mail.gmail.com>
On Fri, Nov 8, 2013 at 10:30 AM, Rajat Jain <rajatjain.linux@gmail.com> wrote:
> On Thu, Nov 7, 2013 at 5:23 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> Where does this delay happen on your system? I tried to work through
>> the path but I don't see it yet. Here's what I expect to happen on your
>> system:
>>
>> pciehp_probe
>> pcie_init
>> readl(SLTCAP)
>> if (NO_CMD_CMPL || !(POWER_CTRL | ATTN_LED | PWR_LED | EMI)) # true
>> ctrl->no_cmd_complete = 1 # set (condition above is true)
>> writew(SLTSTA, 0x1f) # clear ABP PFD MRLSC PDC CC
>> pcie_disable_notification
>> pcie_write_cmd(0, PDCE | ABPE | MRLSCE | PFDE | HPIE | CCIE | DLLSCE)
>> readw(SLTSTA) # CC = 0 (was cleared above)
>> writew(SLTCTL)
>> ...
>> # no waiting here because no_cmd_complete = 1
>> ...
>> hardware sets SLTSTA.CC
>> ...
>> pcie_init_notification
>> pcie_enable_notification
>> pcie_write_cmd
>> readw(SLTSTA) # CC = 1 (was set by HW above)
>> if (SLTSTA.CC) # true
>> if (!NO_CMD_CMPL)) # true
>> dbg("Unexpected CMD_COMPLETED. ...")
>> ctrl->no_cmd_complete = 0
>> writew(SLTCTL)
>> ...
>> # this time we wait because no_cmd_complete = 0
>> ...
>> pcie_wait_cmd(..., poll = 1) # now no_cmd_complete = 0
>
> Actually pcie_wait_cmd() shall be called with poll=0. That is because
> the following conditions are both false (we just enabled both the
> interrupts while enabling notifications):
>
> if (!(slot_ctrl & PCI_EXP_SLTCTL_HPIE) ||
> !(slot_ctrl & PCI_EXP_SLTCTL_CCIE))
> poll = 1;
>
> Thus it does not poll, but waits for interrupt. Which does not happen
> because the CC bit in the slot status register was not cleared.
Right, of course. Thanks for pointing out my error.
> As a
> result, we get the following messages on the console for each
> controller (and 1 second delay):
> =============> Unexpected CMD_COMPLETED. Need to wait for command completed event.
> Command not completed in 1000 msec
> =============> With my patch, we'd eliminate the 1 second delay and the second line
> of output above. If we also want to get rid of the first line
> "Unexpected CMD_COMPLETED..." also, that would probably have to be
> solved by changing the behavior to assume that CC shall be generated
> for every SLOTCTRL register write.
I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint.
We shouldn't complain about hardware that is working perfectly fine.
I don't know the best way to do that yet. I have found a box with the
same hardware that was fixed by 5808639bfa98, so I hope to play with
it and figure out something that will work nicely for both scenarios.
BTW, can you open a report at bugzilla.kernel.org and attach your
"lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd
like to have the details archived somewhere.
Bjorn
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Rajat Jain @ 2013-11-11 21:26 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci@vger.kernel.org, linux hotplug mailing, Kenji Kaneshige,
Yijing Wang, Greg KH, Tom Nguyen, Kristen Accardi, Rajat Jain,
Rajat Jain, Guenter Roeck
In-Reply-To: <CAErSpo7MH=+Jk-ujBoHquk2e+no1cqEV6p=R1E6LGvcLSJQyKw@mail.gmail.com>
Hello,
>> With my patch, we'd eliminate the 1 second delay and the second line
>> of output above. If we also want to get rid of the first line
>> "Unexpected CMD_COMPLETED..." also, that would probably have to be
>> solved by changing the behavior to assume that CC shall be generated
>> for every SLOTCTRL register write.
>
> I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint.
> We shouldn't complain about hardware that is working perfectly fine.
> I don't know the best way to do that yet. I have found a box with the
> same hardware that was fixed by 5808639bfa98, so I hope to play with
> it and figure out something that will work nicely for both scenarios.
Please keep posted :-)
>
> BTW, can you open a report at bugzilla.kernel.org and attach your
> "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd
> like to have the details archived somewhere.
Done:
https://bugzilla.kernel.org/show_bug.cgi?idd821
Please let me know if you need any help in trying something out - I'd
be more than keen to help - write code or test.
BTW, I figured another way that solves the current problem at hand
(for both kind of controllers). Today the events are cleared just
before call to pcie_disable_notification, if we clear them afterwards
also, the current problem is taken care of for all controllers. Of
course this does not look the right way to solve the problem though
(since the ctrl->no_cmd_completed shall still be 1, which is not
right).
@@ -948,6 +948,10 @@ struct controller *pcie_init(struct pcie_device *dev)
/* Clear all remaining event bits in Slot Status register */
if (pciehp_writew(ctrl, PCI_EXP_SLTSTA, 0x1f))
goto abort_ctrl;
/* Disable sotfware notification */
pcie_disable_notification(ctrl);
+ /* Clear all remaining event bits in Slot Status register */
+ if (pciehp_writew(ctrl, PCI_EXP_SLTSTA, 0x1f))
+ goto abort_ctrl;
+
ctrl_info(ctrl, "HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
pdev->vendor, pdev->device, pdev->subsystem_vendor,
pdev->subsystem_device);
Thanks,
Rajat
^ permalink raw reply
* [RFC PATCH 0/4] Allow Link state changes for Hot-Plug
From: Rajat Jain @ 2013-11-19 22:39 UTC (permalink / raw)
To: Linux-PCI, Linux-hotplug, linux-kernel
Cc: Bjorn Helgaas, Guenter Roeck, Kenji Kaneshige, Alex Williamson,
Yijing Wang, Paul Bolle, Eric W. Biederman, Rajat Jain,
Rajat Jain, Guenter Roeck
Hello,
This patch set enables the use of PCI Express link up and link down events
for Hotplug or Unplug. The requirement of such a feature was originally
discussed here:
http://www.spinics.net/lists/linux-pci/msg05783.html
http://www.spinics.net/lists/hotplug/msg05801.html
Patch [1/4]: makes a function non-static for use by patch 2.
Patch [2/4]: Contains the bulk logic to allow link events to be used
for hotplug and removal.
Patch [3/4]: Makes the pciehp_power_thread() lock free by making it
look at a work info->req instead of slot->state.
Patch [4/4]: Introduce slot->hotplug_lock to serialize the hotplug
operations.
I'd appreciate if you could please review and provide me with any comments.
Thanks,
Rajat
Rajat Jain (4):
pciehp: Make check_link_active() non-static
pciehp: Use link state change notifications for hot-plug and removal
pciehp: Ensure all hotplug events are processed, even very fast ones.
pciehp: Introduce hotplug_lock to serialize HP events on each slot
drivers/pci/hotplug/pciehp.h | 6 ++
drivers/pci/hotplug/pciehp_core.c | 10 +-
drivers/pci/hotplug/pciehp_ctrl.c | 181 ++++++++++++++++++++++++++++++++++---
drivers/pci/hotplug/pciehp_hpc.c | 63 +++++++++----
4 files changed, 229 insertions(+), 31 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [RFC PATCH 1/4] pciehp: Make check_link_active() non-static
From: Rajat Jain @ 2013-11-19 22:58 UTC (permalink / raw)
To: Linux-PCI, Linux-hotplug, linux-kernel
Cc: Bjorn Helgaas, Guenter Roeck, Kenji Kaneshige, Alex Williamson,
Yijing Wang, Paul Bolle, Eric W. Biederman, Rajat Jain,
Rajat Jain, Guenter Roeck
check_link_active() functionality needs to be used by subsequent patches
(that introduce link state change based hotplug). Thus make the function
non-static, and rename it to pciehp_check_link_active() so as to be
consistent with other non-static functions.
Signed-off-by: Rajat Jain <rajatjain@juniper.net>
---
drivers/pci/hotplug/pciehp.h | 1 +
drivers/pci/hotplug/pciehp_hpc.c | 6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 541bbe6..fc322ed 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -154,6 +154,7 @@ void pciehp_green_led_on(struct slot *slot);
void pciehp_green_led_off(struct slot *slot);
void pciehp_green_led_blink(struct slot *slot);
int pciehp_check_link_status(struct controller *ctrl);
+bool pciehp_check_link_active(struct controller *ctrl);
void pciehp_release_ctrl(struct controller *ctrl);
int pciehp_reset_slot(struct slot *slot, int probe);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 51f56ef..3a5eee7 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -241,7 +241,7 @@ static int pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask)
return retval;
}
-static bool check_link_active(struct controller *ctrl)
+bool pciehp_check_link_active(struct controller *ctrl)
{
bool ret = false;
u16 lnk_status;
@@ -261,12 +261,12 @@ static void __pcie_wait_link_active(struct controller *ctrl, bool active)
{
int timeout = 1000;
- if (check_link_active(ctrl) = active)
+ if (pciehp_check_link_active(ctrl) = active)
return;
while (timeout > 0) {
msleep(10);
timeout -= 10;
- if (check_link_active(ctrl) = active)
+ if (pciehp_check_link_active(ctrl) = active)
return;
}
ctrl_dbg(ctrl, "Data Link Layer Link Active not %s in 1000 msec\n",
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH 2/4] pciehp: Use link change notifications for hot-plug and, removal
From: Rajat Jain @ 2013-11-19 22:59 UTC (permalink / raw)
To: Linux-PCI, Linux-hotplug, linux-kernel
Cc: Bjorn Helgaas, Guenter Roeck, Kenji Kaneshige, Alex Williamson,
Yijing Wang, Paul Bolle, Eric W. Biederman, Rajat Jain,
Rajat Jain, Guenter Roeck
A lot of systems do not have the fancy buttons and LEDs, and instead
want to rely only on the Link state change events to drive the hotplug
and removal state machinery.
(http://www.spinics.net/lists/hotplug/msg05802.html)
This patch adds support for that functionality. Here are the details
about the patch itself:
* Add "pciehp_use_link_events" module parameter to indicate link state
changes to be used for hot-plug.
* Define and use interrupt events for linkup / linkdown.
* Introduce the functions to handle link-up and link-down events and
queue the work in the slot->wq to be processed by pciehp_power_thread
* Many ports use inband mechanism such as receiver detection to find out
whether an adapter is present. In such HP ports, when link goes down,
adapter presence will also toggle. Thus don't bail out the removal
process initiated by link down, if adapter is not found to be present.
* The current pciehp driver disabled the link in case of a hot-removal.
Since for link change based hot-plug to work, we need that enabled,
hence make sure to not disable the link permanently if link state
based hot-plug is to be used.
* pciehp_reset_slot - reset of secondary bus may cause undesirable
spurious link notifications. Thus disable these around the secondary
bus reset.
Signed-off-by: Rajat Jain <rajatjain@juniper.net>
---
drivers/pci/hotplug/pciehp.h | 4 ++
drivers/pci/hotplug/pciehp_core.c | 3 +
drivers/pci/hotplug/pciehp_ctrl.c | 132 ++++++++++++++++++++++++++++++++++++-
drivers/pci/hotplug/pciehp_hpc.c | 56 +++++++++++-----
4 files changed, 179 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index fc322ed..64a6840 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -44,6 +44,7 @@ extern bool pciehp_poll_mode;
extern int pciehp_poll_time;
extern bool pciehp_debug;
extern bool pciehp_force;
+extern bool pciehp_use_link_events; /* Use link state events for hotplug */
#define dbg(format, arg...) \
do { \
@@ -110,6 +111,8 @@ struct controller {
#define INT_BUTTON_PRESS 7
#define INT_BUTTON_RELEASE 8
#define INT_BUTTON_CANCEL 9
+#define INT_LINK_UP 10
+#define INT_LINK_DOWN 11
#define STATIC_STATE 0
#define BLINKINGON_STATE 1
@@ -133,6 +136,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot);
u8 pciehp_handle_switch_change(struct slot *p_slot);
u8 pciehp_handle_presence_change(struct slot *p_slot);
u8 pciehp_handle_power_fault(struct slot *p_slot);
+u8 pciehp_handle_linkstate_change(struct slot *p_slot);
int pciehp_configure_device(struct slot *p_slot);
int pciehp_unconfigure_device(struct slot *p_slot);
void pciehp_queue_pushbutton_work(struct work_struct *work);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index f4a18f5..5a2c43c 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -42,6 +42,7 @@ bool pciehp_debug;
bool pciehp_poll_mode;
int pciehp_poll_time;
bool pciehp_force;
+bool pciehp_use_link_events;
#define DRIVER_VERSION "0.4"
#define DRIVER_AUTHOR "Dan Zink <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
@@ -55,10 +56,12 @@ module_param(pciehp_debug, bool, 0644);
module_param(pciehp_poll_mode, bool, 0644);
module_param(pciehp_poll_time, int, 0644);
module_param(pciehp_force, bool, 0644);
+module_param(pciehp_use_link_events, bool, 0644);
MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not");
MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not");
MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds");
MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if OSHP is missing");
+MODULE_PARM_DESC(pciehp_use_link_events, "Use link state change events to drive pciehp hot-plug");
#define PCIE_MODULE_NAME "pciehp"
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 38f0186..3899b52 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -150,6 +150,29 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
return 1;
}
+u8 pciehp_handle_linkstate_change(struct slot *p_slot)
+{
+ u32 event_type;
+ struct controller *ctrl = p_slot->ctrl;
+
+ /* Link Status Change */
+ ctrl_dbg(ctrl, "Data Link Layer State change\n");
+
+ if (pciehp_check_link_active(ctrl)) {
+ ctrl_info(ctrl, "slot(%s): Link Up event\n",
+ slot_name(p_slot));
+ event_type = INT_LINK_UP;
+ } else {
+ ctrl_info(ctrl, "slot(%s): Link Down event\n",
+ slot_name(p_slot));
+ event_type = INT_LINK_DOWN;
+ }
+
+ queue_interrupt_event(p_slot, event_type);
+
+ return 1;
+}
+
/* The following routines constitute the bulk of the
hotplug controller logic
*/
@@ -442,6 +465,100 @@ static void handle_surprise_event(struct slot *p_slot)
queue_work(p_slot->wq, &info->work);
}
+/*
+ * Note: This function must be called with slot->lock held
+ */
+static void handle_link_up_event(struct slot *p_slot)
+{
+ struct controller *ctrl = p_slot->ctrl;
+ struct power_work_info *info;
+
+ info = kmalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
+ __func__);
+ return;
+ }
+ info->p_slot = p_slot;
+ INIT_WORK(&info->work, pciehp_power_thread);
+
+ switch (p_slot->state) {
+ case BLINKINGON_STATE:
+ case BLINKINGOFF_STATE:
+ cancel_delayed_work(&p_slot->work);
+ /* Fall through */
+ case STATIC_STATE:
+ p_slot->state = POWERON_STATE;
+ queue_work(p_slot->wq, &info->work);
+ break;
+ case POWERON_STATE:
+ ctrl_info(ctrl,
+ "Link Up event ignored on slot(%s): already powering on\n",
+ slot_name(p_slot));
+ kfree(info);
+ break;
+ case POWEROFF_STATE:
+ ctrl_info(ctrl,
+ "Link Up event queued on slot(%s): currently getting powered off\n",
+ slot_name(p_slot));
+ p_slot->state = POWERON_STATE;
+ queue_work(p_slot->wq, &info->work);
+ break;
+ default:
+ ctrl_err(ctrl, "Not a valid state on slot(%s)\n",
+ slot_name(p_slot));
+ kfree(info);
+ break;
+ }
+}
+
+/*
+ * Note: This function must be called with slot->lock held
+ */
+static void handle_link_down_event(struct slot *p_slot)
+{
+ struct controller *ctrl = p_slot->ctrl;
+ struct power_work_info *info;
+
+ info = kmalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ ctrl_err(p_slot->ctrl, "%s: Cannot allocate memory\n",
+ __func__);
+ return;
+ }
+ info->p_slot = p_slot;
+ INIT_WORK(&info->work, pciehp_power_thread);
+
+ switch (p_slot->state) {
+ case BLINKINGON_STATE:
+ case BLINKINGOFF_STATE:
+ cancel_delayed_work(&p_slot->work);
+ /* Fall through */
+ case STATIC_STATE:
+ p_slot->state = POWEROFF_STATE;
+ queue_work(p_slot->wq, &info->work);
+ break;
+ case POWEROFF_STATE:
+ ctrl_info(ctrl,
+ "Link Down event ignored on slot(%s): already powering off\n",
+ slot_name(p_slot));
+ kfree(info);
+ break;
+ case POWERON_STATE:
+ ctrl_info(ctrl,
+ "Link Down event queued on slot(%s): currently getting powered on\n",
+ slot_name(p_slot));
+ p_slot->state = POWEROFF_STATE;
+ queue_work(p_slot->wq, &info->work);
+ break;
+ default:
+ ctrl_err(ctrl, "Not a valid state on slot %s\n",
+ slot_name(p_slot));
+ kfree(info);
+ break;
+ }
+}
+
static void interrupt_event_handler(struct work_struct *work)
{
struct event_info *info = container_of(work, struct event_info, work);
@@ -468,6 +585,12 @@ static void interrupt_event_handler(struct work_struct *work)
ctrl_dbg(ctrl, "Surprise Removal\n");
handle_surprise_event(p_slot);
break;
+ case INT_LINK_UP:
+ handle_link_up_event(p_slot);
+ break;
+ case INT_LINK_DOWN:
+ handle_link_down_event(p_slot);
+ break;
default:
break;
}
@@ -524,7 +647,14 @@ int pciehp_disable_slot(struct slot *p_slot)
if (!p_slot->ctrl)
return 1;
- if (!HP_SUPR_RM(p_slot->ctrl)) {
+ /*
+ * In certain scenarios, adapter may have already disappeared by this
+ * time. E.g (1) Surprise Removal (2) Many ports use in-band
+ * mechanisms to signal if adapter is present or not. Thus when a
+ * link goes down, the device may have already gone away by the time
+ * this code executes.
+ */
+ if (!HP_SUPR_RM(p_slot->ctrl) && !pciehp_use_link_events) {
ret = pciehp_get_adapter_status(p_slot, &getstatus);
if (ret || !getstatus) {
ctrl_info(ctrl, "No adapter on slot(%s)\n",
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 3a5eee7..90eb8c6 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -620,13 +620,21 @@ int pciehp_power_off_slot(struct slot * slot)
u16 cmd_mask;
int retval;
- /* Disable the link at first */
- pciehp_link_disable(ctrl);
- /* wait the link is down */
- if (ctrl->link_active_reporting)
- pcie_wait_link_not_active(ctrl);
- else
- msleep(1000);
+ if (!pciehp_use_link_events) {
+ /*
+ * In case it is desired to use the link state events for
+ * hot-plug, make sure to NOT disable the link permanently, or
+ * else we might never get a link-up hotplug event again.
+ */
+
+ /* Disable the link first */
+ pciehp_link_disable(ctrl);
+ /* wait until the link is down */
+ if (ctrl->link_active_reporting)
+ pcie_wait_link_not_active(ctrl);
+ else
+ msleep(1000);
+ }
slot_cmd = POWER_OFF;
cmd_mask = PCI_EXP_SLTCTL_PCC;
@@ -661,7 +669,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
- PCI_EXP_SLTSTA_CC);
+ PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
detected &= ~intr_loc;
intr_loc |= detected;
if (!intr_loc)
@@ -702,6 +710,10 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
ctrl->power_fault_detected = 1;
pciehp_handle_power_fault(slot);
}
+
+ if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
+ pciehp_handle_linkstate_change(slot);
+
return IRQ_HANDLED;
}
@@ -724,12 +736,15 @@ int pcie_enable_notification(struct controller *ctrl)
cmd |= PCI_EXP_SLTCTL_ABPE;
if (MRL_SENS(ctrl))
cmd |= PCI_EXP_SLTCTL_MRLSCE;
+ if (pciehp_use_link_events)
+ cmd |= PCI_EXP_SLTCTL_DLLSCE;
if (!pciehp_poll_mode)
cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE;
mask = (PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE |
PCI_EXP_SLTCTL_MRLSCE | PCI_EXP_SLTCTL_PFDE |
- PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE);
+ PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE |
+ PCI_EXP_SLTCTL_DLLSCE);
if (pcie_write_cmd(ctrl, cmd, mask)) {
ctrl_err(ctrl, "Cannot enable software notification\n");
@@ -751,28 +766,39 @@ static void pcie_disable_notification(struct controller *ctrl)
/*
* pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
- * bus reset of the bridge, but if the slot supports surprise removal we need
- * to disable presence detection around the bus reset and clear any spurious
+ * bus reset of the bridge, but if the slot supports surprise removal (or
+ * link state change based hotplug), we need to disable presence detection
+ * (or link state notifications) around the bus reset and clear any spurious
* events after.
*/
int pciehp_reset_slot(struct slot *slot, int probe)
{
struct controller *ctrl = slot->ctrl;
+ u16 stat_mask = 0, ctrl_mask = 0;
if (probe)
return 0;
if (HP_SUPR_RM(ctrl)) {
- pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
+ ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
+ stat_mask |= PCI_EXP_SLTSTA_PDC;
+ }
+ if (pciehp_use_link_events) {
+ ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
+ stat_mask |= PCI_EXP_SLTSTA_DLLSC;
+ }
+
+ if (ctrl_mask) {
+ pcie_write_cmd(ctrl, 0, ctrl_mask);
if (pciehp_poll_mode)
del_timer_sync(&ctrl->poll_timer);
}
pci_reset_bridge_secondary_bus(ctrl->pcie->port);
- if (HP_SUPR_RM(ctrl)) {
- pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDC);
- pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
+ if (ctrl_mask) {
+ pciehp_writew(ctrl, PCI_EXP_SLTSTA, stat_mask);
+ pcie_write_cmd(ctrl, ctrl_mask, ctrl_mask);
if (pciehp_poll_mode)
int_poll_timeout(ctrl->poll_timer.data);
}
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH 3/4] pciehp: Ensure very fast hotplug events are also processed.
From: Rajat Jain @ 2013-11-19 23:01 UTC (permalink / raw)
To: Linux-PCI, Linux-hotplug, linux-kernel
Cc: Bjorn Helgaas, Guenter Roeck, Kenji Kaneshige, Alex Williamson,
Yijing Wang, Paul Bolle, Eric W. Biederman, Rajat Jain,
Rajat Jain, Guenter Roeck
Today, this is how all the hotplug and unplug events work:
Hotplug / Removal needs to be done
=> Set slot->state (protected by slot->lock) to either
POWERON_STATE (for enabling) or POWEROFF_STATE (for disabling).
=> Submit the work item for pciehp_power_thread() to slot->wq.
Problem:
There is a problem if the hotplug events can happen fast enough that
they do not give SW enough time to add or remove the new devices.
=> Assume: Event for unplug comes (e.g. surprise removal). But
before the pciehp_power_thread() work item was executed, the
card was replaced by another card, causing surprise hotplug event.
=> What goes wrong:
=> The hot-removal event sets slot->state to POWEROFF_STATE, and
schedules the pciehp_power_thread().
=> The hot-add event sets slot->state to POWERON_STATE, and
schedules the pciehp_power_thread().
=> Now the pciehp_power_thread() is scheduled twice, and on both
occasions it will find POWERON_STATE and will try to add the
devices on the slot, and will fail complaining that the devices
already exist.
=> Why this is a problem: If the device was replaced between the hot
removal and hot-add, then we should unload the old driver and
reload the new one. This does not happen today. The kernel or the
driver is not even aware that the device was replaced.
The problem is that the pciehp_power_thread() only looks at the
slot->state which would only contain the *latest* state - not
the actual event (add / remove) that was the intent of the IRQ
handler who submitted the work.
What this patch does:
=> Hotplug events pass on an actual request (for addition or removal)
to pciehp_power_thread() which is local to that work item
submission.
=> pciehp_power_thread() does not need to look at slote->state and
hence no locks needed in that.
=> Essentially this results in all the hotplug and unplug events
"replayed" by pciehp_power_thread().
Signed-off-by: Rajat Jain <rajatjain@juniper.net>
---
drivers/pci/hotplug/pciehp_ctrl.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 3899b52..83dbe08 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -299,6 +299,9 @@ static int remove_board(struct slot *p_slot)
struct power_work_info {
struct slot *p_slot;
struct work_struct work;
+ unsigned int req;
+#define DISABLE_REQ 0
+#define ENABLE_REQ 1
};
/**
@@ -314,10 +317,8 @@ static void pciehp_power_thread(struct work_struct *work)
container_of(work, struct power_work_info, work);
struct slot *p_slot = info->p_slot;
- mutex_lock(&p_slot->lock);
- switch (p_slot->state) {
- case POWEROFF_STATE:
- mutex_unlock(&p_slot->lock);
+ switch (info->req) {
+ case DISABLE_REQ:
ctrl_dbg(p_slot->ctrl,
"Disabling domain:bus:device=%04x:%02x:00\n",
pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
@@ -325,18 +326,22 @@ static void pciehp_power_thread(struct work_struct *work)
pciehp_disable_slot(p_slot);
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
- break;
- case POWERON_STATE:
mutex_unlock(&p_slot->lock);
+ break;
+ case ENABLE_REQ:
+ ctrl_dbg(p_slot->ctrl,
+ "Enabling domain:bus:device=%04x:%02x:00\n",
+ pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
+ p_slot->ctrl->pcie->port->subordinate->number);
if (pciehp_enable_slot(p_slot) && PWR_LED(p_slot->ctrl))
pciehp_green_led_off(p_slot);
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
+ mutex_unlock(&p_slot->lock);
break;
default:
break;
}
- mutex_unlock(&p_slot->lock);
kfree(info);
}
@@ -359,9 +364,11 @@ void pciehp_queue_pushbutton_work(struct work_struct *work)
switch (p_slot->state) {
case BLINKINGOFF_STATE:
p_slot->state = POWEROFF_STATE;
+ info->req = DISABLE_REQ;
break;
case BLINKINGON_STATE:
p_slot->state = POWERON_STATE;
+ info->req = ENABLE_REQ;
break;
default:
kfree(info);
@@ -457,10 +464,13 @@ static void handle_surprise_event(struct slot *p_slot)
INIT_WORK(&info->work, pciehp_power_thread);
pciehp_get_adapter_status(p_slot, &getstatus);
- if (!getstatus)
+ if (!getstatus) {
p_slot->state = POWEROFF_STATE;
- else
+ info->req = DISABLE_REQ;
+ } else {
p_slot->state = POWERON_STATE;
+ info->req = ENABLE_REQ;
+ }
queue_work(p_slot->wq, &info->work);
}
@@ -489,6 +499,7 @@ static void handle_link_up_event(struct slot *p_slot)
/* Fall through */
case STATIC_STATE:
p_slot->state = POWERON_STATE;
+ info->req = ENABLE_REQ;
queue_work(p_slot->wq, &info->work);
break;
case POWERON_STATE:
@@ -502,6 +513,7 @@ static void handle_link_up_event(struct slot *p_slot)
"Link Up event queued on slot(%s): currently getting powered off\n",
slot_name(p_slot));
p_slot->state = POWERON_STATE;
+ info->req = ENABLE_REQ;
queue_work(p_slot->wq, &info->work);
break;
default:
@@ -536,6 +548,7 @@ static void handle_link_down_event(struct slot *p_slot)
/* Fall through */
case STATIC_STATE:
p_slot->state = POWEROFF_STATE;
+ info->req = DISABLE_REQ;
queue_work(p_slot->wq, &info->work);
break;
case POWEROFF_STATE:
@@ -549,6 +562,7 @@ static void handle_link_down_event(struct slot *p_slot)
"Link Down event queued on slot(%s): currently getting powered on\n",
slot_name(p_slot));
p_slot->state = POWEROFF_STATE;
+ info->req = DISABLE_REQ;
queue_work(p_slot->wq, &info->work);
break;
default:
--
1.7.9.5
^ permalink raw reply related
* [RFC PATCH 4/4] pciehp: Introduce hotplug_lock to serialize HP events
From: Rajat Jain @ 2013-11-19 23:06 UTC (permalink / raw)
To: Linux-PCI, Linux-hotplug, linux-kernel
Cc: Bjorn Helgaas, Guenter Roeck, Kenji Kaneshige, Alex Williamson,
Yijing Wang, Paul Bolle, Eric W. Biederman, Rajat Jain,
Rajat Jain, Guenter Roeck
Today it is there is no protection around pciehp_enable_slot() and
pciehp_disable_slot() to ensure that they complete before another
hot-plug operation can be done on that particular slot.
This patch introduces the slot->hotplug_lock to ensure that any
hotplug operations (add / remove) complete before another HP event
can begin processing on that particular slot.
Signed-off-by: Rajat Jain <rajatjain@juniper.net>
---
The problem we are trying to solve is that it should not be the
case that the PCI tree is left in an unpredictable or bad state
because pciehp_disable_slot() may get to execute before
pciehp_enable_slot() completes its execution.
I'm not 100% confident that this particular patch is needed. If some one can
confirm me, that the problem this patch is trying to solve, does not exist,
I'll be happy to do away without this patch.
drivers/pci/hotplug/pciehp.h | 1 +
drivers/pci/hotplug/pciehp_core.c | 7 ++++++-
drivers/pci/hotplug/pciehp_ctrl.c | 17 +++++++++++++++--
drivers/pci/hotplug/pciehp_hpc.c | 1 +
4 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 64a6840..fdf2038 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -78,6 +78,7 @@ struct slot {
struct hotplug_slot *hotplug_slot;
struct delayed_work work; /* work for button event */
struct mutex lock;
+ struct mutex hotplug_lock;
struct workqueue_struct *wq;
};
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 5a2c43c..2175f7a 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -281,8 +281,11 @@ static int pciehp_probe(struct pcie_device *dev)
slot = ctrl->slot;
pciehp_get_adapter_status(slot, &occupied);
pciehp_get_power_status(slot, &poweron);
- if (occupied && pciehp_force)
+ if (occupied && pciehp_force) {
+ mutex_lock(&slot->hotplug_lock);
pciehp_enable_slot(slot);
+ mutex_unlock(&slot->hotplug_lock);
+ }
/* If empty slot's power status is on, turn power off */
if (!occupied && poweron && POWER_CTRL(ctrl))
pciehp_power_off_slot(slot);
@@ -326,10 +329,12 @@ static int pciehp_resume (struct pcie_device *dev)
/* Check if slot is occupied */
pciehp_get_adapter_status(slot, &status);
+ mutex_lock(&slot->hotplug_lock);
if (status)
pciehp_enable_slot(slot);
else
pciehp_disable_slot(slot);
+ mutex_unlock(&slot->hotplug_lock);
return 0;
}
#endif /* PM */
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 83dbe08..8ad63a4 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -316,6 +316,7 @@ static void pciehp_power_thread(struct work_struct *work)
struct power_work_info *info container_of(work, struct power_work_info, work);
struct slot *p_slot = info->p_slot;
+ int ret;
switch (info->req) {
case DISABLE_REQ:
@@ -323,7 +324,9 @@ static void pciehp_power_thread(struct work_struct *work)
"Disabling domain:bus:device=%04x:%02x:00\n",
pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
p_slot->ctrl->pcie->port->subordinate->number);
+ mutex_lock(&p_slot->hotplug_lock);
pciehp_disable_slot(p_slot);
+ mutex_unlock(&p_slot->hotplug_lock);
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
mutex_unlock(&p_slot->lock);
@@ -333,7 +336,10 @@ static void pciehp_power_thread(struct work_struct *work)
"Enabling domain:bus:device=%04x:%02x:00\n",
pci_domain_nr(p_slot->ctrl->pcie->port->subordinate),
p_slot->ctrl->pcie->port->subordinate->number);
- if (pciehp_enable_slot(p_slot) && PWR_LED(p_slot->ctrl))
+ mutex_lock(&p_slot->hotplug_lock);
+ ret = pciehp_enable_slot(p_slot);
+ mutex_unlock(&p_slot->hotplug_lock);
+ if (ret && PWR_LED(p_slot->ctrl))
pciehp_green_led_off(p_slot);
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
@@ -613,6 +619,9 @@ static void interrupt_event_handler(struct work_struct *work)
kfree(info);
}
+/*
+ * Note: This function must be called with slot->hotplug_lock held
+ */
int pciehp_enable_slot(struct slot *p_slot)
{
u8 getstatus = 0;
@@ -651,7 +660,9 @@ int pciehp_enable_slot(struct slot *p_slot)
return rc;
}
-
+/*
+ * Note: This function must be called with slot->hotplug_lock held
+ */
int pciehp_disable_slot(struct slot *p_slot)
{
u8 getstatus = 0;
@@ -710,7 +721,9 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
case STATIC_STATE:
p_slot->state = POWERON_STATE;
mutex_unlock(&p_slot->lock);
+ mutex_lock(&p_slot->hotplug_lock);
retval = pciehp_enable_slot(p_slot);
+ mutex_unlock(&p_slot->hotplug_lock);
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
break;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 90eb8c6..740d883 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -841,6 +841,7 @@ static int pcie_init_slot(struct controller *ctrl)
slot->ctrl = ctrl;
mutex_init(&slot->lock);
+ mutex_init(&slot->hotplug_lock);
INIT_DELAYED_WORK(&slot->work, pciehp_queue_pushbutton_work);
ctrl->slot = slot;
return 0;
--
1.7.9.5
^ permalink raw reply related
* Re: [RFC PATCH 0/4] Allow Link state changes for Hot-Plug
From: Rajat Jain @ 2013-11-21 13:16 UTC (permalink / raw)
To: Linux-PCI, Linux-hotplug, linux-kernel
Cc: Bjorn Helgaas, Guenter Roeck, Kenji Kaneshige, Alex Williamson,
Yijing Wang, Paul Bolle, Eric W. Biederman, Rajat Jain,
Rajat Jain, Guenter Roeck
In-Reply-To: <528BE8B6.9080007@gmail.com>
Hello Bjorn / Folks,
Was wondering if you got a chance to look at this patchset?
Thanks & Best Regards,
Rajat Jain
On Tue, Nov 19, 2013 at 2:39 PM, Rajat Jain <rajatjain.linux@gmail.com> wrote:
>
> Hello,
>
> This patch set enables the use of PCI Express link up and link down events
> for Hotplug or Unplug. The requirement of such a feature was originally
> discussed here:
>
> http://www.spinics.net/lists/linux-pci/msg05783.html
> http://www.spinics.net/lists/hotplug/msg05801.html
>
> Patch [1/4]: makes a function non-static for use by patch 2.
> Patch [2/4]: Contains the bulk logic to allow link events to be used
> for hotplug and removal.
> Patch [3/4]: Makes the pciehp_power_thread() lock free by making it
> look at a work info->req instead of slot->state.
> Patch [4/4]: Introduce slot->hotplug_lock to serialize the hotplug
> operations.
>
> I'd appreciate if you could please review and provide me with any comments.
>
> Thanks,
>
> Rajat
>
> Rajat Jain (4):
> pciehp: Make check_link_active() non-static
> pciehp: Use link state change notifications for hot-plug and removal
> pciehp: Ensure all hotplug events are processed, even very fast ones.
> pciehp: Introduce hotplug_lock to serialize HP events on each slot
>
> drivers/pci/hotplug/pciehp.h | 6 ++
> drivers/pci/hotplug/pciehp_core.c | 10 +-
> drivers/pci/hotplug/pciehp_ctrl.c | 181 ++++++++++++++++++++++++++++++++++---
> drivers/pci/hotplug/pciehp_hpc.c | 63 +++++++++----
> 4 files changed, 229 insertions(+), 31 deletions(-)
>
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [RFC PATCH 0/4] Allow Link state changes for Hot-Plug
From: Bjorn Helgaas @ 2013-11-22 4:36 UTC (permalink / raw)
To: Rajat Jain
Cc: Linux-PCI, Linux-hotplug, linux-kernel@vger.kernel.org,
Guenter Roeck, Kenji Kaneshige, Alex Williamson, Yijing Wang,
Paul Bolle, Eric W. Biederman, Rajat Jain, Guenter Roeck
In-Reply-To: <CADTPrLS9iyvDkTLV5YyzJCiuGT2G0kTSbea8ZXzzT=E9RP71-Q@mail.gmail.com>
I haven't had a chance yet, sorry.
On Thu, Nov 21, 2013 at 6:16 AM, Rajat Jain <rajatjain.linux@gmail.com> wrote:
> Hello Bjorn / Folks,
>
> Was wondering if you got a chance to look at this patchset?
>
> Thanks & Best Regards,
>
> Rajat Jain
>
> On Tue, Nov 19, 2013 at 2:39 PM, Rajat Jain <rajatjain.linux@gmail.com> wrote:
>>
>> Hello,
>>
>> This patch set enables the use of PCI Express link up and link down events
>> for Hotplug or Unplug. The requirement of such a feature was originally
>> discussed here:
>>
>> http://www.spinics.net/lists/linux-pci/msg05783.html
>> http://www.spinics.net/lists/hotplug/msg05801.html
>>
>> Patch [1/4]: makes a function non-static for use by patch 2.
>> Patch [2/4]: Contains the bulk logic to allow link events to be used
>> for hotplug and removal.
>> Patch [3/4]: Makes the pciehp_power_thread() lock free by making it
>> look at a work info->req instead of slot->state.
>> Patch [4/4]: Introduce slot->hotplug_lock to serialize the hotplug
>> operations.
>>
>> I'd appreciate if you could please review and provide me with any comments.
>>
>> Thanks,
>>
>> Rajat
>>
>> Rajat Jain (4):
>> pciehp: Make check_link_active() non-static
>> pciehp: Use link state change notifications for hot-plug and removal
>> pciehp: Ensure all hotplug events are processed, even very fast ones.
>> pciehp: Introduce hotplug_lock to serialize HP events on each slot
>>
>> drivers/pci/hotplug/pciehp.h | 6 ++
>> drivers/pci/hotplug/pciehp_core.c | 10 +-
>> drivers/pci/hotplug/pciehp_ctrl.c | 181 ++++++++++++++++++++++++++++++++++---
>> drivers/pci/hotplug/pciehp_hpc.c | 63 +++++++++----
>> 4 files changed, 229 insertions(+), 31 deletions(-)
>>
>> --
>> 1.7.9.5
>>
^ permalink raw reply
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Bjorn Helgaas @ 2013-11-23 0:51 UTC (permalink / raw)
To: Rajat Jain
Cc: linux-pci@vger.kernel.org, linux hotplug mailing, Kenji Kaneshige,
Yijing Wang, Greg KH, Tom Nguyen, Kristen Accardi, Rajat Jain,
Rajat Jain, Guenter Roeck
In-Reply-To: <CADTPrLRJD0er1kijjm2SEE6wUA9eQaz+OOd67E3MpHLu6k-R6Q@mail.gmail.com>
On Mon, Nov 11, 2013 at 01:26:06PM -0800, Rajat Jain wrote:
> Hello,
>
> >> With my patch, we'd eliminate the 1 second delay and the second line
> >> of output above. If we also want to get rid of the first line
> >> "Unexpected CMD_COMPLETED..." also, that would probably have to be
> >> solved by changing the behavior to assume that CC shall be generated
> >> for every SLOTCTRL register write.
> >
> > I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint.
> > We shouldn't complain about hardware that is working perfectly fine.
> > I don't know the best way to do that yet. I have found a box with the
> > same hardware that was fixed by 5808639bfa98, so I hope to play with
> > it and figure out something that will work nicely for both scenarios.
>
> Please keep posted :-)
>
> >
> > BTW, can you open a report at bugzilla.kernel.org and attach your
> > "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd
> > like to have the details archived somewhere.
>
> Done:
>
> https://bugzilla.kernel.org/show_bug.cgi?idd821
>
> Please let me know if you need any help in trying something out - I'd
> be more than keen to help - write code or test.
What do you think of the patch below? I'm afraid we'll trip over
a few other old parts similar to the 82801H, but I'd rather do that
than penalize the parts that work correctly.
PCI: pciehp: Support command completion even with no hotplug hardware
From: Bjorn Helgaas <bhelgaas@google.com>
Commit 5808639bfa98 ("pciehp: fix slow probing") fixed a slow probing
problem on hardware that doesn't conform to the spec, but caused a
similar problem for hardware that *does* conform to the spec.
Per PCIe 3.0, sec 7.8.10 and 6.7.3.2, any write to Slot Control generates a
hot-plug command. Ports that can accept new commands with no delay can set
the "No Command Completed Support" bit. Otherwise the port must indicate
its completion of the command and readiness to accept a new command with a
"command completed event."
5808639bfa98 assumes ports that lack a power controller, power indicator,
attention indicator, and interlock will not generate completion events,
even if they neglect to set "No Command Completed Support." But on ports
that lack those elements and *do* support command completion notification,
it causes:
Unexpected CMD_COMPLETED. Need to wait for command completed event.
Command not completed in 1000 msec
and forces us to wait for a 1 second timeout.
This patch makes the 5808639bfa98 workaround into a quirk that's applied
only to devices known to be broken, currently just Intel 82801H ports.
There are probably other similarly-broken devices that may now probe
slowly, but I don't know how to catch them all without penalizing the
ones that play by the rules.
Link: https://bugzilla.kernel.org/show_bug.cgi?idd821
Reported-by: Rajat Jain <rajatjain@juniper.net>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/hotplug/pciehp_hpc.c | 39 +++++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 3eea3fdd4b0b..2fd2bd59e07f 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -881,6 +881,34 @@ static inline void dbg_ctrl(struct controller *ctrl)
ctrl_info(ctrl, "Slot Control : 0x%04x\n", reg16);
}
+static int pciehp_no_command_complete(struct controller *ctrl)
+{
+ struct pcie_device *dev = ctrl->pcie;
+ struct pci_dev *pdev = dev->port;
+ u16 vendor, device;
+
+ if (NO_CMD_CMPL(ctrl))
+ return 1;
+
+ /*
+ * Controller should notify on command completion unless the "No
+ * Command Completed Support" bit is set. But some hardware does
+ * not. See https://bugzilla.kernel.org/show_bug.cgi?id\x10751
+ */
+ if (!(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl))) {
+ vendor = pdev->vendor;
+ device = pdev->device;
+ if (vendor = PCI_VENDOR_ID_INTEL &&
+ device >= 0x283f && device <= 0x2849) {
+ dev_info(&dev->device, "device [%04x:%04x] does not notify on hotplug command completion\n",
+ vendor, device);
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
struct controller *pcie_init(struct pcie_device *dev)
{
struct controller *ctrl;
@@ -902,15 +930,8 @@ struct controller *pcie_init(struct pcie_device *dev)
mutex_init(&ctrl->ctrl_lock);
init_waitqueue_head(&ctrl->queue);
dbg_ctrl(ctrl);
- /*
- * Controller doesn't notify of command completion if the "No
- * Command Completed Support" bit is set in Slot Capability
- * register or the controller supports none of power
- * controller, attention led, power led and EMI.
- */
- if (NO_CMD_CMPL(ctrl) ||
- !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
- ctrl->no_cmd_complete = 1;
+
+ ctrl->no_cmd_complete = pciehp_no_command_complete(ctrl);
/* Check if Data Link Layer Link Active Reporting is implemented */
if (pciehp_readl(ctrl, PCI_EXP_LNKCAP, &link_cap)) {
^ permalink raw reply related
* Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Guenter Roeck @ 2013-11-23 1:59 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Rajat Jain, linux-pci@vger.kernel.org, linux hotplug mailing,
Kenji Kaneshige, Yijing Wang, Greg KH, Tom Nguyen,
Kristen Accardi, Rajat Jain, Rajat Jain, Guenter Roeck
In-Reply-To: <20131123005102.GA14690@google.com>
On Fri, Nov 22, 2013 at 05:51:02PM -0700, Bjorn Helgaas wrote:
> On Mon, Nov 11, 2013 at 01:26:06PM -0800, Rajat Jain wrote:
> > Hello,
> >
> > >> With my patch, we'd eliminate the 1 second delay and the second line
> > >> of output above. If we also want to get rid of the first line
> > >> "Unexpected CMD_COMPLETED..." also, that would probably have to be
> > >> solved by changing the behavior to assume that CC shall be generated
> > >> for every SLOTCTRL register write.
> > >
> > > I *do* want to get rid of the "Unexpected CMD_COMPLETED" complaint.
> > > We shouldn't complain about hardware that is working perfectly fine.
> > > I don't know the best way to do that yet. I have found a box with the
> > > same hardware that was fixed by 5808639bfa98, so I hope to play with
> > > it and figure out something that will work nicely for both scenarios.
> >
> > Please keep posted :-)
> >
> > >
> > > BTW, can you open a report at bugzilla.kernel.org and attach your
> > > "lspci -vvxxx" and dmesg output? When we eventually merge a fix, I'd
> > > like to have the details archived somewhere.
> >
> > Done:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?idd821
> >
> > Please let me know if you need any help in trying something out - I'd
> > be more than keen to help - write code or test.
>
> What do you think of the patch below? I'm afraid we'll trip over
> a few other old parts similar to the 82801H, but I'd rather do that
> than penalize the parts that work correctly.
>
Works nicely as far as I can see. Tested on P2020 and P5040 based systems
with IDT 89HPES48H12G2.
Tested-by: Guenter Roeck <groeck@juniper.net>
Guenter
>
> PCI: pciehp: Support command completion even with no hotplug hardware
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Commit 5808639bfa98 ("pciehp: fix slow probing") fixed a slow probing
> problem on hardware that doesn't conform to the spec, but caused a
> similar problem for hardware that *does* conform to the spec.
>
> Per PCIe 3.0, sec 7.8.10 and 6.7.3.2, any write to Slot Control generates a
> hot-plug command. Ports that can accept new commands with no delay can set
> the "No Command Completed Support" bit. Otherwise the port must indicate
> its completion of the command and readiness to accept a new command with a
> "command completed event."
>
> 5808639bfa98 assumes ports that lack a power controller, power indicator,
> attention indicator, and interlock will not generate completion events,
> even if they neglect to set "No Command Completed Support." But on ports
> that lack those elements and *do* support command completion notification,
> it causes:
>
> Unexpected CMD_COMPLETED. Need to wait for command completed event.
> Command not completed in 1000 msec
>
> and forces us to wait for a 1 second timeout.
>
> This patch makes the 5808639bfa98 workaround into a quirk that's applied
> only to devices known to be broken, currently just Intel 82801H ports.
> There are probably other similarly-broken devices that may now probe
> slowly, but I don't know how to catch them all without penalizing the
> ones that play by the rules.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?idd821
> Reported-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 39 +++++++++++++++++++++++++++++---------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 3eea3fdd4b0b..2fd2bd59e07f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -881,6 +881,34 @@ static inline void dbg_ctrl(struct controller *ctrl)
> ctrl_info(ctrl, "Slot Control : 0x%04x\n", reg16);
> }
>
> +static int pciehp_no_command_complete(struct controller *ctrl)
> +{
> + struct pcie_device *dev = ctrl->pcie;
> + struct pci_dev *pdev = dev->port;
> + u16 vendor, device;
> +
> + if (NO_CMD_CMPL(ctrl))
> + return 1;
> +
> + /*
> + * Controller should notify on command completion unless the "No
> + * Command Completed Support" bit is set. But some hardware does
> + * not. See https://bugzilla.kernel.org/show_bug.cgi?id\x10751
> + */
> + if (!(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl))) {
> + vendor = pdev->vendor;
> + device = pdev->device;
> + if (vendor = PCI_VENDOR_ID_INTEL &&
> + device >= 0x283f && device <= 0x2849) {
> + dev_info(&dev->device, "device [%04x:%04x] does not notify on hotplug command completion\n",
> + vendor, device);
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> struct controller *pcie_init(struct pcie_device *dev)
> {
> struct controller *ctrl;
> @@ -902,15 +930,8 @@ struct controller *pcie_init(struct pcie_device *dev)
> mutex_init(&ctrl->ctrl_lock);
> init_waitqueue_head(&ctrl->queue);
> dbg_ctrl(ctrl);
> - /*
> - * Controller doesn't notify of command completion if the "No
> - * Command Completed Support" bit is set in Slot Capability
> - * register or the controller supports none of power
> - * controller, attention led, power led and EMI.
> - */
> - if (NO_CMD_CMPL(ctrl) ||
> - !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) | EMI(ctrl)))
> - ctrl->no_cmd_complete = 1;
> +
> + ctrl->no_cmd_complete = pciehp_no_command_complete(ctrl);
>
> /* Check if Data Link Layer Link Active Reporting is implemented */
> if (pciehp_readl(ctrl, PCI_EXP_LNKCAP, &link_cap)) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply
* RE: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.
From: Rajat Jain @ 2013-11-23 14:56 UTC (permalink / raw)
To: Bjorn Helgaas, Rajat Jain
Cc: linux-pci@vger.kernel.org, linux hotplug mailing, Kenji Kaneshige,
Yijing Wang, Greg KH, Tom Nguyen, Kristen Accardi, Rajat Jain,
Guenter Roeck
In-Reply-To: <20131123005102.GA14690@google.com>
Hello Bjorn,
> >
> > https://bugzilla.kernel.org/show_bug.cgi?idd821
> >
> > Please let me know if you need any help in trying something out - I'd
> > be more than keen to help - write code or test.
>
> What do you think of the patch below? I'm afraid we'll trip over a few
> other old parts similar to the 82801H, but I'd rather do that than
> penalize the parts that work correctly.
Tested and works fine. Looks good (on a side note, we are introducing device specific check in a common code, but I do not think there is any other cleaner way). I think this patch should be applied as it solves the problem of slow probing.
On a different note, I feel there is still a need to apply my original patch. There is still an open problem in case of spurious interrupts (or in any case where the condition "if (slot_status & PCI_EXP_SLTSTA_CC)" becomes true in pcie_write_cmd()). That is because once that happens, we never clear that interrupt, and no further hotplug interrupts shall be received unless we do that.
Thanks,
Rajat
>
>
> PCI: pciehp: Support command completion even with no hotplug hardware
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Commit 5808639bfa98 ("pciehp: fix slow probing") fixed a slow probing
> problem on hardware that doesn't conform to the spec, but caused a
> similar problem for hardware that *does* conform to the spec.
>
> Per PCIe 3.0, sec 7.8.10 and 6.7.3.2, any write to Slot Control
> generates a hot-plug command. Ports that can accept new commands with
> no delay can set the "No Command Completed Support" bit. Otherwise the
> port must indicate its completion of the command and readiness to accept
> a new command with a "command completed event."
>
> 5808639bfa98 assumes ports that lack a power controller, power
> indicator, attention indicator, and interlock will not generate
> completion events, even if they neglect to set "No Command Completed
> Support." But on ports that lack those elements and *do* support command
> completion notification, it causes:
>
> Unexpected CMD_COMPLETED. Need to wait for command completed event.
> Command not completed in 1000 msec
>
> and forces us to wait for a 1 second timeout.
>
> This patch makes the 5808639bfa98 workaround into a quirk that's applied
> only to devices known to be broken, currently just Intel 82801H ports.
> There are probably other similarly-broken devices that may now probe
> slowly, but I don't know how to catch them all without penalizing the
> ones that play by the rules.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?idd821
> Reported-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/hotplug/pciehp_hpc.c | 39 +++++++++++++++++++++++++++++--
> -------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> b/drivers/pci/hotplug/pciehp_hpc.c
> index 3eea3fdd4b0b..2fd2bd59e07f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -881,6 +881,34 @@ static inline void dbg_ctrl(struct controller
> *ctrl)
> ctrl_info(ctrl, "Slot Control : 0x%04x\n", reg16);
> }
>
> +static int pciehp_no_command_complete(struct controller *ctrl) {
> + struct pcie_device *dev = ctrl->pcie;
> + struct pci_dev *pdev = dev->port;
> + u16 vendor, device;
> +
> + if (NO_CMD_CMPL(ctrl))
> + return 1;
> +
> + /*
> + * Controller should notify on command completion unless the "No
> + * Command Completed Support" bit is set. But some hardware does
> + * not. See https://bugzilla.kernel.org/show_bug.cgi?id\x10751
> + */
> + if (!(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) |
> EMI(ctrl))) {
> + vendor = pdev->vendor;
> + device = pdev->device;
> + if (vendor = PCI_VENDOR_ID_INTEL &&
> + device >= 0x283f && device <= 0x2849) {
> + dev_info(&dev->device, "device [%04x:%04x] does not
> notify on hotplug command completion\n",
> + vendor, device);
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> struct controller *pcie_init(struct pcie_device *dev) {
> struct controller *ctrl;
> @@ -902,15 +930,8 @@ struct controller *pcie_init(struct pcie_device
> *dev)
> mutex_init(&ctrl->ctrl_lock);
> init_waitqueue_head(&ctrl->queue);
> dbg_ctrl(ctrl);
> - /*
> - * Controller doesn't notify of command completion if the "No
> - * Command Completed Support" bit is set in Slot Capability
> - * register or the controller supports none of power
> - * controller, attention led, power led and EMI.
> - */
> - if (NO_CMD_CMPL(ctrl) ||
> - !(POWER_CTRL(ctrl) | ATTN_LED(ctrl) | PWR_LED(ctrl) |
> EMI(ctrl)))
> - ctrl->no_cmd_complete = 1;
> +
> + ctrl->no_cmd_complete = pciehp_no_command_complete(ctrl);
>
> /* Check if Data Link Layer Link Active Reporting is
> implemented */
> if (pciehp_readl(ctrl, PCI_EXP_LNKCAP, &link_cap)) {
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox