Message ID | 20231120131540.9442-1-ansuelsmth@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9910:0:b0:403:3b70:6f57 with SMTP id i16csp2191496vqn; Mon, 20 Nov 2023 05:18:03 -0800 (PST) X-Google-Smtp-Source: AGHT+IEDeuG48QXbTUEUOVR3YEbo1pHKAdwfOrP7ZzL3/mfx+Jh7xMPkdYhy9w+r4snd0TCtRCJ6 X-Received: by 2002:a17:90a:a413:b0:280:a4a1:5d03 with SMTP id y19-20020a17090aa41300b00280a4a15d03mr990000pjp.4.1700486283690; Mon, 20 Nov 2023 05:18:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700486283; cv=none; d=google.com; s=arc-20160816; b=0t5vCtmGnD1xSPt64dhwbVq+i0bXwpK7dVw7ENH7aF2UEUIUAtXAigXQnCodBaK7eu LOga6MjHhL781cUZw6OKW/voO5pGzNOiaxMBaLT+cEuzu0PCOXJDVk004ywXw2e5SWft zIjYqx6SZQxb//OHqaHGqKpAeHcArTeqikvuWTPr+QOK2J7ztj4Tr3lPpKtUdyGEkEDV Ld8nXMX+56MWuHpSb9/Kk+5DAncvAp1BInVU45VZdDCYu9AKqtiQSXukVXtHj0URPHDv lZHhYS+cvpUHIU04sJdyp9k3kYZksLJnACEJ80AysIRpEst70fAY3Mfb6lEJDgjQgDLf oQWw== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=X7rYESumyoHA+3OjYLlWpPoNiAXnvgYxNGxcQD7theY=; fh=FN1Zrg7tI0TFQ/wlQU1yDE6eiTIZgV9MgEbR9HGA/R4=; b=FUYTXOXiPKWlnLeQZqbubvhpmDXWf6T6eHhd1nBkHuCd6kP8o2rrwzajSH05/cPR3u CfTC+mNI8B3O5vyW+HCmrAzYpHfKYUpDzsSAFuVEJcw9mZ6331MY7QzYVnqF3zmoppxu 9aipYwQP47a7QNoIbXzBc3GQKV6WePooaT/yUfoVHWq9NpKwxZU+iuMRr18U1tolvvnd lJeTGft/i2Gde5w8iOtr6bErIxgtN1ccjPsdh7XJsMqNsQ1qpPXAG5x6njzLZJ3w97mg pSxkAYEZ1jLSIjzJ65Qe3RSfIYE4V1pA5KBMEsebEZ+h3B+tls+9Uosuchv+1kquMxko ME3w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=LjntltSi; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id b19-20020a63d313000000b005ac8d44bad4si8136983pgg.592.2023.11.20.05.18.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Nov 2023 05:18:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=LjntltSi; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 181BA8090EBB; Mon, 20 Nov 2023 05:16:26 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232363AbjKTNQL (ORCPT <rfc822;heyuhang3455@gmail.com> + 27 others); Mon, 20 Nov 2023 08:16:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56166 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231961AbjKTNQK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 20 Nov 2023 08:16:10 -0500 Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BBCF7F7; Mon, 20 Nov 2023 05:16:04 -0800 (PST) Received: by mail-wr1-x430.google.com with SMTP id ffacd0b85a97d-32faea0fa1fso2521630f8f.1; Mon, 20 Nov 2023 05:16:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1700486163; x=1701090963; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=X7rYESumyoHA+3OjYLlWpPoNiAXnvgYxNGxcQD7theY=; b=LjntltSiwd5fVAQ+LoTCzOqNscxspe91SWRZ15AIYLYTarZ9KKOe66h5ohaI0AwNgm ZGUuBpIkQlzTecwW+y/3ClbfrAGsWQZ9nl1vUgDIEPOyvxMnWbiIletrXrrOzS1eFUwD SRcpd6ayU6kkqH80vzxVqQjf1roD5m5kz0YBMq9jn0pRwh3gnSa0BmjX9+f+kxqcU1Sr KOlWtpWu0MLrPl5IyqEvfnFonuGKJjDhsapb/mfxMWNp2LFJWYP+2m5hRJh+4eM4CQ+x CKDvfln2cjSk49CXdoqsoOl07AXmW1zt2oMa/2IYR5p2g8+uRadIz/dlEpz7O2CYZ8tN 9R3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700486163; x=1701090963; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=X7rYESumyoHA+3OjYLlWpPoNiAXnvgYxNGxcQD7theY=; b=OzvdXfB45xgYZeSmwWD5nfxMlUVyiKWRJHLtlsbHyAm2VO5R6L1f4z2H+h1wypzY6j 52LLvmVx+c4c1tzScNjbg6tl+ITbCEjIXfbtCyIqMwAOuoSs30liHIiNzc3bkWUBK5PQ u3fJNz7F6t67HawdPyPbfQbfE5rtbRAkHXhTrrvHV+GnDmet7I5EOhC860ZkRPWGyfcW MRuCaUsjg+RYI0LKwDZTTvD0OUbchWpbf62IdXJ9aMyewnb3RqB5hnxcmgtSxBjmRnA8 wiMRV4DvymZtbgEcKU4S6GIwPyEUKsdA/iTD4ThL5pUFJ6uSRpM1LG3y7j95G/GqlFqk WFxg== X-Gm-Message-State: AOJu0YzS9fpsxN/52jizAuC5drjbOdQhL9jO9mA53ZdJOqQYgzvabhie +bXMQd6LnUfY23TTa+kg2CM= X-Received: by 2002:adf:d1ca:0:b0:331:6a14:b8d5 with SMTP id b10-20020adfd1ca000000b003316a14b8d5mr7650035wrd.29.1700486162654; Mon, 20 Nov 2023 05:16:02 -0800 (PST) Received: from localhost.localdomain (93-34-89-13.ip49.fastwebnet.it. [93.34.89.13]) by smtp.googlemail.com with ESMTPSA id x11-20020adff0cb000000b00332cb4697ebsm1263592wro.55.2023.11.20.05.16.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Nov 2023 05:16:02 -0800 (PST) From: Christian Marangi <ansuelsmth@gmail.com> To: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Florian Fainelli <f.fainelli@gmail.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Christian Marangi <ansuelsmth@gmail.com> Subject: [net-next PATCH] net: phy: correctly check soft_reset ret ONLY if defined for PHY Date: Mon, 20 Nov 2023 14:15:40 +0100 Message-Id: <20231120131540.9442-1-ansuelsmth@gmail.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Mon, 20 Nov 2023 05:16:26 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1783089105244083730 X-GMAIL-MSGID: 1783089105244083730 |
Series |
[net-next] net: phy: correctly check soft_reset ret ONLY if defined for PHY
|
|
Commit Message
Christian Marangi
Nov. 20, 2023, 1:15 p.m. UTC
soft_reset call for phy_init_hw had multiple revision across the years
and the implementation goes back to 2014. Originally was a simple call
to write the generic PHY reset BIT, it was then moved to a dedicated
function. It was then added the option for PHY driver to define their
own special way to reset the PHY. Till this change, checking for ret was
correct as it was always filled by either the generic reset or the
custom implementation. This changed tho with commit 6e2d85ec0559 ("net:
phy: Stop with excessive soft reset"), as the generic reset call to PHY
was dropped but the ret check was never made entirely optional and
dependent whether soft_reset was defined for the PHY driver or not.
Luckly nothing was ever added before the soft_reset call so the ret
check (in the case where a PHY didn't had soft_reset defined) although
wrong, never caused problems as ret was init 0 at the start of
phy_init_hw.
To prevent any kind of problem and to make the function cleaner and more
robust, correctly move the ret check if the soft_reset section making it
optional and needed only with the function defined.
Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/phy/phy_device.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Comments
On Mon, Nov 20, 2023 at 06:15:43PM +0100, Larysa Zaremba wrote: > On Mon, Nov 20, 2023 at 02:15:40PM +0100, Christian Marangi wrote: > > soft_reset call for phy_init_hw had multiple revision across the years > > and the implementation goes back to 2014. Originally was a simple call > > to write the generic PHY reset BIT, it was then moved to a dedicated > > function. It was then added the option for PHY driver to define their > > own special way to reset the PHY. Till this change, checking for ret was > > correct as it was always filled by either the generic reset or the > > custom implementation. This changed tho with commit 6e2d85ec0559 ("net: > > phy: Stop with excessive soft reset"), as the generic reset call to PHY > > was dropped but the ret check was never made entirely optional and > > dependent whether soft_reset was defined for the PHY driver or not. > > > > Luckly nothing was ever added before the soft_reset call so the ret > > check (in the case where a PHY didn't had soft_reset defined) although > > wrong, never caused problems as ret was init 0 at the start of > > phy_init_hw. > > > > To prevent any kind of problem and to make the function cleaner and more > > robust, correctly move the ret check if the soft_reset section making it > > optional and needed only with the function defined. > > > > Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > drivers/net/phy/phy_device.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index 2ce74593d6e4..478126f6b5bc 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -1235,14 +1235,13 @@ int phy_init_hw(struct phy_device *phydev) > > > > if (phydev->drv->soft_reset) { > > ret = phydev->drv->soft_reset(phydev); > > + if (ret < 0) > > + return ret; > > + > > /* see comment in genphy_soft_reset for an explanation */ > > - if (!ret) > > - phydev->suspended = 0; > > + phydev->suspended = 0; > > Are you sure 'suspended' should not be cleared, if soft_reset fails? > > > } Idea is not change current implementation... And before this, suspended was cleared only with reset not failing. > > > > - if (ret < 0) > > - return ret; > > - > > ret = phy_scan_fixups(phydev); > > if (ret < 0) > > return ret; > > -- > > 2.40.1 > > > >
On Mon, Nov 20, 2023 at 09:42:34AM -0800, Jakub Kicinski wrote: > On Mon, 20 Nov 2023 14:15:40 +0100 Christian Marangi wrote: > > Luckly nothing was ever added before the soft_reset call so the ret > > check (in the case where a PHY didn't had soft_reset defined) although > > wrong, never caused problems as ret was init 0 at the start of > > phy_init_hw. > > not currently a bug => no Fixes tag, please I know it's not a bug but still the referenced commit was wrong. Can I at least use Ref to reference it? Due to the changes done to this function, it's hard to catch where the problem arised with a git blame.
On Mon, Nov 20, 2023 at 02:15:40PM +0100, Christian Marangi wrote: > soft_reset call for phy_init_hw had multiple revision across the years > and the implementation goes back to 2014. Originally was a simple call > to write the generic PHY reset BIT, it was then moved to a dedicated > function. It was then added the option for PHY driver to define their > own special way to reset the PHY. Till this change, checking for ret was > correct as it was always filled by either the generic reset or the > custom implementation. This changed tho with commit 6e2d85ec0559 ("net: > phy: Stop with excessive soft reset"), as the generic reset call to PHY > was dropped but the ret check was never made entirely optional and > dependent whether soft_reset was defined for the PHY driver or not. > > Luckly nothing was ever added before the soft_reset call so the ret > check (in the case where a PHY didn't had soft_reset defined) although > wrong, never caused problems as ret was init 0 at the start of > phy_init_hw. > > To prevent any kind of problem and to make the function cleaner and more > robust, correctly move the ret check if the soft_reset section making it > optional and needed only with the function defined. > > Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > drivers/net/phy/phy_device.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 2ce74593d6e4..478126f6b5bc 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1235,14 +1235,13 @@ int phy_init_hw(struct phy_device *phydev) > > if (phydev->drv->soft_reset) { > ret = phydev->drv->soft_reset(phydev); > + if (ret < 0) > + return ret; > + > /* see comment in genphy_soft_reset for an explanation */ > - if (!ret) > - phydev->suspended = 0; > + phydev->suspended = 0; Are you sure 'suspended' should not be cleared, if soft_reset fails? > } > > - if (ret < 0) > - return ret; > - > ret = phy_scan_fixups(phydev); > if (ret < 0) > return ret; > -- > 2.40.1 > >
On Mon, 20 Nov 2023 14:15:40 +0100 Christian Marangi wrote: > Luckly nothing was ever added before the soft_reset call so the ret > check (in the case where a PHY didn't had soft_reset defined) although > wrong, never caused problems as ret was init 0 at the start of > phy_init_hw. not currently a bug => no Fixes tag, please
On Mon, 20 Nov 2023 17:50:53 +0100 Christian Marangi wrote: > On Mon, Nov 20, 2023 at 09:42:34AM -0800, Jakub Kicinski wrote: > > On Mon, 20 Nov 2023 14:15:40 +0100 Christian Marangi wrote: > > > Luckly nothing was ever added before the soft_reset call so the ret > > > check (in the case where a PHY didn't had soft_reset defined) although > > > wrong, never caused problems as ret was init 0 at the start of > > > phy_init_hw. > > > > not currently a bug => no Fixes tag, please > > I know it's not a bug but still the referenced commit was wrong. Can I > at least use Ref to reference it? Not sure what you mean by "Ref" > Due to the changes done to this function, it's hard to catch where the > problem arised with a git blame. Right, and you already quote the commit in the body. No objections to repeating that if you want, maybe: Introduced by commit 6e2d85ec0559 ("net: phy: Stop with excessive soft reset"). but as a part of the "body" of the commit message, not tags.
On Mon, Nov 20, 2023 at 05:49:28PM +0100, Christian Marangi wrote: > On Mon, Nov 20, 2023 at 06:15:43PM +0100, Larysa Zaremba wrote: > > On Mon, Nov 20, 2023 at 02:15:40PM +0100, Christian Marangi wrote: > > > soft_reset call for phy_init_hw had multiple revision across the years > > > and the implementation goes back to 2014. Originally was a simple call > > > to write the generic PHY reset BIT, it was then moved to a dedicated > > > function. It was then added the option for PHY driver to define their > > > own special way to reset the PHY. Till this change, checking for ret was > > > correct as it was always filled by either the generic reset or the > > > custom implementation. This changed tho with commit 6e2d85ec0559 ("net: > > > phy: Stop with excessive soft reset"), as the generic reset call to PHY > > > was dropped but the ret check was never made entirely optional and > > > dependent whether soft_reset was defined for the PHY driver or not. > > > > > > Luckly nothing was ever added before the soft_reset call so the ret > > > check (in the case where a PHY didn't had soft_reset defined) although > > > wrong, never caused problems as ret was init 0 at the start of > > > phy_init_hw. > > > > > > To prevent any kind of problem and to make the function cleaner and more > > > robust, correctly move the ret check if the soft_reset section making it > > > optional and needed only with the function defined. > > > > > > Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > --- > > > drivers/net/phy/phy_device.c | 9 ++++----- > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > > index 2ce74593d6e4..478126f6b5bc 100644 > > > --- a/drivers/net/phy/phy_device.c > > > +++ b/drivers/net/phy/phy_device.c > > > @@ -1235,14 +1235,13 @@ int phy_init_hw(struct phy_device *phydev) > > > > > > if (phydev->drv->soft_reset) { > > > ret = phydev->drv->soft_reset(phydev); > > > + if (ret < 0) > > > + return ret; > > > + > > > /* see comment in genphy_soft_reset for an explanation */ > > > - if (!ret) > > > - phydev->suspended = 0; > > > + phydev->suspended = 0; > > > > Are you sure 'suspended' should not be cleared, if soft_reset fails? > > > > > } > > Idea is not change current implementation... And before this, suspended > was cleared only with reset not failing. > Ah, OK. I should not do reviews that late. If the change is purely cosmetic though, I think Fixes tag does not apply here. Other than that: Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> But I am also not certain, if maintainers would take such small of a change. > > > > > > - if (ret < 0) > > > - return ret; > > > - > > > ret = phy_scan_fixups(phydev); > > > if (ret < 0) > > > return ret; > > > -- > > > 2.40.1 > > > > > > > > -- > Ansuel >
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 2ce74593d6e4..478126f6b5bc 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1235,14 +1235,13 @@ int phy_init_hw(struct phy_device *phydev) if (phydev->drv->soft_reset) { ret = phydev->drv->soft_reset(phydev); + if (ret < 0) + return ret; + /* see comment in genphy_soft_reset for an explanation */ - if (!ret) - phydev->suspended = 0; + phydev->suspended = 0; } - if (ret < 0) - return ret; - ret = phy_scan_fixups(phydev); if (ret < 0) return ret;