Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
From:   "Gan, Yi Fang" <yi.fang.gan@intel.com>
To:     Russell King <linux@armlinux.org.uk>
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" <netdev@vger.kernel.org>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.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
Thread-Topic: [PATCH net-next 1/1] net: stmmac: add check for advertising
 linkmode request for set-eee
Thread-Index: AQHaCKJwVwif6aPTx0uyiZuY8b+qrrBdP/QAgAZSMzCAABBMgIABUPlA
Date:   Wed, 1 Nov 2023 05:41:48 +0000
Message-ID: <DM6PR11MB330605F0FDCE81F8379ACFEBB9A7A@DM6PR11MB3306.namprd11.prod.outlook.com>
References: <20231027065054.3808352-1-yi.fang.gan@intel.com>
 <ZTtpBCZuB+bjVt9D@shell.armlinux.org.uk>
 <DM6PR11MB3306A3162F6A6086A4CBA049B9A0A@DM6PR11MB3306.namprd11.prod.outlook.com>
 <ZUDEIZwKMl88hGcX@shell.armlinux.org.uk>
In-Reply-To: <ZUDEIZwKMl88hGcX@shell.armlinux.org.uk>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Gy86kkRzuYym9LDP0Jv7dZ4vUWrfj0t+WzDMGyzLUCsCZMvBSPxChUkEL6Xu?=
 =?us-ascii?Q?Xbhe3bg0lKplxrlS4CqDXrMU8/suh2zh2IqVGE7vq/m3gvT5KIRjNXKfrgf6?=
 =?us-ascii?Q?LocFTVbhLaZu575kWJHM7WbretjA2p/S6GYeqhQAV0PSYi3eO9SlgH5dN2rI?=
 =?us-ascii?Q?B6PKUMvC3g7gBOC7IZzg08DV5wObrWturFTlIp2ad8dxNetrtbxPa9SjATjy?=
 =?us-ascii?Q?CNR1DrKsv6IfKPSXxws0ulaho4dvHj2wHClvrVsRwdjgv+lnAO8KTi5tUUI2?=
 =?us-ascii?Q?9StWmzi0oOoTDrlKOLhP5Fwl/7ii0eu0P2hYmw1Fr7FGpM4GjET3x0aXrDmS?=
 =?us-ascii?Q?r+qguIsuhtvTbQcaoZsvyE5wvnthSiIUl0v15wvQlGJ7KmxYmjh1vTuceMWf?=
 =?us-ascii?Q?sLb0rQR+CJwA+FPg/kcAh7HYiUhgWmxkn56E5oRFf02DFm6nqV5ZUDpH8You?=
 =?us-ascii?Q?BfliVczHvH4rDEIjHlXMX5TqfSELJ6etZLMFgohxLwJjmYsyFJCm9XvFD6+g?=
 =?us-ascii?Q?ZAaGrh0IM/NH1XT6E8ZuhPaSAWYLsc+MN5+EqT/pY5huA4SR0uXNEXnYXBmh?=
 =?us-ascii?Q?qrKprgoSOujVNCO6hS/R2HzSLasV4qH63SG9Flza0fbnssQXYGCYTCO5vicE?=
 =?us-ascii?Q?VoLcyJ6vOwd0nCfdSN14HMj2u+DRV+OLlFB3cj9vCqwoly3GMwZTsqAYwp+M?=
 =?us-ascii?Q?j1KPiuch5VMTr3KfqgYNZ9Kk8p3SNfYJPoNyuP8Wz4dwynRIb5BCrp/4ai+6?=
 =?us-ascii?Q?qzlDU+Ytlvb3jHpLBOrGI1EPvZf6gVcqOCBmvrHkOaN4kF2Pdi922D3bzAI0?=
 =?us-ascii?Q?jdxxND1KObOFkVpWDQckMF+tf0TUXSldy1k0fhfCfJxtJBM6+JI3c9Dcvexh?=
 =?us-ascii?Q?LJvg889lrtkJ/l03bc0WWtJpsNTn7zOdv0xmRZYh3Bb2z08hAZYMaz03+ASC?=
 =?us-ascii?Q?sQuyEqtJ5seOjevci3F4pYMP0dK8KnL2ZeKEkDmve0lwx0Od6LeSJK9sQrW4?=
 =?us-ascii?Q?gGUzPqAAvgC0keBOLQu1b6t7F6CMcMN5ozM3RqUbOn+wKxUtDjfoM4s4UJoP?=
 =?us-ascii?Q?z2hfkNvHUA6lplWkHXAmR35OGck6ChJVyZKuuwtGwQPTyFtpZlGR9POAJAIq?=
 =?us-ascii?Q?QLXeQXGSIkjVxvA2dTrTO/zUxyh05EuYThiGtafyljS59+ukB2fQVZgMNJiX?=
 =?us-ascii?Q?L8Q8Y63wQYpLDuVOWpGkbHcMZuHtUtrMCzCGaCSwUm6HoFQ6pzARHPJS73va?=
 =?us-ascii?Q?5lqS1TZOLkyVOOeqSF4M17raRvv+gv3aNyHc3qg0ONRqKTULsg4jB3/KOPnW?=
 =?us-ascii?Q?1nq8LVemlAizKpi3T//22BFCcqcYpos2KsJA8uAPFdQFOzlmJCHztlhmrD7L?=
 =?us-ascii?Q?mJDDiZ91zi3p2z0JrUH2sO9fJAc0ZWpQVlEilJEyh+FmG9ZoQm/PDIPfBYgG?=
 =?us-ascii?Q?lxTAEmzEA/hdYUeU2QIWITaRjrSyubvGVkdGEaN0nG4QbHw/ewUIrpIybxJg?=
 =?us-ascii?Q?pxy/nOHnCXtZD6a37rBwSNrsi1/ub8q3FGnA9F7WqdD4KDf+gH6KO5yRoUZR?=
 =?us-ascii?Q?VDBl2qRs4h7hpkhNUfmPtMpkrPmXieNdhu81F7zu?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3306.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 50a6fdc6-c216-4d35-8548-08dbda9d3dca
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Nov 2023 05:41:48.4189
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /SrLds/E9M5yZCP+/kWWAGiDu37dRv7BlLG/HKXQ6IY8dnqpOVF+G8RzibM+qr+x8UDeOK6hodLMA34scT1Iqg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6603
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.3 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,T_SCC_BODY_TEXT_LINE,
	URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.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 (groat.vger.email [0.0.0.0]); Tue, 31 Oct 2023 22:42:25 -0700 (PDT)


