net/dsa/user.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
The WOL configuration now checks if the DSA switch supports setting WOL
before attempting to apply settings via phylink. This prevents
unnecessary calls to phylink_ethtool_set_wol when WOL is not supported.
Signed-off-by: Mohammed Anees <pvmohammedanees2003@gmail.com>
---
net/dsa/user.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 74eda9b30608..c685ccea9ddf 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1217,10 +1217,12 @@ static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
struct dsa_switch *ds = dp->ds;
int ret = -EOPNOTSUPP;
- phylink_ethtool_set_wol(dp->pl, w);
-
- if (ds->ops->set_wol)
+ if (ds->ops->set_wol) {
ret = ds->ops->set_wol(ds, dp->index, w);
+ if (ret)
+ return ret;
+ phylink_ethtool_set_wol(dp->pl, w);
+ }
return ret;
}
--
2.46.0On Sat, Oct 05, 2024 at 03:32:06AM +0530, Mohammed Anees wrote:
> The WOL configuration now checks if the DSA switch supports setting WOL
> before attempting to apply settings via phylink. This prevents
> unnecessary calls to phylink_ethtool_set_wol when WOL is not supported.
The commit message should say why a change is being made. Why should
phylink_ethtool_set_wol() not be called? Why is it unnecassary? What
if the PHY supports WoL, and does not need any help from DSA?
Andrew
---
pw-bot: cr
In the original code, we initialize ret = -EOPNOTSUPP and then call
phylink_ethtool_set_wol(). If DSA supports WOL, we call set_wol().
However, we aren’t checking if phylink_ethtool_set_wol() succeeds,
so I assumed both functions should be called, and if either fails,
we return -EOPNOTSUPP.
static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
{
struct dsa_port *dp = dsa_user_to_port(dev);
struct dsa_switch *ds = dp->ds;
int ret = -EOPNOTSUPP;
phylink_ethtool_set_wol(dp->pl, w);
if (ds->ops->set_wol)
ret = ds->ops->set_wol(ds, dp->index, w);
return ret;
}
From your response, it seems either of the two function can handle setting
WOL, if so shouldn't we check the return value of phylink_ethtool_set_wol()
to ensure it succeeds?
Thanks!
On Sun, Oct 06, 2024 at 12:12:33AM +0530, Mohammed Anees wrote:
> In the original code, we initialize ret = -EOPNOTSUPP and then call
> phylink_ethtool_set_wol(). If DSA supports WOL, we call set_wol().
> However, we aren’t checking if phylink_ethtool_set_wol() succeeds,
> so I assumed both functions should be called, and if either fails,
> we return -EOPNOTSUPP.
>
>
> static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
> {
> struct dsa_port *dp = dsa_user_to_port(dev);
> struct dsa_switch *ds = dp->ds;
> int ret = -EOPNOTSUPP;
>
> phylink_ethtool_set_wol(dp->pl, w);
>
> if (ds->ops->set_wol)
> ret = ds->ops->set_wol(ds, dp->index, w);
>
> return ret;
> }
>
> >From your response, it seems either of the two function can handle setting
> WOL, if so shouldn't we check the return value of phylink_ethtool_set_wol()
> to ensure it succeeds?
It is actually a bit more subtle than that, and i think everything
gets it wrong. Yes, we should check the return code from
phylink_ethtool_set_wol. If it does not return an error, we are
done. If it returns an error other than EOPNOTSUPP, it should return
it. And in the case of EOPNOTSUPP we should try to see if DSA supports
the WoL mode. And this is probably an over simplification. ethtool man
page says:
wol p|u|m|b|a|g|s|f|d...
Sets Wake-on-LAN options. Not all devices support this. The
argument to this option is a string of characters specifying
which options to enable.
p Wake on PHY activity
u Wake on unicast messages
m Wake on multicast messages
b Wake on broadcast messages
a Wake on ARP
g Wake on MagicPacket™
s Enable SecureOn™ password for MagicPacket™
f Wake on filter(s)
d Disable (wake on nothing). This option
clears all previous options.
So userspace could say pumbagsf, with the PHY supporting pmub and the
MAC supporting agsf, and the two need to cooperate.
get_wol() needs to call both phylink_ethtool_get_wol() and dsa
get_wol, and combine the results.
Andrew
Considering the insight you've provided, I've written the code below
static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
{
struct dsa_port *dp = dsa_user_to_port(dev);
struct dsa_switch *ds = dp->ds;
int ret;
ret = phylink_ethtool_set_wol(dp->pl, w);
if (ret != -EOPNOTSUPP)
return ret;
if (ds->ops->set_wol)
ret = ds->ops->set_wol(ds, dp->index, w);
if (ret != -EOPNOTSUPP)
return ret;
return -EOPNOTSUPP;
}
Does this approach address the issue, or do you think it would
be better to pass a struct that explicitly contains only the
options supported by each function (phylink_ethtool_set_wol()
and ds->ops->set_wol())? That way we don't have to check for
-EOPNOTSUPP as we are giving valid options to each function.
On Sun, Oct 06, 2024 at 09:40:32PM +0530, Mohammed Anees wrote:
> Considering the insight you've provided, I've written the code below
>
> static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
> {
> struct dsa_port *dp = dsa_user_to_port(dev);
> struct dsa_switch *ds = dp->ds;
> int ret;
>
> ret = phylink_ethtool_set_wol(dp->pl, w);
>
> if (ret != -EOPNOTSUPP)
> return ret;
>
> if (ds->ops->set_wol)
> ret = ds->ops->set_wol(ds, dp->index, w);
> if (ret != -EOPNOTSUPP)
> return ret;
This can be simplified to just:
> if (ds->ops->set_wol)
> return ds->ops->set_wol(ds, dp->index, w);
>
> return -EOPNOTSUPP;
> }
Andrew
On Sun, Oct 06, 2024 at 09:57:26PM +0200, Andrew Lunn wrote:
> On Sun, Oct 06, 2024 at 09:40:32PM +0530, Mohammed Anees wrote:
> > Considering the insight you've provided, I've written the code below
> >
> > static int dsa_user_set_wol(struct net_device *dev, struct ethtool_wolinfo *w)
> > {
> > struct dsa_port *dp = dsa_user_to_port(dev);
> > struct dsa_switch *ds = dp->ds;
> > int ret;
> >
> > ret = phylink_ethtool_set_wol(dp->pl, w);
> >
> > if (ret != -EOPNOTSUPP)
> > return ret;
> >
> > if (ds->ops->set_wol)
> > ret = ds->ops->set_wol(ds, dp->index, w);
> > if (ret != -EOPNOTSUPP)
> > return ret;
>
> This can be simplified to just:
>
> > if (ds->ops->set_wol)
> > return ds->ops->set_wol(ds, dp->index, w);
> >
> > return -EOPNOTSUPP;
> > }
I don't think the above is correct. While the simplification is, the
overall logic is not.
Let's go back to what Andrew said in his previous reply:
"So userspace could say pumbagsf, with the PHY supporting pmub and the
MAC supporting agsf, and the two need to cooperate."
The above does not do this. Let's go back further:
phylink_ethtool_set_wol(dp->pl, w);
if (ds->ops->set_wol)
ret = ds->ops->set_wol(ds, dp->index, w);
The original code _does_ do this, allowing the PHY and MAC to set
their modes, although the return code is not correct.
I notice V2 of the patch has been posted - in my opinion prematurely
because there's clearly the discussion on the first version has not
reached a conclusion yet.
What I would propose is the following:
int phy_ret, mac_ret;
phy_ret = phylink_ethtool_set_wol(dp->pl, w);
if (phy_ret != 0 && phy_ret != -EOPNOTSUPP)
return phy_ret;
if (ds->ops->set_wol)
mac_ret = ds->ops->set_wol(ds, dp->index, w);
else
mac_ret = -EOPNOTSUPP;
if (mac_ret != 0 && mac_ret != -EOPNOTSUPP)
return mac_ret;
/* Combine the two return codes. If either returned zero,
* then we have been successful.
*/
if (phy_ret == 0 || mac_ret == 0)
return 0;
return -EOPNOTSUPP;
Which I think is the closest one can get to - there is the possibility
for phylink_ethtool_set_wol() to have modified the WoL state, but
ds->ops->set_wol() to fail with an error code, causing this to return
failure, but I don't see that as being avoidable without yet more
complexity.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Apologies for overlooking the previous code. Based on your
suggestion, I’ve refined the implementation. Below is the
final version, please let me know if this works, and I’ll
send a new patch.
int phy_ret, mac_ret = -EOPNOTSUPP;
phy_ret = phylink_ethtool_set_wol(dp->pl, w);
if (phy_ret != 0 && phy_ret != -EOPNOTSUPP)
return phy_ret;
if (ds->ops->set_wol) {
mac_ret = ds->ops->set_wol(ds, dp->index, w);
if (mac_ret != 0 && mac_ret != -EOPNOTSUPP)
return mac_ret;
}
// Return success if either PHY or MAC succeeded
if (phy_ret == 0 || mac_ret == 0)
return 0;
return -EOPNOTSUPP;
Thanks!
© 2016 - 2024 Red Hat, Inc.