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 <redacted>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
[off-list ref]; David S . Miller [off-list ref]; Eric
Dumazet [off-list ref]; Jakub Kicinski [off-list ref]; Paolo
Abeni [off-list ref]; Maxime Coquelin
[off-list ref]; 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 [off-list ref]; Voon,
Weifeng [off-list ref]; Song, Yoong Siang
[off-list ref]; Ahmad Tarmizi, Noor Azura
[off-list ref]
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:

quoted

From: Noor Azura Ahmad Tarmizi <redacted>

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!