Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command

[Mathias Nyman]

On 21.12.2016 16:10, OGAWA Hirofumi wrote:
> Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> writes:
>
> > > Below is the latest code. I put my comments in line.
> > >
> > >    322 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci)
> > >    323 {
> > >    324         u64 temp_64;
> > >    325         int ret;
> > >    326
> > >    327         xhci_dbg(xhci, "Abort command ring\n");
> > >    328
> > >    329         reinit_completion(&xhci->cmd_ring_stop_completion);
> > >    330
> > >    331         temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
> > >    332         xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
> > >    333                         &xhci->op_regs->cmd_ring);
> > >
> > > We should hold xhci->lock when we are modifying xhci registers
> > > at runtime.
> >
> > Makes sense, but we need to unlock it before sleeping or waiting for
> > completion.  I need to look into that in more detail.
> >
> > But this was an issue already before these changes.
>
> We set CMD_RING_STATE_ABORTED state under locking. I'm not checking what
> is for taking lock for register though, I guess it should be enough just
> lock around of read=>write of ->cmd_ring if need lock.

After your patch it should be enough to have the lock only while
reading and writing the cmd_ring register.

If we want a locking fix that applies more easily to older stable releases before
your change then the lock needs to cover set CMD_RING_STATE_ABORT, read cmd_reg,
write cmd_reg and busiloop checking CRR bit.  Otherwise the stop cmd ring interrupt
handler may restart the ring just before we start checing CRR. The stop cmd ring
interrupt will set the CMD_RING_STATE_ABORTED to CMD_RING_STATE_RUNNING so
ring will really restart in the interrupt handler.

> [Rather ->cmd_ring user should check CMD_RING_STATE_ABORTED state.]
>
> > But then again I really like OGAWA Hiroumi's solution that separates the
> > command ring stopping from aborting commands and restarting the ring.
> >
> > The current way of always restarting the command ring as a response to
> > a stop command ring command really limits its usage.
> >
> > So, with this in mind most reasonable would be to
> > 1. fix the lock to cover abort+CRR check, and send it to usb-linus +stable
> > 2. rebase OGAWA Hirofumi's changes on top of that, and send to usb-linus only
> > 3. remove unnecessary second abort try as a separate patch, send to usb-next
> > 4. remove polling for the Command ring running (CRR), waiting for completion
> >      is enough, if completion times out then we can check CRR. for usb-next
>
> I think we should check both of CRR and even of stop completion. Because
> CRR and stop completion was not same time (both can be first).

We can keep both, maybe change the order and do the busylooping-checking after
waiting for completion, but thats a optimization for usb-next sometimes later.

Thanks

-Mathias