From patchwork Thu Oct 31 09:43:02 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Micha=C5=82_Pecio?= X-Patchwork-Id: 13857760 Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7249A13BAC3 for ; Thu, 31 Oct 2024 09:43:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.44 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730367790; cv=none; b=Ial/1PtrsaU6Bq3PCJ+6zT9CiWzViR/8RTIxpykbnyVPyYbc9x6GbZTpucvQ1o4roEcV6JKFXrJrohgHnOHrC4KIHfNiBT+FRYRzQOiJgTCHALOwclB3VxyIbStQLIo7/itYR6uYp0AW9yLfQifsyC7UsGFamgJxwtvdhvvwetY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730367790; c=relaxed/simple; bh=1pMRCbv89qdg0o11ljih3Oei+WaZwQDEVdijBxFNLqY=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=JfqYCULsgi+3CdqGeWY6FfrUYlmk7R5uHRtZlqodCg15/uO//R7GV8ghgXJoFmZblw6WVXXn3pAC0EVYqD7WPBYFDZyLjw4Hhba2ayV5U689Iz5Vx6C42oPpBbuTfSsAbR/NnPP81qcU+KC5ylSeyafx1KMESumsNlCHMy63l54= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=PR9IqjYM; arc=none smtp.client-ip=209.85.167.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="PR9IqjYM" Received: by mail-lf1-f44.google.com with SMTP id 2adb3069b0e04-539f4d8ef66so931355e87.1 for ; Thu, 31 Oct 2024 02:43:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730367786; x=1730972586; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=EZYvbcblMqiRk+LHTGLqF0u2yHlKbcksAPMq/ywRDBU=; b=PR9IqjYMQPdAFsu1oOXHurWTr2cySJkMlf4mxc0Bz6EbcCOJwh4b5wM2ALfpMDhM+d flV0rjLF4KtisTtZw2ma/o+3AUTLFavxI3fa5i3hR7OaEZf9++ptsiqQwPGpmkwdXvUg VVZK53Bq5R+byMZTlMfDC2yCe3HD8TLFsOeiT8d0PvRnTiqsvKBAHCGk1kjAzss1QGeK eLWWP2j1LuX0xzCY9n8PQAkcvbJ6KwuNk8D+pWpVRi/9/xgpme8w66ylqjMqeGZ0TqAe fRpIZpPpgZwKAZU/YTqACzYLBAxZUjUOowUQLoegzvg9p0/jL5haaFw80tVyghI1JW1D HTDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730367786; x=1730972586; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=EZYvbcblMqiRk+LHTGLqF0u2yHlKbcksAPMq/ywRDBU=; b=bpNs/RfZoZl0sIPbFtQFpWjfv5CO935AVn9gxJV7XeRyVADTjDeWP/2hRqBF+EpW8U /6/d7ibigMD2JtKm4shDJxbu5RYcoDISV8+mJG4zely4hqbpM2fCWzlEmQ9USPHVgVrv 9lAWxGIpJLs+OZH+vgD1xCCx+OYEORYCaNiegq1lA3JbHGhv/stvE5q5lRgGRLY8hCUh v2VRMEjoH0XupqeOrNU8lJbLGiVWJof+ZqMq7H2vvG7wkta3vBWCkl+Dl32jQ6+ep5+6 QxndTnZpChiuWuKD8V5dRNZEoz9I7ZyobZw3OuWZ2LDksqt8Mj8repWGR2sskLwXRVL7 4KoQ== X-Gm-Message-State: AOJu0YzeTw321da6OO74IXCxRNDzSO03MhQ2MKEL39XPSIuV+J8osq7M kizX88bkbW8qVFKn/BIKtiz2VvSlL3cpPKfRCUrR3G2BPcj0Y3GpDhhDyw== X-Google-Smtp-Source: AGHT+IHzYn0oMyQ7sTFjqlkrB7jEi5UWX1iQ8IXjyuCnz20ezOvs2QHQ9xb03Z7mO0rsTdvubjKOEA== X-Received: by 2002:a05:6512:1392:b0:536:73b5:d971 with SMTP id 2adb3069b0e04-53b34c5f886mr9722493e87.38.1730367786148; Thu, 31 Oct 2024 02:43:06 -0700 (PDT) Received: from foxbook (bfh123.neoplus.adsl.tpnet.pl. [83.28.45.123]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-53c7bde0ae2sm141369e87.260.2024.10.31.02.43.05 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 31 Oct 2024 02:43:05 -0700 (PDT) Date: Thu, 31 Oct 2024 10:43:02 +0100 From: Michal Pecio To: Mathias Nyman Cc: linux-usb@vger.kernel.org Subject: [PATCH v3 1/3] usb: xhci: Limit Stop Endpoint retries Message-ID: <20241031104302.6b7538cd@foxbook> In-Reply-To: <20241031104159.25f9ff70@foxbook> References: <20241031104159.25f9ff70@foxbook> Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Some host controllers fail to atomically transition an endpoint to the Running state on a doorbell ring and enter a hidden "Restarting" state, which looks very much like Stopped, with the important difference that it will spontaneously transition to Running anytime soon. A Stop Endpoint command queued in the Restarting state typically fails with Context State Error and the completion handler sees the Endpoint Context State as either still Stopped or already Running. Even a case of Halted was observed, when an error occurred right after the restart. The Halted state is already recovered from by resetting the endpoint. The Running state is handled by retrying Stop Endpoint. The Stopped state was recognized as a problem on NEC controllers and worked around also by retrying, because the endpoint soon restarts and then stops for good. But there is a risk: the command may fail if the endpoint is "stopped for good" already, and retries will fail forever. The possibility of this was not realized at the time, but a number of cases was discovered later and reproduced. Some proved difficult to deal with, and it is outright impossible to predict if an endpoint may fail to ever restart at all due to hardware bug. One bug of this kind (albeit not on NEC) was discovered and is reproducible from userspace: while true; do ifconfig eth1 up; ifconfig eth1 down; done An endless retries storm is quite nasty. Besides putting needless load on the xHC and CPU, it causes URBs never to be given back, paralyzing the device and connection/disconnection logic for the whole bus if the device is unplugged. User processes waiting for URBs become unkillable and xhci_hcd cannot be unbound from the HC or reloaded. For peace of mind, impose a timeout on Stop Endpoint retries in this case. If they don't succeed in 100ms, consider the endpoint stopped permanently for some reason and just give back the unlinked URBs. This failure case is rare already and work is under way to make it rarer. Start this work today by also handling one simple case of race with Reset Endpoint, because it costs practically nothing to implement. Fixes: fd9d55d190c0 ("xhci: retry Stop Endpoint on buggy NEC controllers") CC: stable@vger.kernel.org Signed-off-by: Michal Pecio --- drivers/usb/host/xhci-ring.c | 28 ++++++++++++++++++++++++---- drivers/usb/host/xhci.c | 2 ++ drivers/usb/host/xhci.h | 1 + 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index b6eb928e260f..98e12267bbf6 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -52,6 +52,7 @@ * endpoint rings; it generates events on the event ring for these. */ +#include #include #include #include @@ -1151,16 +1152,35 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id, return; case EP_STATE_STOPPED: /* - * NEC uPD720200 sometimes sets this state and fails with - * Context Error while continuing to process TRBs. - * Be conservative and trust EP_CTX_STATE on other chips. + * Per xHCI 4.6.9, Stop Endpoint command on a Stopped + * EP is a Context State Error, and EP stays Stopped. + * + * But maybe it failed on Halted, and somebody ran Reset + * Endpoint later. EP state is now Stopped and EP_HALTED + * still set because Reset EP handler will run after us. + */ + if (ep->ep_state & EP_HALTED) + break; + /* + * On some HCs EP state remains Stopped for some tens of + * us to a few ms or more after a doorbell ring, and any + * new Stop Endpoint fails without aborting the restart. + * This handler may run quickly enough to still see this + * Stopped state, but it will soon change to Running. + * + * Assume this bug on unexpected Stop Endpoint failures. + * Keep retrying until the EP starts and stops again, on + * chips where this is known to help. Wait for 100ms. */ if (!(xhci->quirks & XHCI_NEC_HOST)) break; + if (time_is_before_jiffies(ep->stop_time + msecs_to_jiffies(100))) + break; fallthrough; case EP_STATE_RUNNING: /* Race, HW handled stop ep cmd before ep was running */ - xhci_dbg(xhci, "Stop ep completion ctx error, ep is running\n"); + xhci_dbg(xhci, "Stop ep completion ctx error, ctx_state %d\n", + GET_EP_CTX_STATE(ep_ctx)); command = xhci_alloc_command(xhci, false, GFP_ATOMIC); if (!command) { diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 899c0effb5d3..1ed4dce7eeb7 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -8,6 +8,7 @@ * Some code borrowed from the Linux EHCI driver. */ +#include #include #include #include @@ -1777,6 +1778,7 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) ret = -ENOMEM; goto done; } + ep->stop_time = jiffies; ep->ep_state |= EP_STOP_CMD_PENDING; xhci_queue_stop_endpoint(xhci, command, urb->dev->slot_id, ep_index, 0); diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index f0fb696d5619..0bef6c8b51cf 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -690,6 +690,7 @@ struct xhci_virt_ep { /* Bandwidth checking storage */ struct xhci_bw_info bw_info; struct list_head bw_endpoint_list; + unsigned long stop_time; /* Isoch Frame ID checking storage */ int next_frame_id; /* Use new Isoch TRB layout needed for extended TBC support */ From patchwork Thu Oct 31 09:44:01 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Micha=C5=82_Pecio?= X-Patchwork-Id: 13857761 Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E4082194C8F for ; Thu, 31 Oct 2024 09:44:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.51 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730367850; cv=none; b=kvbOcQqqEbvKaWuy7t8TDBo4VD1MI8Ew8SxwmXtR669mOOODJQchOHz6d5TiUpBG3XYjR+Kx+ChW35Rum+YNyrgxpi38ebgcVayhq0N+3Uj86kwrUZqzuS1IahjXMqH92agMKbegJUlG1TUv9aPJFVPsWIKMo7Q3kDX7VJXEJhI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730367850; c=relaxed/simple; bh=nJqeqVFIORHTjM8jepMIV646vUUWTZkmi01il8W04F4=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=Qj83r1RsvF+cdhnTwu4cSHfrzWaGOBP9vuZ3AWXRzv3ldKWcwF05SvYezRlehTG/RwZuX4r4tvMJ9c0I+GI4gE4LI0akh8uE20EDSFMT3BZbScPiq2p55dg5DXpIjAsME7tu1/93uHs0SWjEA6xfj38Eiea1dZF5mpxTpVIX2vY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=PW4VWVIU; arc=none smtp.client-ip=209.85.167.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="PW4VWVIU" Received: by mail-lf1-f51.google.com with SMTP id 2adb3069b0e04-539f8490856so742415e87.2 for ; Thu, 31 Oct 2024 02:44:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730367846; x=1730972646; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=C4mVf7CRw66+FEOJmtEGI+h7+Iv2YLHTEZ7B64Dw5CA=; b=PW4VWVIUQMl6JnkZrX2wM8R/h6vPUTitca/9HUxMmZIirrsOTViRG82LAxPqSQIeEP FMtRsDcTmus7nwu2aPH7S17PXBAga/rq5AokSjsRDygDyGBJ78x5tpprkYAywURBSmVx KWOxe+O9uxDC28Trlrlmu/c2v3b8j/rM2eBKTFb1eBtr71HgN2yZF6P/7efoBlb9X8K+ uJAKqBZiXukPPA40iN/W9zeJEtwX3FBVTXD5KZG0sw/981V7wR2N8lAYMd0/H8aPFvMm zhePmldi1+uiNOGFU+X6Ksx+BWxNaaJMq7JUBi7YIkDdlF73bhL8zGthL9bF92wjnOzD xxJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730367846; x=1730972646; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=C4mVf7CRw66+FEOJmtEGI+h7+Iv2YLHTEZ7B64Dw5CA=; b=rVP7rxPK2alc0Fw9kBkgX+T1nR/pQCKQVAjMoEjSSKXELjhhjM+vUUbMd9eFjF3GjM N7VHOY3UbF41r17eoK8t2BKYxnIkTNPzdYorOj4Xhy2p+u9a1AJf0ivblTFf3sZb5y2F NsRYocUABDiwybqYOV8q5TNcQy1oH9EvyYEvtvSBeaGJKsLnnW6Jz+eJfiiNA0X8uXZx T18kjCNhoWJdzDYVrvYSZ9oxLKDAEEPlu0ZqfgsTYjDaXHKd6LguTIwSbYndtm2C5qJq bCgsMKv8JzBT4ezc0MyT16w1Tx2kLwwimdzvN16myZZLJYE5AZciE0rnKndLX+yfPgre xFYw== X-Gm-Message-State: AOJu0YxdFX7tvTTYMLYJOQ4U6UXYJblTec0EiVNwAopGAimOdixhll0F 0kjX9jWc5uu7QHnl6/rOuDXN2XWHhe0PH+/ZZnDiMt6zVPVGVHzESlJa6A== X-Google-Smtp-Source: AGHT+IE0XJLc7I+AAm4dsFFKI0RvPkNXqWSxmNKhpRi5MCk0mrzzujL6p0Uj7DPK/bJjisqYAMttPg== X-Received: by 2002:a05:6512:1302:b0:536:553f:3ef9 with SMTP id 2adb3069b0e04-53c79e4c5c4mr1281395e87.27.1730367845766; Thu, 31 Oct 2024 02:44:05 -0700 (PDT) Received: from foxbook (bfh123.neoplus.adsl.tpnet.pl. [83.28.45.123]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-53c7bde32fesm140530e87.282.2024.10.31.02.44.03 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 31 Oct 2024 02:44:04 -0700 (PDT) Date: Thu, 31 Oct 2024 10:44:01 +0100 From: Michal Pecio To: Mathias Nyman Cc: linux-usb@vger.kernel.org Subject: [PATCH v3 2/3] usb: xhci: Fix TD invalidation under pending Set TR Dequeue Message-ID: <20241031104401.22a80b14@foxbook> In-Reply-To: <20241031104159.25f9ff70@foxbook> References: <20241031104159.25f9ff70@foxbook> Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 xhci_invalidate_cancelled_tds() may work incorrectly if the hardware is modifying some Endpoint or Stream Context by executing a Set TR Dequeue command at the same time. And even if it worked, it couldn't queue another Set TR Dequeue if required, failing to clear xHC cache. Yet this function is called by completion handlers of commands like Stop Endpoint and Reset Endpoint without any testing for SET_DEQ_PENDING. The problem remained undetected for long time because the UAS driver rarely cancels URBs, but it does cancel them sometimes, e.g. on I/O errors. Make it legal to call xhci_invalidate_cancelled_tds() under a pending Set TR Dequeue. Bail out of the function when SET_DEQ_PENDING is set, then make the completion handler call the function again and also call xhci_giveback_invalidated_tds(), which typically must be called next. Clean up some code which duplicates functionality of these functions. Fixes: 5ceac4402f5d ("xhci: Handle TD clearing for multiple streams case") CC: stable@vger.kernel.org Signed-off-by: Michal Pecio --- drivers/usb/host/xhci-ring.c | 45 ++++++++++++------------------------ 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 98e12267bbf6..52eb3ee1d7bf 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -959,7 +959,7 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci, * We're also in the event handler, so we can't get re-interrupted if another * Stop Endpoint command completes. * - * only call this when ring is not in a running state + * only call this on a ring in Stopped state (we may need to queue Set TR Deq) */ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) @@ -973,6 +973,13 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) unsigned int slot_id = ep->vdev->slot_id; int err; + /* + * This is not going to work if the hardware is changing its dequeue + * pointers as we look at them. Completion handler will call us later. + */ + if (ep->ep_state & SET_DEQ_PENDING) + return 0; + xhci = ep->xhci; list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) { @@ -1001,7 +1008,9 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) if (td->cancel_status == TD_HALTED || trb_in_td(xhci, td, hw_deq, false)) { switch (td->cancel_status) { case TD_CLEARED: /* TD is already no-op */ - case TD_CLEARING_CACHE: /* set TR deq command already queued */ + break; + case TD_CLEARING_CACHE: /* set TR deq command completed */ + td->cancel_status = TD_CLEARED; break; case TD_DIRTY: /* TD is cached, clear it */ case TD_HALTED: @@ -1358,8 +1367,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, struct xhci_virt_ep *ep; struct xhci_ep_ctx *ep_ctx; struct xhci_slot_ctx *slot_ctx; - struct xhci_td *td, *tmp_td; - bool deferred = false; ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3])); stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2])); @@ -1451,37 +1458,15 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, ep->queued_deq_seg, ep->queued_deq_ptr); } } - /* HW cached TDs cleared from cache, give them back */ - list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, - cancelled_td_list) { - ep_ring = xhci_urb_to_transfer_ring(ep->xhci, td->urb); - if (td->cancel_status == TD_CLEARING_CACHE) { - td->cancel_status = TD_CLEARED; - xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n", - __func__, td->urb); - xhci_td_cleanup(ep->xhci, td, ep_ring, td->status); - } else if (td->cancel_status == TD_CLEARING_CACHE_DEFERRED) { - deferred = true; - } else { - xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n", - __func__, td->urb, td->cancel_status); - } - } cleanup: ep->ep_state &= ~SET_DEQ_PENDING; ep->queued_deq_seg = NULL; ep->queued_deq_ptr = NULL; - if (deferred) { - /* We have more streams to clear */ - xhci_dbg(ep->xhci, "%s: Pending TDs to clear, continuing with invalidation\n", - __func__); - xhci_invalidate_cancelled_tds(ep); - } else { - /* Restart any rings with pending URBs */ - xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__); - ring_doorbell_for_active_rings(xhci, slot_id, ep_index); - } + /* Continue cancellations or restart the ring if done */ + xhci_invalidate_cancelled_tds(ep); + ring_doorbell_for_active_rings(xhci, slot_id, ep_index); + xhci_giveback_invalidated_tds(ep); } static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id, From patchwork Thu Oct 31 09:45:14 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Micha=C5=82_Pecio?= X-Patchwork-Id: 13857762 Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 71F8614B092 for ; Thu, 31 Oct 2024 09:45:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.46 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730367923; cv=none; b=fV2cAPNWzVIAXdLF7vcrJ76yc0d1asRXFjmR12Ef0gGYJkY83lyM+HwUFBW66O/CIwvgp/iV7dLGrhpGe23ZxZZPTIDeiEtZCgFXskNADKHwPyO3OzMG/kabYLvwAA61QzlbDOgb1nW2rHUJ0S/1SqIFeICpZVdPZ4R54GI23UE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730367923; c=relaxed/simple; bh=JunvI9XqI9yAFv1j4jxWHR1+zx58G4fPLc9u+mGQF4A=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=UoxjCCtgTqG5AWSLWm6TnZOixMBQlxx4Lx9Dh/KyshHjx0SatGQ0ZUu/M+9OLKh6zDeinwcjcGHD995IYYiO+U9o5wmckFTQy4EEIcCDuu002mInRLs/7WZJ8W5T6wlxgnBkoMM86PZSThop2GLFhBO0Koqysy2ciNJXZrwHpkw= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=h8AnruTV; arc=none smtp.client-ip=209.85.167.46 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="h8AnruTV" Received: by mail-lf1-f46.google.com with SMTP id 2adb3069b0e04-539f6e1f756so750424e87.0 for ; Thu, 31 Oct 2024 02:45:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730367920; x=1730972720; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=PlltefIDet63GGdv5+a74N+BOLA2GSCLpaDd5YD1nDw=; b=h8AnruTVi019HOVyk0kBTGoy0wMwn9KrdVl6f69ye6BtglPq5e9VEp3CLebmF76rTo TPEf/gNe2zozVB75RGWHtf7Qd5P5VqXL1pCwFvm73Pjkx1ATNcd9yiqXcipg7yvtuzfN jJ0mXi7C9aCU5dIB6ZYSpi5ay4Wz2PAPrkFWYsJM2nqwrwU7Bj0i+yPVnNNlaiFzNiVc UUIG3Ou/aqHiIm1Iui1NSkiiSj/i8ubxcB+S7A9O/r+3kwNhRl9Zlpm8nz5tRJXRnhy/ 5lT0ZEmmPDS1JGJNJ2hiv2hMslFeusQBuIa9IZqwIBLIvhRa0QejGrk4vPwRr7Wn5xJg /RcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730367920; x=1730972720; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=PlltefIDet63GGdv5+a74N+BOLA2GSCLpaDd5YD1nDw=; b=nnkmbJjttOSOROGUJNKLIj8ctIQ/nP3nDaZbYwDoaTE/G6BmeRfkDpMAdzAR8HCw+E RpuhG6QfThFvNNk1QimTxuFkBbmMSL/MvjZvZTiPBzcir7+eTkmeEO1iKFvJar0pHAGr x8XAjdb1hF0jCFZnpIKMXj1OCji9gdZwLpN4fWXLWOxLJwJ0n/KL8xmkm2ktZH1VZA9D YLhB6J1s/DlAXpiIDqRGqsB13udJBTAJ5D7eqcOA4/n7kpeHj5nUHDdgh4vCqoSJNvrj ly+aR9XB5V0yDxHEJrnuePIFUu4ByX3vYYw5WxqbFBScxYt8tg7+2KDautiVnY4cHkhj tCkQ== X-Gm-Message-State: AOJu0Yzo+D5+WgU7xMFdDEpCGYQiexu97f5jAJWE9NlNoj1TkxPlLkNI FTBey6F765hUb1U1Bql7obsi1x0EBL4LQxy9GsEESkqMUkxvdwp/nHkhQg== X-Google-Smtp-Source: AGHT+IGoMQXOo8V70oV6OSJRvri3ox38N/hY7lZV7q9+h9zmY6DoxQdmDnWvZFS4gmWbau5dlc3KZg== X-Received: by 2002:a05:6512:318c:b0:53b:1f14:e11a with SMTP id 2adb3069b0e04-53b348d0d32mr9408763e87.15.1730367919331; Thu, 31 Oct 2024 02:45:19 -0700 (PDT) Received: from foxbook (bfh123.neoplus.adsl.tpnet.pl. [83.28.45.123]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-53c7bdcbe2dsm142066e87.196.2024.10.31.02.45.17 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 31 Oct 2024 02:45:17 -0700 (PDT) Date: Thu, 31 Oct 2024 10:45:14 +0100 From: Michal Pecio To: Mathias Nyman Cc: linux-usb@vger.kernel.org Subject: [PATCH v3 3/3] usb: xhci: Avoid queuing redundant Stop Endpoint commands Message-ID: <20241031104514.256db6c1@foxbook> In-Reply-To: <20241031104159.25f9ff70@foxbook> References: <20241031104159.25f9ff70@foxbook> Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Stop Endpoint command on an already stopped endpoint fails, which may be misinterpreted as a hardware bug by the completion handler. This results in an unnecessary delay with repeated retries of the command. Avoid queuing this command when our endpoint state indicates that it is pointless: when other endpoint management commands are pending and will process cancelled TDs on completion, or when the endpoint is waiting for TT buffer to be cleared. This should reduce unnecessary retries to almost zero. Known exceptions include hard endpoint reset not followed by a ring restart and HW faults causing the endpoint never to restart at all. The endpoint reset case is likely a bug, so dealing with it is left for later, when the matter is analyzed and resolved satisfactorily. Fixes: fd9d55d190c0 ("xhci: retry Stop Endpoint on buggy NEC controllers") CC: stable@vger.kernel.org Signed-off-by: Michal Pecio --- drivers/usb/host/xhci-ring.c | 13 +++++++++++++ drivers/usb/host/xhci.c | 24 ++++++++++++++++++++---- drivers/usb/host/xhci.h | 1 + 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 52eb3ee1d7bf..7726168b72ab 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1071,6 +1071,19 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep) return 0; } +/* + * Erase queued TDs from transfer ring(s) and give back those the xHC didn't + * stop on. If necessary, queue commands to move the xHC off cancelled TDs it + * stopped on. Those will be given back later when the commands complete. + * + * Call under xhci->lock on a stopped endpoint. + */ +void xhci_process_cancelled_tds(struct xhci_virt_ep *ep) +{ + xhci_invalidate_cancelled_tds(ep); + xhci_giveback_invalidated_tds(ep); +} + /* * Returns the TD the endpoint ring halted on. * Only call for non-running rings without streams. diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 1ed4dce7eeb7..e75a6bf4e8d5 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1680,6 +1680,7 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) unsigned long flags; int ret, i; u32 temp; + bool cancelled = false; struct xhci_hcd *xhci; struct urb_priv *urb_priv; struct xhci_td *td; @@ -1766,13 +1767,28 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) td->cancel_status = TD_DIRTY; list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list); + cancelled = true; } } - /* Queue a stop endpoint command, but only if this is - * the first cancellation to be handled. - */ - if (!(ep->ep_state & EP_STOP_CMD_PENDING)) { + if (!cancelled) + goto done; + + /* These completion handlers will sort out cancelled TDs for us */ + if (ep->ep_state & (EP_STOP_CMD_PENDING | EP_HALTED | SET_DEQ_PENDING)) { + xhci_dbg(xhci, "Not queuing Stop Endpoint on slot %d ep %d in state 0x%x\n", + urb->dev->slot_id, ep_index, ep->ep_state); + goto done; + } + + /* In this case no commands are pending but the endpoint is stopped */ + if (ep->ep_state & EP_CLEARING_TT) { + /* and cancelled TDs can be given back right away */ + xhci_dbg(xhci, "Invalidating TDs instantly on slot %d ep %d in state 0x%x\n", + urb->dev->slot_id, ep_index, ep->ep_state); + xhci_process_cancelled_tds(ep); + } else { + /* Otherwise, queue a new Stop Endpoint command */ command = xhci_alloc_command(xhci, false, GFP_ATOMIC); if (!command) { ret = -ENOMEM; diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 0bef6c8b51cf..33e57f408e86 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1914,6 +1914,7 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci, void xhci_cleanup_command_queue(struct xhci_hcd *xhci); void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring); unsigned int count_trbs(u64 addr, u64 len); +void xhci_process_cancelled_tds(struct xhci_virt_ep *ep); /* xHCI roothub code */ void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,