Re: [PATCH v4] pciehp: only wait command complete for real hotplug control

[Bjorn Helgaas <bhelgaas@google.com>]

On Fri, May 30, 2014 at 03:28:16PM -0700, Yinghai Lu wrote:
> On Tue, May 20, 2014 at 1:53 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Thu, May 01, 2014 at 10:40:55AM -0700, Yinghai Lu wrote:
> 
> >> With that we could save 16 seconds during booting, later with 32 sockets system
> >> with 64 pcie hotplug slots we could save 64 seconds.
> >
> > Doesn't this just move a timeout out of the boot path and into a future
> > hotplug event?
> 
> Both. But not affect real Hotplug events (EIC, PCC, PIC, AIC).
> 
> >
> > These controllers do not set the "No Command Completed Support" bit, so
> > pciehp expects that they will generate Command Completed events after they
> > process commands.
> >
> > During initialization, pcie_enable_notification() writes a command to set
> > the DLLSCE, ABPE, PDCE, MRLSCE, HPIE, CCIE bits.  The (EIC, PCC, PIC, AIC)
> > bits aren't changed, so per CF118, the controller will not set the Command
> > Completed bit.
> >
> > Later, when we turn on power to the slot, ctrl->cmd_busy will still be set,
> > so won't we timeout waiting for the Command Completed bit?
> 
> If it is real hotplug events, cmd_busy will be cleared by pcie_isr.
> 
> If it is not real hotplug events, cmd_busy is still set, and but CC is not set.

The first command is not a "real" hotplug event (it doesn't change (EIC,
PCC, PIC, AIC)).  So these broken controllers won't set CC.  The second
command probably *will* be a real hotplug event.  But we have to wait for
CC to be set before we can issue it, and on these broken controllers, that
wait will time out.

> later before we try to write cmd again, but only cmd_busy is set &&
> CC is not set, that means previous write cmd is not real hotplug event.
> so we should not wait.

It means no such thing.  If cmd_busy is set and CC is not set, that means
we've written a command to SLTCTL, but it hasn't completed yet, and we
can't issue another command until either CC is set or a timeout period has
elapsed.

> > (I think there's a bug in your patch (see below) that would explain why you
> > wouldn't see this timeout in testing.)
> >
> >>       /*
> >>        * Wait for command completion.
> >>        */
> >> -     if (!ctrl->no_cmd_complete) {
> >> +     if (!ctrl->no_cmd_complete && (slot_status & PCI_EXP_SLTSTA_CC) &&
> >> +         ctrl->cmd_busy) {
> >
> > I don't understand this change.  Why do you test "slot_status &
> > PCI_EXP_SLTSTA_CC" here?  Did you mean to write this:
> >
> >   if (!ctrl->no_cmd_complete && !(slot_status & PCI_EXP_SLTSTA_CC) && ...
> >
> > instead?  I think we want to wait if (1) the controller generates Command
> > Complete events and (2) Slot Status indicates that a command is still in
> > progress.
> 
> Yes. your logic is right for those real hotplug event, that means
> CC is not set, and isr is not called to clear cmd_busy.
> 
> but it will wait for real or non-real cmd right after non-real hotplug cmd.
> 
> looks like we need to start one timer for non-real hotplug to clear cmd_busy?

I think your current patch is incorrect because you wait if the previous
command has completed, and you write a new command if the previous one is
still in progress.  That's backwards.

The code has to work correctly for spec-compliant hardware.  If we have to
wait for timeouts on hardware with errata, that's unfortunate, but at least
things will still work correctly.

If you can make it work correctly with no timeouts on both working and
broken hardware, that's obviously ideal.

Bjorn