Patchwork pciehp: Acknowledge the spurious "cmd completed" event.

login
register
mail settings
Submitter Rajat Jain
Date Nov. 5, 2013, 10:33 p.m.
Message ID <52797238.8070304@gmail.com>
Download mbox | patch
Permalink /patch/288668/
State Rejected
Headers show

Comments

Rajat Jain - Nov. 5, 2013, 10:33 p.m.
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(+)
Bjorn Helgaas - Nov. 6, 2013, 12:25 a.m.
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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajat Jain - Nov. 6, 2013, 2:38 a.m.
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=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Bus: primary=02, secondary=50, subordinate=5f, 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=fast >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=100ns PATEntryBits=4
        Arb:    Fixed- WRR32- WRR64- WRR128-
        Ctrl:   ArbSelect=Fixed
        Status: InProgress-
        VC0:    Caps:   PATOffset=04 MaxTimeSlots=1 RejSnoopTrans-
                Arb:    Fixed+ WRR32- WRR64- WRR128- TWRR128- WRR256-
                Ctrl:   Enable+ ID=0 ArbSelect=Fixed TC/VC=ff
                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=0000: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
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajat Jain - Nov. 7, 2013, 9:53 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Nov. 8, 2013, 1:23 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajat Jain - Nov. 8, 2013, 5:30 p.m.
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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Nov. 8, 2013, 11:15 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rajat Jain - Nov. 11, 2013, 9:26 p.m.
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?id=64821

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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