From: Noor Azura Ahmad Tarmizi <[email protected]>
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 <[email protected]>
Signed-off-by: Gan, Yi Fang <[email protected]>
---
.../ethernet/stmicro/stmmac/stmmac_ethtool.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
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)
--
2.34.1
On Fri, Oct 27, 2023 at 02:50:54PM +0800, Gan Yi Fang wrote:
> From: Noor Azura Ahmad Tarmizi <[email protected]>
>
> 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!
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 <[email protected]>
> Sent: Friday, October 27, 2023 3:39 PM
> To: Gan, Yi Fang <[email protected]>
> Cc: Alexandre Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; [email protected]; linux-stm32@st-
> md-mailman.stormreply.com; [email protected]; linux-
> [email protected]; Looi, Hong Aun <[email protected]>; Voon,
> Weifeng <[email protected]>; Song, Yoong Siang
> <[email protected]>; Ahmad Tarmizi, Noor Azura
> <[email protected]>
> 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 <[email protected]>
> >
> > 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> -----Original Message-----
> From: Russell King <[email protected]>
> Sent: Tuesday, October 31, 2023 5:09 PM
> To: Gan, Yi Fang <[email protected]>
> Cc: Alexandre Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; [email protected]; linux-stm32@st-md-
> mailman.stormreply.com; [email protected]; linux-
> [email protected]; Looi, Hong Aun <[email protected]>; Voon,
> Weifeng <[email protected]>; Song, Yoong Siang
> <[email protected]>; Ahmad Tarmizi, Noor Azura
> <[email protected]>
> 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