From patchwork Wed Nov 6 10:14:58 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mathias Nyman X-Patchwork-Id: 13864340 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E8CDE1DDC1D; Wed, 6 Nov 2024 10:13:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.10 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730888018; cv=none; b=YyyQYMEtSVBHBc7g0dMWAIf435a/YUw4M18Iy3M7CSeUp5wvQVa4cknTX1nzxOCKhLEKAvghww20RmKf1jkibJhrpZMqSDTuWbEcWL9q+B8Ln/xYsub7w4KB2Jx4qQVYDIGg7g7s5IDclJ40QLCix9WZS1spPl5hXs3lKGoCFLE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730888018; c=relaxed/simple; bh=43ODk6I7RgobtDdGFTmHEg+YHk+2/hr1nIyRuX5jXpo=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=iB1eIXvSgrv+z/T4krYEwoxjIDAeEjzMHK2fhBH9iVwFji6GTN+yU0BiH0qBwNm+5OasKbObS2z5nI925MQS+PY3Xo2q65JZ1R/Af5PMxFrMEPjunsrspyrdBfIP6SjTNN1Hnj4pi/DdxqgncimTB24fVFZrUX+lDSaC6bTbHuI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=none smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=kIy5/hx5; arc=none smtp.client-ip=192.198.163.10 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="kIy5/hx5" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1730888017; x=1762424017; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=43ODk6I7RgobtDdGFTmHEg+YHk+2/hr1nIyRuX5jXpo=; b=kIy5/hx5bDBGka3h3ymU0YVOAx84v4v8p4Lp1c+35l8WCM6zPFj7ck/a bU27dOwoJxMtsM4484qR+Q2qh6qkT4dNzl4EKt4dU3eXPa+OVphoeI2ni 2+0fq5zqF43wk7q/FGxxO04h9eks37Y6t036BHTafNP5Njsp9E9fhqo0T oqjtXqYG1f95X42CYHecqMa9YfQHaEOX5huqUIZbjEfufy/abw4t5e7Xd YUgZ9staNoQrnJAFVTZQ8ioJirikIvwG1U0T5IjQuTtUczkOMAm5wnRvJ vPt3SHo29U7HGI0ccD3VgodiWrBk4OXs/1WZ0kDzZaEY1m+OuSL+ig7iF w==; X-CSE-ConnectionGUID: g8A0aWb9RiyDgayMaXIjXg== X-CSE-MsgGUID: buS3/TfcR4qYWpQ0Grl5kQ== X-IronPort-AV: E=McAfee;i="6700,10204,11247"; a="42059491" X-IronPort-AV: E=Sophos;i="6.11,262,1725346800"; d="scan'208";a="42059491" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by fmvoesa104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Nov 2024 02:13:36 -0800 X-CSE-ConnectionGUID: E06k7VtNR6+rG8G5jgl/TQ== X-CSE-MsgGUID: RilclrP6Sjmbl8Zl83wVPg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,262,1725346800"; d="scan'208";a="84813468" Received: from mattu-haswell.fi.intel.com ([10.237.72.199]) by fmviesa010.fm.intel.com with ESMTP; 06 Nov 2024 02:13:35 -0800 From: Mathias Nyman To: Cc: , Michal Pecio , stable@vger.kernel.org, Mathias Nyman Subject: [PATCH 32/33] usb: xhci: Fix TD invalidation under pending Set TR Dequeue Date: Wed, 6 Nov 2024 12:14:58 +0200 Message-Id: <20241106101459.775897-33-mathias.nyman@linux.intel.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20241106101459.775897-1-mathias.nyman@linux.intel.com> References: <20241106101459.775897-1-mathias.nyman@linux.intel.com> Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: Michal Pecio xhci_invalidate_cancelled_tds() may not work correctly if the hardware is modifying endpoint or stream contexts at the same time by executing a Set TR Dequeue command. And even if it worked, it would be unable to queue Set TR Dequeue for the next stream, failing to clear xHC cache. On stream endpoints, a chain of Set TR Dequeue commands may take some time to execute and we may want to cancel more TDs during this time. Currently this leads to Stop Endpoint completion handler calling this function without testing for SET_DEQ_PENDING, which will trigger the aforementioned problems when it happens. On all endpoints, a halt condition causes Reset Endpoint to be queued and an error status given to the class driver, which may unlink more URBs in response. Stop Endpoint is queued and its handler may execute concurrently with Set TR Dequeue queued by Reset Endpoint handler. (Reset Endpoint handler calls this function too, but there seems to be no possibility of it running concurrently with Set TR Dequeue). Fix xhci_invalidate_cancelled_tds() to work correctly 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 needs to be called next. This seems to fix another potential bug, where the handler would call xhci_invalidate_cancelled_tds(), which may clear some deferred TDs if a sanity check fails, and the TDs wouldn't be given back promptly. Said sanity check seems to be wrong and prone to false positives when the endpoint halts, but fixing it is beyond the scope of this change, besides ensuring that cleared TDs are given back properly. Fixes: 5ceac4402f5d ("xhci: Handle TD clearing for multiple streams case") CC: stable@vger.kernel.org Signed-off-by: Michal Pecio Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci-ring.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index dd23596ccd84..55be03be2374 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -980,6 +980,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) { @@ -1367,7 +1374,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, struct xhci_slot_ctx *slot_ctx; struct xhci_stream_ctx *stream_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])); @@ -1471,8 +1477,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, 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); @@ -1483,11 +1487,15 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id, ep->queued_deq_seg = NULL; ep->queued_deq_ptr = NULL; - if (deferred) { - /* We have more streams to clear */ + /* Check for deferred or newly cancelled TDs */ + if (!list_empty(&ep->cancelled_td_list)) { xhci_dbg(ep->xhci, "%s: Pending TDs to clear, continuing with invalidation\n", __func__); xhci_invalidate_cancelled_tds(ep); + /* Try to restart the endpoint if all is done */ + ring_doorbell_for_active_rings(xhci, slot_id, ep_index); + /* Start giving back any TDs invalidated above */ + xhci_giveback_invalidated_tds(ep); } else { /* Restart any rings with pending URBs */ xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__);