[PATCH net-next 1/1] net: stmmac: add check for advertising linkmode request for set-eee

[Gan, Yi Fang]

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 at armlinux.org.uk>
> Sent: Friday, October 27, 2023 3:39 PM
> To: Gan, Yi Fang <yi.fang.gan at intel.com>
> Cc: Alexandre Torgue <alexandre.torgue at foss.st.com>; Jose Abreu
> <joabreu at synopsys.com>; David S . Miller <davem at davemloft.net>; Eric
> Dumazet <edumazet at google.com>; Jakub Kicinski <kuba at kernel.org>; Paolo
> Abeni <pabeni at redhat.com>; Maxime Coquelin
> <mcoquelin.stm32 at gmail.com>; netdev at vger.kernel.org; linux-stm32 at st-
> md-mailman.stormreply.com; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; Looi, Hong Aun <hong.aun.looi at intel.com>; Voon,
> Weifeng <weifeng.voon at intel.com>; Song, Yoong Siang
> <yoong.siang.song at intel.com>; Ahmad Tarmizi, Noor Azura
> <noor.azura.ahmad.tarmizi at 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 at 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!