From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com ([192.55.52.115]:52791 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752607AbdC0Ot2 (ORCPT ); Mon, 27 Mar 2017 10:49:28 -0400 From: Felipe Balbi To: Mathias Nyman , gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org, Mathias Nyman , stable@vger.kernel.org Subject: Re: [PATCH v2 2/3] xhci: Set URB actual length for stopped control transfers In-Reply-To: <1490624830-9761-3-git-send-email-mathias.nyman@linux.intel.com> References: <1490624830-9761-1-git-send-email-mathias.nyman@linux.intel.com> <1490624830-9761-3-git-send-email-mathias.nyman@linux.intel.com> Date: Mon, 27 Mar 2017 17:47:58 +0300 Message-ID: <877f3akc81.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: stable-owner@vger.kernel.org List-ID: Hi, Mathias Nyman writes: > A control transfer that stopped at the status stage incorrectly > warned about a "unexpected TRB Type 4", and did not set the > transferred actual_length for the URB. > > The transferred actual_length should be set the same way for > COMP_STOPPED control transfers as in the generic cases. > > generic case if we get an event at: > > TRB_SETUP stage: > length = 0; > > TRB_DATA/TRB_NORMAL state: > length = requested - remaining; > > TRB_STATUS stage: > length = requested > > The URB actual_length for control transfers doesn't care about sent > bytes of the SETUP stage, or remaining bytes of the STATUS stage. > > Cc: > Signed-off-by: Mathias Nyman > --- > drivers/usb/host/xhci-ring.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index d9936c7..584b6fe 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1975,25 +1975,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td, > *status = 0; > break; > case COMP_STOPPED_SHORT_PACKET: > - if (trb_type == TRB_DATA || trb_type == TRB_NORMAL) > + if (trb_type == TRB_DATA || trb_type == TRB_NORMAL) { > + td->urb_length_set = true; > td->urb->actual_length = remaining; > - else > + } else { > xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl setup or status TRB\n"); > + } > goto finish_td; > case COMP_STOPPED: > - switch (trb_type) { > - case TRB_SETUP: > - td->urb->actual_length = 0; > - goto finish_td; > - case TRB_DATA: > - case TRB_NORMAL: > - td->urb->actual_length = requested - remaining; > - goto finish_td; > - default: > - xhci_warn(xhci, "WARN: unexpected TRB Type %d\n", > - trb_type); > - goto finish_td; > - } > + break; instead of this giant patch, why didn't you just add case TRB_STATUS to the switch statement above? It would've been a single line that would solve your problem. Not to mention that we loose the xhci_warn() for unexpected TRB types which might help finding valid issues (such as $subject). Note that now we do nothing on COMP_STOPPED, this means that we're relying on some generic case that, arguably, could be removed altogether. $subject is just re-adding part of obfuscation we had and took so much work to clean up. How have you tested $subject, btw? -- balbi