> -----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
>=20
> 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)
>=20
> This is probably wrong (see below.)
>=20
> >
> > > 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.
>=20
> Why can't the user read back what settings were actually set like the oth=
er
> ethtool APIs? This is how ETHTOOL_GLINKSETTINGS works.
>=20
For current implementation, the behaviour of "fail to set" and
"set successfully" are the same from user's perspective.=20
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.
>=20
> Maybe because that is the correct implementation?
>=20
Yes, I agree with this.

> > > Why should we trust the value in edata->supported provided by the use=
r?
> > The edata->supported is getting from the current setting and the value =
is set
> upon bootup.
> > Users are not allowed to change it.
>=20
> "not allowed" but there is nothing that prevents it. So an easy way to by=
pass
> your check is:
>=20
> 	struct ethtool_eee eeecmd;
>=20
> 	eeecmd.cmd =3D ETHTOOL_GEEE;
> 	send_ioctl(..., &eeecmd);
>=20
> 	eeecmd.cmd =3D ETHTOOL_SEEE;
> 	eeecmd.supported =3D ~0;
> 	eeecmd.advertised =3D ~0;
> 	error =3D send_ioctl(..., &eeecmd);
>=20
> and that won't return any error. So your check is weak at best, and relie=
s upon
> the user doing the right thing.
>=20
Thank you for the example.

> > > Sorry, but no. I see no reason that this should be done, especially n=
ot in the
> stmmac driver.
> > I understand your reasoning. From your point of view, is this kind of
> > error message/ error handling not needed?
>=20
> It is not - ethtool APIs don't return errors if the advertise mask is lar=
ger than the
> supported mask - they merely limit to what is supported and set that. Whe=
n
> 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 poin=
t.)
>=20
> So, if in userspace you really want to know if some modes were dropped, t=
hen
> 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.=20

Thanks.
Best Regards,
Fang