diff mbox series

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

Message ID 20231027065054.3808352-1-yi.fang.gan@intel.com (mailing list archive)
State New, archived
Headers show
Series [net-next,1/1] net: stmmac: add check for advertising linkmode request for set-eee | expand

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

Russell King (Oracle) Oct. 27, 2023, 7:38 a.m. UTC | #1 
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.
Gan, Yi Fang Oct. 31, 2023, 8:44 a.m. UTC | #2 
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!
Russell King (Oracle) Oct. 31, 2023, 9:08 a.m. UTC | #3 
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.
Gan, Yi Fang Nov. 1, 2023, 5:41 a.m. UTC | #4 
> -----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 mbox series

Patch

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)