Re: [PATCH] pciehp: Acknowledge the spurious "cmd completed" event.

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 <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> 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
>