From patchwork Tue Sep 26 21:27:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 145062 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2236500vqu; Tue, 26 Sep 2023 15:41:00 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFocWTiRmP54/ci6YFRjvXuVgrw1Roo4+FAzpR3X+hWCG2mglOyDgHN8iUbG0+txlD2R5Pz X-Received: by 2002:a05:6a21:8182:b0:14b:f9b2:eb7 with SMTP id pd2-20020a056a21818200b0014bf9b20eb7mr201118pzb.59.1695768060568; Tue, 26 Sep 2023 15:41:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695768060; cv=none; d=google.com; s=arc-20160816; b=UbB3qeosRF2GPiKHBV+Vm98o36Tq5Q++qif/R6kh3pcN+gI9YTYVK0pVIRmRvUgrD7 uJR2s004cBiHkeEEiuUDHNtN3EuGCjTjzM29gpoFAEkZMCkswLE66QW0RltpgyOJUoup T1DwlgeBZkhTbcfPAe7ib9bkonehwTeVezYULifTYv3r8+MiK48e11jqW9NuXQjKJWeB xPv19Udp3Z5UnezKY2WZQLzdEmYcE0dFjkux4Q7n1BaVp6Z21tOUDrouYL/ymvK4QARH Yp++pfvtRgbPNwsYLACu/QUivQx6z+ZHpKpfIoPRk/esMZ45za/qoE6nJQLiXr8RaFPW CKYQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=1loE5eg1Eoj4ZTfC+vU3O0r4gZ2OLkuivrhZ+A+bBkA=; fh=+jFQ56TNFLR3IUpgtbTPY77mIf5RVpH/r8s3U4e9QnU=; b=z2plVhScj7wXK1FY4s8m/A83ebH8h+bEmIfM3lWzQ75dHj4ac76/BeRZArJ/DAP27F FAdZE2kb5awBwEqTfw8jwT3V25cjKmpLkrVqP65rUw8tY0IeTd4Ecb9d9sPIq72zdPP+ AQY81fpJYqUfwpc1xSPk9Lk/VPMo+2tMg8PBu2TQn7d5FQEUDXALMFdoLN3M+H6AI1qd yut98Ctb9ckLIkePspfg0yoZKRP7ULhaHc6RzRoVN5rZbnsvgR/CBdSR+p0GrZNPgK2a A57ISBAo2Qxpr4+1rW9RnW+jbZbeoAFUrdCMjjgQD455CaTvsq/QJA9qR3O6I/8YvYSB l1Eg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=n0iEUIJZ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id t2-20020a17090aba8200b00273ce975beesi13836507pjr.114.2023.09.26.15.41.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 15:41:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=n0iEUIJZ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 823C98070ED0; Tue, 26 Sep 2023 15:16:25 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231529AbjIZWQA (ORCPT + 28 others); Tue, 26 Sep 2023 18:16:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49938 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229808AbjIZWN6 (ORCPT ); Tue, 26 Sep 2023 18:13:58 -0400 Received: from mail-oi1-x235.google.com (mail-oi1-x235.google.com [IPv6:2607:f8b0:4864:20::235]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0A71BA267 for ; Tue, 26 Sep 2023 14:28:57 -0700 (PDT) Received: by mail-oi1-x235.google.com with SMTP id 5614622812f47-3ae2ec1a222so4280832b6e.2 for ; Tue, 26 Sep 2023 14:28:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1695763736; x=1696368536; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=1loE5eg1Eoj4ZTfC+vU3O0r4gZ2OLkuivrhZ+A+bBkA=; b=n0iEUIJZLZHal0tpBXmBPg99RnWiFacLJT5nsc//If9A9v3R1yk7gDIrkhtqPg0n5j XcT9T4gNsx0Z/ZE6YUyTcdNHquUmYPhwCsD/yA5FCj2LF8xD40/I80ycF0NRx4iOR5Vb Nix8n4jULZdUHcGa+soPXgh69sMfdAvDZ4lZc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695763736; x=1696368536; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1loE5eg1Eoj4ZTfC+vU3O0r4gZ2OLkuivrhZ+A+bBkA=; b=CTdJO+Th1s8bBJjh1ddjSI7XsGtWczwEaQesGoay1bd5z9DbZNq3bDH7WerdnAmUnX myumu8cj3eIDKIYzpTnc0eva3HbyHXth8JJd8ObW68z5BPqOQmkTMx4JmAwjz7nusle9 lxPI8zG1QGG/a2OiQtvh0GmZeraio1DuoraoI9mrGf/ooA1ibXj6h5lUqPH6A/ZNKWDg MvssOAjG/g8ea/pd74BpviXIoreZD7ah4yf2W8hJgw3veW0pSODAAKhpWLXKWRM/Nt1m Bps9LijHmD1Os8J2nDALzrk94psCPM+qNIDY2dBjg6E1cwBtseeUxhtobDMssJmjaDe9 dy8A== X-Gm-Message-State: AOJu0Yx3gLYmZZuaysCHxBHy0icJy4qKJOptFcE+AxZESpxmuztceSTv L5EwLWgDZIc5l54LTkh8e0futA== X-Received: by 2002:aca:1218:0:b0:3a7:6d64:aa68 with SMTP id 24-20020aca1218000000b003a76d64aa68mr232514ois.18.1695763736267; Tue, 26 Sep 2023 14:28:56 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:f39:c3f2:a3b:4fcd]) by smtp.gmail.com with ESMTPSA id f15-20020aa78b0f000000b0068fece2c190sm10337251pfd.70.2023.09.26.14.28.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 14:28:55 -0700 (PDT) From: Douglas Anderson To: Jakub Kicinski , Hayes Wang , "David S . Miller" Cc: linux-usb@vger.kernel.org, Grant Grundler , Edward Hill , Douglas Anderson , anton@polit.no, bjorn@mork.no, edumazet@google.com, horms@kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, pabeni@redhat.com Subject: [PATCH 1/3] r8152: Increase USB control msg timeout to 5000ms as per spec Date: Tue, 26 Sep 2023 14:27:26 -0700 Message-ID: <20230926142724.1.I6e4fb5ae61b4c6ab32058cb12228fd5bd32da676@changeid> X-Mailer: git-send-email 2.42.0.515.g380fc7ccd1-goog In-Reply-To: <20230926212824.1512665-1-dianders@chromium.org> References: <20230926212824.1512665-1-dianders@chromium.org> MIME-Version: 1.0 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 26 Sep 2023 15:16:25 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778141689790885550 X-GMAIL-MSGID: 1778141689790885550 According to the comment next to USB_CTRL_GET_TIMEOUT and USB_CTRL_SET_TIMEOUT, although sending/receiving control messages is usually quite fast, the spec allows them to take up to 5 seconds. Let's increase the timeout in the Realtek driver from 500ms to 5000ms (using the #defines) to account for this. This is not just a theoretical change. The need for the longer timeout was seen in testing. Specifically, if you drop a sc7180-trogdor based Chromebook into the kdb debugger and then "go" again after sitting in the debugger for a while, the next USB control message takes a long time. Out of ~40 tests the slowest USB control message was 4.5 seconds. While dropping into kdb is not exactly an end-user scenario, the above is similar to what could happen due to an temporary interrupt storm, what could happen if there was a host controller (HW or SW) issue, or what could happen if the Realtek device got into a confused state and needed time to recover. This change is fairly critical since the r8152 driver in Linux doesn't expect register reads/writes (which are backed by USB control messages) to fail. Fixes: ac718b69301c ("net/usb: new driver for RTL8152") Suggested-by: Hayes Wang Signed-off-by: Douglas Anderson --- drivers/net/usb/r8152.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 0c13d9950cd8..482957beae66 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -1212,7 +1212,7 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) ret = usb_control_msg(tp->udev, tp->pipe_ctrl_in, RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, - value, index, tmp, size, 500); + value, index, tmp, size, USB_CTRL_GET_TIMEOUT); if (ret < 0) memset(data, 0xff, size); else @@ -1235,7 +1235,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) ret = usb_control_msg(tp->udev, tp->pipe_ctrl_out, RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE, - value, index, tmp, size, 500); + value, index, tmp, size, USB_CTRL_SET_TIMEOUT); kfree(tmp); @@ -9494,7 +9494,8 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev) ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, - PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), 500); + PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), + USB_CTRL_GET_TIMEOUT); if (ret > 0) ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK; From patchwork Tue Sep 26 21:27:27 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 145132 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2371452vqu; Tue, 26 Sep 2023 21:14:39 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEtmFDs+dQYHOzlNNpjKuB+1K+5zgd032LRTofJzXg/+ktNo9ArfXAt5mawq6G+ZXojXKh0 X-Received: by 2002:a05:6871:1c8:b0:1ba:989b:ca65 with SMTP id q8-20020a05687101c800b001ba989bca65mr1299860oad.19.1695788079644; Tue, 26 Sep 2023 21:14:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695788079; cv=none; d=google.com; s=arc-20160816; b=eWTNb631LV9+qzBfY3936wqiO+XHphaOCiFQNESm2Gkjhyo2mqX4O3pfboJ+1+vBbF /K2HUBEwBjbV8vU8Y0ivBZx75nwHepiGlNt/cVhoAxDl9NOAOpcYpX7po3fylONM6jkK I261qbTyg2xWboDwQDqKQJwLHa68gHmI8Okh2knkLIsHh8WPdXfZkTL+DnvfKidQpEgw GAvKpTl93E75nd0M9drqZXfvgEVUuFmo1WU8IXxmAE7TKdo1o+M2m9EthjX9vsdRbBqx d4Xuwxb2xws8ZYc/qCUP1gvRTQZG7jdU0WVoDEqgfRzs7TpJpnfIwl5bb3gO9HOPMLPR ppEg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=qVCYC0vkrZ6zXTzyEFnI7OnTxuwy99dqGDu6Ppi9+fs=; fh=NyP4UI5Vj96jY/xxS6Kk8+vInDjmHuA1DJ5b7iYQuQs=; b=pqUsf5DhOaazUxC+K1wyDihq3RqZmpbjvkC2NYSI9hZQGpBRrwalaD8PXzE8DyZQ21 XoJrkEk0QC9+BJyjoXRTIEmbfzVh4L+XcOjU6HFdFCRpLVvJzPLJVwguWoBXr7j4scuy SJGNs6fGmayyIHc4+Y1U92MfV1pGNBQD2/jQzrqsxkKPzusgqMxTstgg4Axhf6eYkfNe 86usKY0QEYLn5S3qDtTl3EqeedYdODFHDQ4BHXXmzd+Bz/7iCoXIdCfP/TCmlPHL07Nw T8D9WNb9Pf54CBi35De2q/mo5vq9SW/ZBl/jQgd/TrLkw5n5+NvtBeGB3qj4tvVW50uA YBhQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=SJGNymYY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id a21-20020a63e855000000b0056aa105b6a4si14816696pgk.79.2023.09.26.21.14.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 21:14:39 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=SJGNymYY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 1FF5F81BFCD8; Tue, 26 Sep 2023 19:35:16 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235803AbjI0Cez (ORCPT + 28 others); Tue, 26 Sep 2023 22:34:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43750 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235957AbjI0Ccx (ORCPT ); Tue, 26 Sep 2023 22:32:53 -0400 Received: from mail-pf1-x435.google.com (mail-pf1-x435.google.com [IPv6:2607:f8b0:4864:20::435]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C60C1A26C for ; Tue, 26 Sep 2023 14:28:58 -0700 (PDT) Received: by mail-pf1-x435.google.com with SMTP id d2e1a72fcca58-690d8c05784so7563281b3a.2 for ; Tue, 26 Sep 2023 14:28:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1695763738; x=1696368538; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=qVCYC0vkrZ6zXTzyEFnI7OnTxuwy99dqGDu6Ppi9+fs=; b=SJGNymYYAvCwIq47qwJNzf0kqY+QCYUyEqgOttrW+4HnQ2tokbmtWl1GicjlIg6y5a c5oIwCoVzEch2FvoWvgJEqYP72kB4KQfWVzqKhRxi9IKpdGl3oimCzrtXzu1yV7UtLgD wggXHhpwkNilkn+lNkZx5hi84jB3z/7qUaJFg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695763738; x=1696368538; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qVCYC0vkrZ6zXTzyEFnI7OnTxuwy99dqGDu6Ppi9+fs=; b=Zh5kOugAVoQnmF5YQy9mxHfqqUMMCeLHigwP4G+Tp3yhiRB1DKw31yUgvRERlaTTTq lOm8c0Sx8UtOvFkG0qkLdI90IMD3j6AX2ysnMCJKB3WMPOb33CyELxb2tawOxC4mOjDF 7uTUaqkVrWcUghbhzCMzr43rPweji0ssq/KgT0WEP0KvnMjQ7uNlVpiy7X+b3j6NbHkm SCFTl7dW08cRrvHj7Gn2VOpdwH0a4GC94v3XSsZsaTYE+TYb1mt5iQympsTEDr4v4thh GhBs0zshv5uhGf25DxbHpcMlvgGU0qynQYcKTyxXa8SsKWalgsQFz4aY0yWnFO1EK2Yv cT+w== X-Gm-Message-State: AOJu0YxMj7aNZni2CMdmmCPulQbcPRbTVS7xaEYTWmoBRSFxm5R5TlLU NyU4VErrY13OYRLApdyqAluUiw== X-Received: by 2002:a05:6a00:1acb:b0:68e:29a6:e247 with SMTP id f11-20020a056a001acb00b0068e29a6e247mr233576pfv.10.1695763738175; Tue, 26 Sep 2023 14:28:58 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:f39:c3f2:a3b:4fcd]) by smtp.gmail.com with ESMTPSA id f15-20020aa78b0f000000b0068fece2c190sm10337251pfd.70.2023.09.26.14.28.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 14:28:57 -0700 (PDT) From: Douglas Anderson To: Jakub Kicinski , Hayes Wang , "David S . Miller" Cc: linux-usb@vger.kernel.org, Grant Grundler , Edward Hill , Douglas Anderson , andre.przywara@arm.com, bjorn@mork.no, edumazet@google.com, gaul@gaul.org, horms@kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, pabeni@redhat.com Subject: [PATCH 2/3] r8152: Retry register reads/writes Date: Tue, 26 Sep 2023 14:27:27 -0700 Message-ID: <20230926142724.2.I65ea4ac938a55877dc99fdf5b3883ad92d8abce2@changeid> X-Mailer: git-send-email 2.42.0.515.g380fc7ccd1-goog In-Reply-To: <20230926212824.1512665-1-dianders@chromium.org> References: <20230926212824.1512665-1-dianders@chromium.org> MIME-Version: 1.0 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 26 Sep 2023 19:35:16 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778162681232195124 X-GMAIL-MSGID: 1778162681232195124 Even though the functions to read/write registers can fail, most of the places in the r8152 driver that read/write register values don't check error codes. The lack of error code checking is problematic in at least two ways. The first problem is that the r8152 driver often uses code patterns similar to this: x = read_register() x = x | SOME_BIT; write_register(x); ...with the above pattern, if the read_register() fails and returns garbage then we'll end up trying to write modified garbage back to the Realtek adapter. If the write_register() succeeds that's bad. Note that as of commit f53a7ad18959 ("r8152: Set memory to all 0xFFs on failed reg reads") the "garbage" returned by read_register() will at least be consistent garbage, but it is still garbage. It turns out that this problem is very serious. Writing garbage to some of the hardware registers on the Ethernet adapter can put the adapter in such a bad state that it needs to be power cycled (fully unplugged and plugged in again) before it can enumerate again. The second problem is that the r8152 driver generally has functions that are long sequences of register writes. Assuming everything will be OK if a random register write fails in the middle isn't a great assumption. One might wonder if the above two problems are real. You could ask if we would really have a successful write after a failed read. It turns out that the answer appears to be "yes, this can happen". In fact, we've seen at least two distinct failure modes where this happens. On a sc7180-trogdor Chromebook if you drop into kdb for a while and then resume, you can see: 1. We get a "Tx timeout" 2. The "Tx timeout" queues up a USB reset. 3. In rtl8152_pre_reset() we try to reinit the hardware. 4. The first several (2-9) register accesses fail with a timeout, then things recover. The above test case was actually fixed by the patch ("r8152: Increase USB control msg timeout to 5000ms as per spec") but at least shows that we really can see successful calls after failed ones. On a different (AMD) based Chromebook, we found that during reboot tests we'd also sometimes get a transitory failure. In this case we saw -EPIPE being returned sometimes. Retrying one time worked fine. To keep things robust, let's try register access up to 3 times. If we get 3 5-second timeouts in a row this could block things for 15 seconds but that hasn't been seen in practice. If we see that happening and there is a better way to solve it then we can add a special case for that later. Signed-off-by: Douglas Anderson --- Originally when looking at this problem I thought that the obvious solution was to "just" add better error handling to the driver. This _sounds_ appealing, but it's a massive change and touches a significant portion of the lines in this driver. It's also not always obvious what the driver should be doing to handle errors. drivers/net/usb/r8152.c | 67 +++++++++++++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 482957beae66..976d6caf2f04 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -1200,6 +1200,52 @@ static unsigned int agg_buf_sz = 16384; #define RTL_LIMITED_TSO_SIZE (size_to_mtu(agg_buf_sz) - sizeof(struct tx_desc)) +/* If we get a failure and the USB device is still attached when trying to read + * or write registers then we'll retry a few times. Failures accessing registers + * shouldn't be common and this adds robustness. Much code in the driver doesn't + * check for errors. Notably, many parts of the driver do a read/modify/write + * of a register value without confirming that the read succeeded. Writing back + * modified garbage like this can fully wedge the adapter, requiring a power + * cycle. + */ +#define REGISTER_ACCESS_TRIES 3 + +static +int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request, + __u8 requesttype, __u16 value, __u16 index, void *data, + __u16 size, const char *msg_tag) +{ + int i; + int ret; + + for (i = 0; i < REGISTER_ACCESS_TRIES; i++) { + ret = usb_control_msg(udev, pipe, request, requesttype, + value, index, data, size, + USB_CTRL_GET_TIMEOUT); + + /* No need to retry or spam errors if the USB device got + * unplugged; just return immediately. + */ + if (udev->state == USB_STATE_NOTATTACHED) + return ret; + + if (ret >= 0) + break; + } + + if (ret < 0) { + dev_err(&udev->dev, + "Failed to %s %d bytes at %#06x/%#06x (%d)\n", + msg_tag, size, value, index, ret); + } else if (i != 0) { + dev_warn(&udev->dev, + "Needed %d tries to %s %d bytes at %#06x/%#06x\n", + i + 1, msg_tag, size, value, index); + } + + return ret; +} + static int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) { @@ -1210,9 +1256,10 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) if (!tmp) return -ENOMEM; - ret = usb_control_msg(tp->udev, tp->pipe_ctrl_in, - RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, - value, index, tmp, size, USB_CTRL_GET_TIMEOUT); + ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_in, + RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, + value, index, tmp, size, "read"); + if (ret < 0) memset(data, 0xff, size); else @@ -1233,9 +1280,9 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) if (!tmp) return -ENOMEM; - ret = usb_control_msg(tp->udev, tp->pipe_ctrl_out, - RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE, - value, index, tmp, size, USB_CTRL_SET_TIMEOUT); + ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_out, + RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE, + value, index, tmp, size, "write"); kfree(tmp); @@ -9492,10 +9539,10 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev) if (!tmp) return 0; - ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), - RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, - PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), - USB_CTRL_GET_TIMEOUT); + ret = r8152_control_msg(udev, usb_rcvctrlpipe(udev, 0), + RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, + PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), + "read"); if (ret > 0) ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK; From patchwork Tue Sep 26 21:27:28 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 145130 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2369412vqu; Tue, 26 Sep 2023 21:09:23 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGGtbwQuzhHgqbL4by4VQdCF2/nXNanDgO3kaABHNF4uQPdmP0gvjK1M/Nxi0olgNqvXLaC X-Received: by 2002:a17:902:db05:b0:1c0:cbaf:6954 with SMTP id m5-20020a170902db0500b001c0cbaf6954mr1583565plx.25.1695787763228; Tue, 26 Sep 2023 21:09:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695787763; cv=none; d=google.com; s=arc-20160816; b=RjeaqCdwwWn+UaYrteIIC8qyo0f9HEhLF9yP8OggbcDcP1oNXyd4aClVehcR5B2mQE +uVABbXURBmpF+/YB4hYFpKTs3h9O2YZT5NbrFiB+8ww7wFfbOtzNbfcN2URVAA623q6 qSeez1SwJB6uMUL/GBuQVvBnZn3L5Efr169rWBE2KGen9FA9SfwBw6edg4A2mkmbQoTG M+oDCON2VMsirihXm83HuzubV7G+6m06N7Fojk/14fKtvJ/IA1hqd0HJzH1Qek15kwoL WbiYWKjphvP+O5Qwnfn5/xqURpynjscU72Kyz3T2sP/l2KaGhSEJp/o2b3A1j1MTHwGK dNCw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=JZ7x1sGJs3IwI2EHRWherpwDYB2HwErhY7l2CYJSHl8=; fh=Fes9nhUOsCacsX0OIeDnDf4a0ukWGQ+Zd4/6dfHJnFM=; b=tKa8r173AAYvG+OanvuFgIkp7h0+mDYBvHzxp8a6pOppODmMJFPVj5SJG2hROcQ3g3 /7A/8dWv0jAZfMBxmxyorLBrDz8YlgvoLJwIRNYVv2LF9lnDf+RfzBIHPNAGKuEfbXUs 0BGKYE1PyQvs1NNcLqyiAsCjyOK/9q4c+oh131E28DwREN2nYQy6q39KHCoOtmLTyEtP PZTGQ7IzfRsdPWGmVkWSxZW8WTxefWKWzBqjPZjI3ksXptDceqzUlnjbWJLhdCmUMSTX A90oQu6RAehI2Fq6H0ZtqczpfZBw7kLLdk5ke8feR2jinqX2SKCwgF+Iyt6rNxhunrRN nkjQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=VJIuaHiW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from snail.vger.email (snail.vger.email. [23.128.96.37]) by mx.google.com with ESMTPS id n8-20020a170902d2c800b001bf1d1d99b0si15301392plc.568.2023.09.26.21.09.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 21:09:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=VJIuaHiW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 03655813CD9C; Tue, 26 Sep 2023 15:35:58 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229793AbjIZWfw (ORCPT + 28 others); Tue, 26 Sep 2023 18:35:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53646 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230159AbjIZWdu (ORCPT ); Tue, 26 Sep 2023 18:33:50 -0400 Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 69814A276 for ; Tue, 26 Sep 2023 14:29:00 -0700 (PDT) Received: by mail-pf1-x42b.google.com with SMTP id d2e1a72fcca58-690d2441b95so7339859b3a.1 for ; Tue, 26 Sep 2023 14:29:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1695763740; x=1696368540; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=JZ7x1sGJs3IwI2EHRWherpwDYB2HwErhY7l2CYJSHl8=; b=VJIuaHiWKyIqjyfefgBHHnrzx1bPs6hpgjKgZkpdlvzWpBF1epF9oc6phvRSxezdsF 2wnyEHTfBcq9kjMIivQIxor8T/f84K53NS8ky5knTzHpKLhghakZ2tXKeRZ4yD+Fz+E3 zJYrcsrG2SEUInFEiXGd6DYDpAiPd+5Yd2BK4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695763740; x=1696368540; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=JZ7x1sGJs3IwI2EHRWherpwDYB2HwErhY7l2CYJSHl8=; b=U3KZ1wALPZPB6u/LnR3GsvqaETQlgBvae8TQsT0GmH9tXSC+DvbCa0abeVjrpQ1fcQ sAYEI+A0WwjzezSJWjA2KsN0uGiQA0FGifRoMZK3p7/E3mSXVdNXEV2k9uyetrzr2xuY 6qP9YWh8EZ41SI+D7oQqNeGJREH9f1ssa15Z9rFjcf5VrJ8ftUVwGI3ettrdoCmZ840P 8WLj4rTE7Om+W/J2V7xtk/Ze7/AH4GWW9EjBiaOkcyVx4dyhlr5OcriO5+64gTiC/RIW P8k93DIi9n8l0L73e/YpNoikFwW1/0gBbjOeTXS3RPhfDD5ger7AZDhBQ0Vymx2Yxi0g S8Pg== X-Gm-Message-State: AOJu0Yzbl0R034nRyvgqhUVmKkwdhMBz1mVAD0BTtGyKxAqhWzLt2tnm bu23JHR3bciiMzRm8ef4t+v3+A== X-Received: by 2002:a62:ee06:0:b0:68f:a92a:8509 with SMTP id e6-20020a62ee06000000b0068fa92a8509mr4687125pfi.7.1695763739823; Tue, 26 Sep 2023 14:28:59 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:f39:c3f2:a3b:4fcd]) by smtp.gmail.com with ESMTPSA id f15-20020aa78b0f000000b0068fece2c190sm10337251pfd.70.2023.09.26.14.28.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 14:28:59 -0700 (PDT) From: Douglas Anderson To: Jakub Kicinski , Hayes Wang , "David S . Miller" Cc: linux-usb@vger.kernel.org, Grant Grundler , Edward Hill , Douglas Anderson , bjorn@mork.no, edumazet@google.com, horms@kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, pabeni@redhat.com Subject: [PATCH 3/3] r8152: Block future register access if register access fails Date: Tue, 26 Sep 2023 14:27:28 -0700 Message-ID: <20230926142724.3.Ib2affdbfdc2527aaeef9b46d4f23f7c04147faeb@changeid> X-Mailer: git-send-email 2.42.0.515.g380fc7ccd1-goog In-Reply-To: <20230926212824.1512665-1-dianders@chromium.org> References: <20230926212824.1512665-1-dianders@chromium.org> MIME-Version: 1.0 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Tue, 26 Sep 2023 15:35:59 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778162349943448745 X-GMAIL-MSGID: 1778162349943448745 As talked about in the patch ("r8152: Retry register reads/writes"), much of the r8152 driver doesn't check for error codes when modifying the adapter's registers. Specifically, the worst part of this appears to be when the driver does a read-modify-write of a register. If the read is garbage (because the read failed) and the write succeeds, we end up writing modified garbage to the adapter and this appears to be able to hard wedge it. After we added the retries, it should be nearly impossible to see this problem. After all, the problem shows up if the register read failed and _then_ the register write worked. If the read fails for many times in a row, chances are things are really dead and no future writes will succeed. However, nearly impossible isn't actually impossible. To be safe, let's fully lock out all register access if all of our retries fail. When we do this, we'll try to queue up a USB reset and try to unlock register access after the reset. To avoid looping forever, we'll only attempt this a handful of times. Signed-off-by: Douglas Anderson --- This patch was tested with REGISTER_ACCESS_TRIES set to 1, effectively disabling the patch ("r8152: Retry register reads/writes"). I also decreased the timeout back to 500ms so I could use kgdb to reproduce failures. To confirm that the reset queuing worked, I allowed queueing a reset even if pre-reset was running. That worked fine, but did cause a double-reset. drivers/net/usb/r8152.c | 80 +++++++++++++++++++++++++++++++++++------ 1 file changed, 69 insertions(+), 11 deletions(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 976d6caf2f04..a7a55849fa1a 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -953,6 +953,10 @@ struct r8152 { u8 version; u8 duplex; u8 autoneg; + + bool reg_access_fail; + bool in_pre_reset; + unsigned int reg_access_reset_count; }; /** @@ -1210,14 +1214,22 @@ static unsigned int agg_buf_sz = 16384; */ #define REGISTER_ACCESS_TRIES 3 +/* If register access fails too many times, stop trying to access the registers + * and issue a reset to fix the issue. + */ +#define REGISTER_ACCESS_MAX_RESETS 3 + static int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request, __u8 requesttype, __u16 value, __u16 index, void *data, - __u16 size, const char *msg_tag) + __u16 size, const char *msg_tag, struct r8152 *tp) { int i; int ret; + if (tp && tp->reg_access_fail) + return -EIO; + for (i = 0; i < REGISTER_ACCESS_TRIES; i++) { ret = usb_control_msg(udev, pipe, request, requesttype, value, index, data, size, @@ -1233,14 +1245,54 @@ int r8152_control_msg(struct usb_device *udev, unsigned int pipe, __u8 request, break; } - if (ret < 0) { + if (ret >= 0) { + if (tp) + tp->reg_access_reset_count = 0; + + if (i != 0) + dev_warn(&udev->dev, + "Needed %d tries to %s %d bytes at %#06x/%#06x\n", + i + 1, msg_tag, size, value, index); + + return ret; + } + + dev_err(&udev->dev, + "Failed to %s %d bytes at %#06x/%#06x (%d)\n", + msg_tag, size, value, index, ret); + + /* If tp is NULL then we're called from early probe or from the + * r8153_ecm module. In either case, the retries above are pretty much + * all we can do. Bail. + */ + if (!tp) + return ret; + + /* Block all future register access until we reset. This prevents the + * parts of the driver that do read-modify-write without checking for + * errors from writing back modified garbage to the adapter. + */ + tp->reg_access_fail = true; + + /* Failing to access registers in pre-reset is not surprising since we + * wouldn't be resetting if things were behaving normally. The register + * access we do in pre-reset isn't truly mandatory--we're just reusing + * the disable() function and trying to be nice by powering the + * adapter down before resetting it. + * + * If we're in pre-reset, we'll return right away and not try to queue + * up yet another reset. We know the post-reset is already coming. + */ + if (tp->in_pre_reset) + return ret; + + if (tp->reg_access_reset_count < REGISTER_ACCESS_MAX_RESETS) { + usb_queue_reset_device(tp->intf); + tp->reg_access_reset_count++; + } else if (tp->reg_access_reset_count == REGISTER_ACCESS_MAX_RESETS) { dev_err(&udev->dev, - "Failed to %s %d bytes at %#06x/%#06x (%d)\n", - msg_tag, size, value, index, ret); - } else if (i != 0) { - dev_warn(&udev->dev, - "Needed %d tries to %s %d bytes at %#06x/%#06x\n", - i + 1, msg_tag, size, value, index); + "Tried to reset %d times; giving up.\n", + REGISTER_ACCESS_MAX_RESETS); } return ret; @@ -1258,7 +1310,7 @@ int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_in, RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, - value, index, tmp, size, "read"); + value, index, tmp, size, "read", tp); if (ret < 0) memset(data, 0xff, size); @@ -1282,7 +1334,7 @@ int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) ret = r8152_control_msg(tp->udev, tp->pipe_ctrl_out, RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE, - value, index, tmp, size, "write"); + value, index, tmp, size, "write", tp); kfree(tmp); @@ -8317,7 +8369,9 @@ static int rtl8152_pre_reset(struct usb_interface *intf) napi_disable(&tp->napi); if (netif_carrier_ok(netdev)) { mutex_lock(&tp->control); + tp->in_pre_reset = true; tp->rtl_ops.disable(tp); + tp->in_pre_reset = false; mutex_unlock(&tp->control); } @@ -8333,6 +8387,10 @@ static int rtl8152_post_reset(struct usb_interface *intf) if (!tp) return 0; + mutex_lock(&tp->control); + tp->reg_access_fail = false; + mutex_unlock(&tp->control); + /* reset the MAC address in case of policy change */ if (determine_ethernet_addr(tp, &sa) >= 0) { rtnl_lock(); @@ -9542,7 +9600,7 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev) ret = r8152_control_msg(udev, usb_rcvctrlpipe(udev, 0), RTL8152_REQ_GET_REGS, RTL8152_REQT_READ, PLA_TCR0, MCU_TYPE_PLA, tmp, sizeof(*tmp), - "read"); + "read", NULL); if (ret > 0) ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;