Message ID | 20231027065054.3808352-1-yi.fang.gan@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp408159vqb; Thu, 26 Oct 2023 23:54:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEWLpvEHdcyWciqL5KRkwAxtKL4OJH6CnTrGfPXWIgzVnx3OGa4RlvDvZbi62qZFzPOWpMa X-Received: by 2002:a81:9a82:0:b0:59e:8f6d:92e with SMTP id r124-20020a819a82000000b0059e8f6d092emr1502696ywg.49.1698389677295; Thu, 26 Oct 2023 23:54:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698389677; cv=none; d=google.com; s=arc-20160816; b=GNimHWQyjn0QJns6zweLMzbgmt26pQGeMkuLFW3AviF1NrYJP71CcBcmR4Uf3quY8+ kucQbf3pvpgLsSbAEbwaivd9GfKVpD8w59Xvcyn3+DTGg+/MaeU24e30gaMcxOASpURD 40pTcQWczcRmq1W7aFRrUa4vce/A3MzKypqmu+J/n5006+1V6JVfuvZD/aOR9ddMePRS YNNu9L5NHNPidhwrCyAQGih8aaKxDYBw3DAzbtvXrIyJ0lgxdm5e/oNfntgYB8wXaQY8 rlwboxUx3AxsBaXgdsJi+EmRkALOPQEx61n/k8S0wD8F0SdH+vr/WSqJg94llv+8FPUJ ExPQ== 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=sUQmlZkBpPAgHZivKc4H7oUZlaAH0nJqWxadM6h+oBI=; fh=CSdkpeQMZtYechySQp6aEMacGbd0Q3rYrIlDzoSws28=; b=ZSfuDvlt8tVfUOrucHLaORcb8Irsovx1lWiMC6uBy0Z0muh81FABZxuhHof7i5ug25 P8nqJTWleE1/M98J1A/ZNpQ+NCzdo89pd8MdUpTLfPEtyYsrDEyC+cQzn4A4o6dItMYq Fgjc8KG2Kp5COMZM0sA9xcHauaskVHfYwkWtzK7R0nP9HxUz1a1xQIV92mJ4C29qIAnL 2+y39jbfyMUuxOBvHi2OByW3IcZUhi0AfvxEv8gMLUPwh8CgYdw05qo5/fKvKnvYk7VU QF5KvNYYgM2MGbBz+aHES2aY1z0uh2n5PcXJjJF1Nlf9LsADlXr5ABOwxQMxXeiXSwW/ 6dOg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=S+iOBHWU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id i65-20020a0dc644000000b005a82a298f71si1416476ywd.564.2023.10.26.23.54.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Oct 2023 23:54:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=S+iOBHWU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 2619081EA728; Thu, 26 Oct 2023 23:54:35 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345285AbjJ0Gya (ORCPT <rfc822;aposhian.dev@gmail.com> + 26 others); Fri, 27 Oct 2023 02:54:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57600 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229712AbjJ0Gy2 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 27 Oct 2023 02:54:28 -0400 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.10]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B4298116; Thu, 26 Oct 2023 23:54:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698389667; x=1729925667; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=FLxMqcP+5XqmQmOKGaFAfXKYkByQwryiGge0XuweW0I=; b=S+iOBHWU7sLWy/h2VgIQH+9iZP+9j7oLhUVH41XnTFVrQIllWxQutJQk Fls8wDniKpSMzmfpp4AGgCR0hCaH6MQ8iIc+kvKuwSvgFeosaj4Y0z1qc 6CQYAnK3kO6RgJ9gi5Lv2DwztrYn1FyMGRc+yy1pJRDcCexcYc3mPjSyV 4kmboAXynFj2hH9DvG+KjCNi2IulzZPiNbm6Qq4jXz6Fin7xMVymudr35 kVxjs++M2k0Dq+Gc9ihrL58Dq1QFJXp/h17lPREbUD5K8EdwXzU4x3V3Q nH8IpCmYqUkp0dLiJy6Tq2VMBbJ7BoYO/aHUIN2HqW6qSAqTdUFfZ40Ou A==; X-IronPort-AV: E=McAfee;i="6600,9927,10875"; a="539357" X-IronPort-AV: E=Sophos;i="6.03,255,1694761200"; d="scan'208";a="539357" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orvoesa102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Oct 2023 23:54:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10875"; a="753016032" X-IronPort-AV: E=Sophos;i="6.03,255,1694761200"; d="scan'208";a="753016032" Received: from ssid-ilbpg3-teeminta.png.intel.com ([10.88.227.74]) by orsmga007.jf.intel.com with ESMTP; 26 Oct 2023 23:54:21 -0700 From: Gan Yi Fang <yi.fang.gan@intel.com> To: Alexandre Torgue <alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com>, "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Looi Hong Aun <hong.aun.looi@intel.com>, Voon Weifeng <weifeng.voon@intel.com>, Song Yoong Siang <yoong.siang.song@intel.com>, Ahmad Tarmizi Noor Azura <noor.azura.ahmad.tarmizi@intel.com>, Gan Yi Fang <yi.fang.gan@intel.com> Subject: [PATCH net-next 1/1] net: stmmac: add check for advertising linkmode request for set-eee Date: Fri, 27 Oct 2023 14:50:54 +0800 Message-Id: <20231027065054.3808352-1-yi.fang.gan@intel.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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 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]); Thu, 26 Oct 2023 23:54:35 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780890654148956243 X-GMAIL-MSGID: 1780890654148956243 |
Series |
[net-next,1/1] net: stmmac: add check for advertising linkmode request for set-eee
|
|
Commit Message
Gan, Yi Fang
Oct. 27, 2023, 6:50 a.m. UTC
From: Noor Azura Ahmad Tarmizi <noor.azura.ahmad.tarmizi@intel.com> Add check for advertising linkmode set request with what is currently being supported by PHY before configuring the EEE. Unsupported setting will be rejected and a message will be prompted. No checking is required while setting the EEE to off. Signed-off-by: Noor Azura Ahmad Tarmizi <noor.azura.ahmad.tarmizi@intel.com> Signed-off-by: Gan, Yi Fang <yi.fang.gan@intel.com> --- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
Comments
On Fri, Oct 27, 2023 at 02:50:54PM +0800, Gan Yi Fang wrote: > From: Noor Azura Ahmad Tarmizi <noor.azura.ahmad.tarmizi@intel.com> > > Add check for advertising linkmode set request with what is currently > being supported by PHY before configuring the EEE. Unsupported setting > will be rejected and a message will be prompted. No checking is > required while setting the EEE to off. Why should this functionality be specific to stmmac? Why do we need this? What is wrong with the checking and masking that phylib is doing? Why should we trust the value in edata->supported provided by the user? Sorry, but no. I see no reason that this should be done, especially not in the stmmac driver.
Hi Russell King, Why should this functionality be specific to stmmac? This functionality is not specific to stmmac but other drivers can have their own implementation. (e.g. https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qede/qede_ethtool.c#L1855) Why do we need this? Current implementation will not take any effect if user enters unsupported value but user might not aware. With this, an error will be prompted if unsupported value is given. What is wrong with the checking and masking that phylib is doing? Nothing wrong with the phylib but there is no error return back to ethtool commands if unsupported value is given. Why should we trust the value in edata->supported provided by the user? The edata->supported is getting from the current setting and the value is set upon bootup. Users are not allowed to change it. Sorry, but no. I see no reason that this should be done, especially not in the stmmac driver. I understand your reasoning. From your point of view, is this kind of error message/ error handling not needed? Best Regards, Fang > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Friday, October 27, 2023 3:39 PM > To: Gan, Yi Fang <yi.fang.gan@intel.com> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo > Abeni <pabeni@redhat.com>; Maxime Coquelin > <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st- > md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; Looi, Hong Aun <hong.aun.looi@intel.com>; Voon, > Weifeng <weifeng.voon@intel.com>; Song, Yoong Siang > <yoong.siang.song@intel.com>; Ahmad Tarmizi, Noor Azura > <noor.azura.ahmad.tarmizi@intel.com> > Subject: Re: [PATCH net-next 1/1] net: stmmac: add check for advertising > linkmode request for set-eee > > On Fri, Oct 27, 2023 at 02:50:54PM +0800, Gan Yi Fang wrote: > > From: Noor Azura Ahmad Tarmizi <noor.azura.ahmad.tarmizi@intel.com> > > > > Add check for advertising linkmode set request with what is currently > > being supported by PHY before configuring the EEE. Unsupported setting > > will be rejected and a message will be prompted. No checking is > > required while setting the EEE to off. > > Why should this functionality be specific to stmmac? > > Why do we need this? > > What is wrong with the checking and masking that phylib is doing? > > Why should we trust the value in edata->supported provided by the user? > > Sorry, but no. I see no reason that this should be done, especially not in the > stmmac driver. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Oct 31, 2023 at 08:44:23AM +0000, Gan, Yi Fang wrote: > Hi Russell King, > > > Why should this functionality be specific to stmmac? > This functionality is not specific to stmmac but other drivers can have their > own implementation. > (e.g. https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qede/qede_ethtool.c#L1855) This is probably wrong (see below.) > > > Why do we need this? > Current implementation will not take any effect if user enters unsupported value but user might > not aware. With this, an error will be prompted if unsupported value is given. Why can't the user read back what settings were actually set like the other ethtool APIs? This is how ETHTOOL_GLINKSETTINGS works. > > What is wrong with the checking and masking that phylib is doing? > Nothing wrong with the phylib but there is no error return back to ethtool commands > if unsupported value is given. Maybe because that is the correct implementation? > > Why should we trust the value in edata->supported provided by the user? > The edata->supported is getting from the current setting and the value is set upon bootup. > Users are not allowed to change it. "not allowed" but there is nothing that prevents it. So an easy way to bypass your check is: struct ethtool_eee eeecmd; eeecmd.cmd = ETHTOOL_GEEE; send_ioctl(..., &eeecmd); eeecmd.cmd = ETHTOOL_SEEE; eeecmd.supported = ~0; eeecmd.advertised = ~0; error = send_ioctl(..., &eeecmd); and that won't return any error. So your check is weak at best, and relies upon the user doing the right thing. > > Sorry, but no. I see no reason that this should be done, especially not in the stmmac driver. > I understand your reasoning. From your point of view, is this kind of error message/ error handling > not needed? It is not - ethtool APIs don't return errors if the advertise mask is larger than the supported mask - they merely limit to what is supported and set that. When subsequently querying the settings, they return what is actually set (so the advertise mask will always be a subset of the supported mask at that point.) So, if in userspace you really want to know if some modes were dropped, then you have to do a set-get-check sequence. Thanks.
> -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Tuesday, October 31, 2023 5:09 PM > To: Gan, Yi Fang <yi.fang.gan@intel.com> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu > <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo > Abeni <pabeni@redhat.com>; Maxime Coquelin > <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md- > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; Looi, Hong Aun <hong.aun.looi@intel.com>; Voon, > Weifeng <weifeng.voon@intel.com>; Song, Yoong Siang > <yoong.siang.song@intel.com>; Ahmad Tarmizi, Noor Azura > <noor.azura.ahmad.tarmizi@intel.com> > Subject: Re: [PATCH net-next 1/1] net: stmmac: add check for advertising > linkmode request for set-eee > > On Tue, Oct 31, 2023 at 08:44:23AM +0000, Gan, Yi Fang wrote: > > Hi Russell King, > > > > > Why should this functionality be specific to stmmac? > > This functionality is not specific to stmmac but other drivers can > > have their own implementation. > > (e.g. > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ql > > ogic/qede/qede_ethtool.c#L1855) > > This is probably wrong (see below.) > > > > > > Why do we need this? > > Current implementation will not take any effect if user enters > > unsupported value but user might not aware. With this, an error will be > prompted if unsupported value is given. > > Why can't the user read back what settings were actually set like the other > ethtool APIs? This is how ETHTOOL_GLINKSETTINGS works. > For current implementation, the behaviour of "fail to set" and "set successfully" are the same from user's perspective. But yes, user have responsibility to read back and check the setting. > > > What is wrong with the checking and masking that phylib is doing? > > Nothing wrong with the phylib but there is no error return back to > > ethtool commands if unsupported value is given. > > Maybe because that is the correct implementation? > Yes, I agree with this. > > > Why should we trust the value in edata->supported provided by the user? > > The edata->supported is getting from the current setting and the value is set > upon bootup. > > Users are not allowed to change it. > > "not allowed" but there is nothing that prevents it. So an easy way to bypass > your check is: > > struct ethtool_eee eeecmd; > > eeecmd.cmd = ETHTOOL_GEEE; > send_ioctl(..., &eeecmd); > > eeecmd.cmd = ETHTOOL_SEEE; > eeecmd.supported = ~0; > eeecmd.advertised = ~0; > error = send_ioctl(..., &eeecmd); > > and that won't return any error. So your check is weak at best, and relies upon > the user doing the right thing. > Thank you for the example. > > > Sorry, but no. I see no reason that this should be done, especially not in the > stmmac driver. > > I understand your reasoning. From your point of view, is this kind of > > error message/ error handling not needed? > > It is not - ethtool APIs don't return errors if the advertise mask is larger than the > supported mask - they merely limit to what is supported and set that. When > subsequently querying the settings, they return what is actually set (so the > advertise mask will always be a subset of the supported mask at that point.) > > So, if in userspace you really want to know if some modes were dropped, then > you have to do a set-get-check sequence. > Thank you for all the feedbacks and explanations. It was a great discussion. I understand your concerns and reasonings. Let's close the review for this patch. Thanks. Best Regards, Fang
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index f628411ae4ae..6c090d4b7117 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -867,8 +867,24 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev, netdev_warn(priv->dev, "Setting EEE tx-lpi is not supported\n"); - if (!edata->eee_enabled) + if (!edata->eee_enabled) { stmmac_disable_eee_mode(priv); + } else { + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported); + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertised); + + ethtool_convert_legacy_u32_to_link_mode(supported, + edata->supported); + ethtool_convert_legacy_u32_to_link_mode(advertised, + edata->advertised); + + /*Check if the advertise speed is supported.*/ + if (!bitmap_subset(advertised, + supported, + __ETHTOOL_LINK_MODE_MASK_NBITS)){ + return -EOPNOTSUPP; + } + } ret = phylink_ethtool_set_eee(priv->phylink, edata); if (ret)