From patchwork Wed Nov 12 18:51:13 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Felipe Balbi X-Patchwork-Id: 40689 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wg0-f70.google.com (mail-wg0-f70.google.com [74.125.82.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id D8AB524493 for ; Wed, 12 Nov 2014 18:51:08 +0000 (UTC) Received: by mail-wg0-f70.google.com with SMTP id x13sf6965821wgg.9 for ; Wed, 12 Nov 2014 10:51:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:from:to:cc:subject:date:message-id :mime-version:sender:precedence:list-id:x-original-sender :x-original-authentication-results:mailing-list:list-post:list-help :list-archive:list-unsubscribe:content-type; bh=krBhif7OCT0I0tPXLaNRFUpMJalp6xS039Cm7jYMhwE=; b=gXFCvH0XzfrU7sPf1+Pg8RTrqCDmNuhC/gFrIXXpOxL8y8h5FTPnU9tmGVefcROh2p xvH6OyzRts03vb/AXub6oyfss+fsg5XqHfwp5VTIQvxwwCUYNQ4sBakDJxM5l+SP2XYw ekWaWEXQcvJikwWbu+v4CJSWy8sfHinsf357WSHWLeWLSbAc2i3mthNH/dF+KWfwsVqj yi7JfowxEi9ow9f4QZGrhcSqW6QQjYEA7SJpyWi2vcY80RWBurUiiE7sycVOHqfoH3aE EjRw9TVqxt6h9jI/7Ya8tzZ/CP7s2Wqsxn+bAeekbpyKhNyDyC8jzuJDs2WNz67emT31 ahqA== X-Gm-Message-State: ALoCoQkIA2xm9dAnM6p1QuNwkKdvaibPgAsKtdllwRU9XeDAVmgrElx4D4ATTfaITq/+DMNrj0dF X-Received: by 10.180.101.170 with SMTP id fh10mr6953777wib.4.1415818268104; Wed, 12 Nov 2014 10:51:08 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.161.203 with SMTP id xu11ls708418lab.90.gmail; Wed, 12 Nov 2014 10:51:07 -0800 (PST) X-Received: by 10.152.42.226 with SMTP id r2mr4018884lal.29.1415818267842; Wed, 12 Nov 2014 10:51:07 -0800 (PST) Received: from mail-la0-f43.google.com (mail-la0-f43.google.com. [209.85.215.43]) by mx.google.com with ESMTPS id ke10si35987077lbc.41.2014.11.12.10.51.07 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 12 Nov 2014 10:51:07 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.43 as permitted sender) client-ip=209.85.215.43; Received: by mail-la0-f43.google.com with SMTP id ge10so12027912lab.16 for ; Wed, 12 Nov 2014 10:51:07 -0800 (PST) X-Received: by 10.152.6.228 with SMTP id e4mr43856414laa.71.1415818267543; Wed, 12 Nov 2014 10:51:07 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.184.201 with SMTP id ew9csp473832lbc; Wed, 12 Nov 2014 10:51:06 -0800 (PST) X-Received: by 10.68.224.227 with SMTP id rf3mr49834023pbc.61.1415818265777; Wed, 12 Nov 2014 10:51:05 -0800 (PST) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d17si23444255pdl.249.2014.11.12.10.51.05 for ; Wed, 12 Nov 2014 10:51:05 -0800 (PST) Received-SPF: none (google.com: linux-usb-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753374AbaKLSu7 (ORCPT + 3 others); Wed, 12 Nov 2014 13:50:59 -0500 Received: from devils.ext.ti.com ([198.47.26.153]:60173 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753100AbaKLSu4 (ORCPT ); Wed, 12 Nov 2014 13:50:56 -0500 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by devils.ext.ti.com (8.13.7/8.13.7) with ESMTP id sACIomaj012601; Wed, 12 Nov 2014 12:50:48 -0600 Received: from DFLE73.ent.ti.com (dfle73.ent.ti.com [128.247.5.110]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id sACIom05011263; Wed, 12 Nov 2014 12:50:48 -0600 Received: from dflp32.itg.ti.com (10.64.6.15) by DFLE73.ent.ti.com (128.247.5.110) with Microsoft SMTP Server id 14.3.174.1; Wed, 12 Nov 2014 12:50:47 -0600 Received: from localhost (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp32.itg.ti.com (8.14.3/8.13.8) with ESMTP id sACIolPV024102; Wed, 12 Nov 2014 12:50:47 -0600 From: Felipe Balbi To: Mathias Nyman CC: Linux USB Mailing List , Alan Stern , Greg KH , Felipe Balbi , Subject: [RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td() Date: Wed, 12 Nov 2014 12:51:13 -0600 Message-ID: <1415818273-22610-1-git-send-email-balbi@ti.com> X-Mailer: git-send-email 2.1.0.GIT MIME-Version: 1.0 Sender: linux-usb-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-usb@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: balbi@ti.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.43 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , If class driver wants to SetFeature(ENDPOINT_HALT) and later tries to talk to the stalled endpoint, xhci will move endpoint to EP_STATE_STALL and subsequent usb_submit_urb() will not cause a USB token to be shifted into the data lines. Because of that, peripheral will never have any means of STALLing a follow up token. This is a known error at least with g_zero + testusb -t 13 and can be easily reproduced. Cc: # v3.14+ Signed-off-by: Felipe Balbi --- this is a second version of patch sent originally in January this year [1]. I suppose this is still not the best fix (as noted on code comment below), but it'll serve to restart the thread on what's the best way to fix this. I thought about clearing endpoint halt if !control from prepare_ring(), but I'm not entirely of the implications there either. I'm adding stable to Cc for v3.14+ (which is the earlier kernel I could easily run to reproduce this) but it's likely a much older bug. Still, if nobody other than me complained until now, perhaps we don't care as much ? [1] http://marc.info/?l=linux-usb&m=139025060301432&w=2 drivers/usb/host/xhci-ring.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index bc6fcbc..4e8c3cf 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1826,13 +1826,45 @@ static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td, if (trb_comp_code == COMP_STALL) { /* The transfer is completed from the driver's * perspective, but we need to issue a set dequeue - * command for this stalled endpoint to move the dequeue - * pointer past the TD. We can't do that here because - * the halt condition must be cleared first. Let the - * USB class driver clear the stall later. + * command for this stalled endpoint to move the + * dequeue pointer past the TD. + * + * Before we can move the dequeue pointer past the TD, + * we must first clear the halt condition, however, we + * can't clear such condition for control endpoints + * since that can only be cleared through a USB Reset. + * + * Note that this isn't 100% safe because as soon as a + * stalled TD is completed, we will clear the halt + * condition on the endpoint from the host side, which + * might not always be what we want. + * + * Perhaps a better approach would be to differentiate + * SetFeature(ENDPOINT_HALT) from a STALL due to error + * or Babble and only clear halt in that case. + * + * Another difficulty with this case is that if class + * driver wants to talk to a halted endpoint to verify + * SetFeature(ENDPOINT_HALT) was implemented correctly + * by firmware (one such case is g_zero + testusb -t + * 13), without clearing HALT on the host side, EP + * doorbell will be rung but endpoint will not exit + * EP_STATE_HALTED, because we *must* first issue a + * Reset Endpoint command to move the EP to the + * EP_STATE_STOPPED before EP doorbell works again. + * + * So, we will issue 'Reset Endpoint' here to make sure a + * subsequent usb_submit_urb() will actually cause EP + * doorbell to ring so a USB token is shifted into the + * wire and peripheral has a chance of replying with + * STALL again. */ ep->stopped_td = td; ep->stopped_stream = ep_ring->stream_id; + if (!usb_endpoint_xfer_control(&td->urb->ep->desc)) + xhci_cleanup_halted_endpoint(xhci, + slot_id, ep_index, ep_ring->stream_id, + td, event_trb); } else if (xhci_requires_manual_halt_cleanup(xhci, ep_ctx, trb_comp_code)) { /* Other types of errors halt the endpoint, but the