Mathias Nyman <mathias.ny...@linux.intel.com> 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.
[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).
Thanks.
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
