| Message ID | 1394578975-12588-1-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2014-03-12 at 00:02 +0100, Sebastian Hesselbarth wrote: > phy_ethtool_get_wol is a helper to get current WOL settings from > a phy device. When using this helper on a PHY without .get_wol > callback, struct ethtool_wolinfo is never set-up correctly and > may contain misleading information about WOL status. > > To fix this, always zero relevant fields of struct ethtool_wolinfo > regardless of .get_wol callback availability. Sorry, I still disagree with this. You're trying to make phy_ethtool_get_wol() do two subtly different things: - Provide an implementation of ethtool_ops::get_wol, leaving the net driver only to look up phy_device - Provide a standalone function for executing ETHTOOL_GWOL on a phy_device You may notice that phy_suspend() already sets wol.cmd = ETHTOOL_GWOL. So it seems to me like it's taking responsibility for initialising the structure like ethtool_get_wol() does. The bug is then that phy_suspend() doesn't clear the rest of the structure. That is not the responsibility of phy_ethtool_get_wol(). Ben. > Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Changelog: > v1->v2: > - clear whole struct ethtool_wolinfo > - check for non-NULL phy_device > v2->v3: > - only clear ->supported and ->wolopts (Suggested by Ben Hutchings) > > Cc: David Miller <davem@davemloft.net> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Ben Hutchings <ben@decadent.org.uk> > Cc: netdev@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/net/phy/phy.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 19c9eca0ef26..94234a91a50f 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol); > > void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > { > - if (phydev->drv->get_wol) > + wol->supported = wol->wolopts = 0; > + > + if (phydev && phydev->drv->get_wol) > phydev->drv->get_wol(phydev, wol); > } > EXPORT_SYMBOL(phy_ethtool_get_wol);
On 03/13/2014 02:21 AM, Ben Hutchings wrote: > On Wed, 2014-03-12 at 00:02 +0100, Sebastian Hesselbarth wrote: >> phy_ethtool_get_wol is a helper to get current WOL settings from >> a phy device. When using this helper on a PHY without .get_wol >> callback, struct ethtool_wolinfo is never set-up correctly and >> may contain misleading information about WOL status. >> >> To fix this, always zero relevant fields of struct ethtool_wolinfo >> regardless of .get_wol callback availability. > > Sorry, I still disagree with this. Which is really fine with me. Thanks for constantly commenting this. > You're trying to make phy_ethtool_get_wol() do two subtly different > things: > - Provide an implementation of ethtool_ops::get_wol, leaving the net > driver only to look up phy_device > - Provide a standalone function for executing ETHTOOL_GWOL on a > phy_device > > You may notice that phy_suspend() already sets wol.cmd = ETHTOOL_GWOL. Yeah, which was added by me because I misinterpreted phy_ethtool_get_wol depending on it. It doesn't because we just "reuse" struct ethtool_wolinfo. > So it seems to me like it's taking responsibility for initialising the > structure like ethtool_get_wol() does. The bug is then that > phy_suspend() doesn't clear the rest of the structure. That is not the > responsibility of phy_ethtool_get_wol(). I agree that public ethtool_get_wol should clear the struct, but I am not so happy to have an in-kernel API depend on the caller to setup the struct. In any way, if there are strong reasons not to clear struct ethtool_wolinfo again in phy_ethtool_get_wol, I can properly clear it in phy_suspend instead. I don't have a strong opinion about it. Sebastian >> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Changelog: >> v1->v2: >> - clear whole struct ethtool_wolinfo >> - check for non-NULL phy_device >> v2->v3: >> - only clear ->supported and ->wolopts (Suggested by Ben Hutchings) >> >> Cc: David Miller <davem@davemloft.net> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Ben Hutchings <ben@decadent.org.uk> >> Cc: netdev@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/net/phy/phy.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 19c9eca0ef26..94234a91a50f 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol); >> >> void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) >> { >> - if (phydev->drv->get_wol) >> + wol->supported = wol->wolopts = 0; >> + >> + if (phydev && phydev->drv->get_wol) >> phydev->drv->get_wol(phydev, wol); >> } >> EXPORT_SYMBOL(phy_ethtool_get_wol); >
From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> Date: Wed, 12 Mar 2014 00:02:55 +0100 > phy_ethtool_get_wol is a helper to get current WOL settings from > a phy device. When using this helper on a PHY without .get_wol > callback, struct ethtool_wolinfo is never set-up correctly and > may contain misleading information about WOL status. > > To fix this, always zero relevant fields of struct ethtool_wolinfo > regardless of .get_wol callback availability. > > Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> I'm starting to see this situation more clearly now, especially with Ben's most recent commentary. The basic notion is that one must do ethtool ops are designed such that the top-level execution context in net/core/ethtool.c takes care of initializing the structure. In this case, we're referring specifically to ethtool_get_wol(), which runs any time ETHTOOL_GWOL is requested. Therefore no ethtool_ops->get_wol() implementation should duplicate this work, that goes for all of such cases which invoke the function we are talking about here, phy_ethtool_get_wol(). So the first change is definitely to remove: wol->supported = 0; wol->wolopts = 0; from: drivers/net/ethernet/marvell/mv643xx_eth.c:mv643xx_eth_get_wol() drivers/net/ethernet/ti/cpsw.c:cpsw_get_wol() Next, I think phy_suspend() must create the same environment the call sites above guarentee for phy_ethtool_get_wol(), namely by changing the declaration of 'wol' to: struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; which will cause the compiler to clear out the rest of the structure for us, as the same declaration does in ethtool_get_wol(). Finally, purge the spurious clears in phydev_ops->get_wol(), namely in at803x_get_wol() and m88e1318_get_wol(). So, to reiterate, OPS never have to be mindful of initializing the ethtool result with zeros. However, anyone who calls into OPS directly must provide said expected state. Are we all on the same page now?
On 03/13/2014 08:38 PM, David Miller wrote: > From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Date: Wed, 12 Mar 2014 00:02:55 +0100 > >> phy_ethtool_get_wol is a helper to get current WOL settings from >> a phy device. When using this helper on a PHY without .get_wol >> callback, struct ethtool_wolinfo is never set-up correctly and >> may contain misleading information about WOL status. >> >> To fix this, always zero relevant fields of struct ethtool_wolinfo >> regardless of .get_wol callback availability. >> >> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > I'm starting to see this situation more clearly now, especially with > Ben's most recent commentary. > > The basic notion is that one must do ethtool ops are designed such that > the top-level execution context in net/core/ethtool.c takes care of > initializing the structure. > > In this case, we're referring specifically to ethtool_get_wol(), which > runs any time ETHTOOL_GWOL is requested. > > Therefore no ethtool_ops->get_wol() implementation should duplicate > this work, that goes for all of such cases which invoke the function > we are talking about here, phy_ethtool_get_wol(). > > So the first change is definitely to remove: > > wol->supported = 0; > wol->wolopts = 0; > > from: > > drivers/net/ethernet/marvell/mv643xx_eth.c:mv643xx_eth_get_wol() > drivers/net/ethernet/ti/cpsw.c:cpsw_get_wol() > > Next, I think phy_suspend() must create the same environment the > call sites above guarentee for phy_ethtool_get_wol(), namely > by changing the declaration of 'wol' to: > > struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; > > which will cause the compiler to clear out the rest of the structure > for us, as the same declaration does in ethtool_get_wol(). > > Finally, purge the spurious clears in phydev_ops->get_wol(), namely > in at803x_get_wol() and m88e1318_get_wol(). > > So, to reiterate, OPS never have to be mindful of initializing the > ethtool result with zeros. However, anyone who calls into OPS > directly must provide said expected state. > > Are we all on the same page now? Yes, we are. I'll send a fix for phy_suspend in a minute - still based on v3.14-rc1 in case there will be another -rc. If not, I'll rebase that fix on net-next together with patches for the four offending drivers you named above. Sebastian
On 03/13/2014 08:38 PM, David Miller wrote: > From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Date: Wed, 12 Mar 2014 00:02:55 +0100 [...] >> To fix this, always zero relevant fields of struct ethtool_wolinfo >> regardless of .get_wol callback availability. [...] > I'm starting to see this situation more clearly now, especially with > Ben's most recent commentary. > > The basic notion is that one must do ethtool ops are designed such that > the top-level execution context in net/core/ethtool.c takes care of > initializing the structure. > > In this case, we're referring specifically to ethtool_get_wol(), which > runs any time ETHTOOL_GWOL is requested. > > Therefore no ethtool_ops->get_wol() implementation should duplicate > this work, that goes for all of such cases which invoke the function > we are talking about here, phy_ethtool_get_wol(). > > So the first change is definitely to remove: > > wol->supported = 0; > wol->wolopts = 0; > > from: > > drivers/net/ethernet/marvell/mv643xx_eth.c:mv643xx_eth_get_wol() > drivers/net/ethernet/ti/cpsw.c:cpsw_get_wol() > [...] > > Finally, purge the spurious clears in phydev_ops->get_wol(), namely > in at803x_get_wol() and m88e1318_get_wol(). David, I was preparing cleanups for mv643xx_eth, cpsw, at803x, and mv88e1318. Out of curiosity, I did a git grep "wol->" drivers/net/ | grep "= 0" | wc -l 29 and found some other "spurious clears" ;) I can go that road and remove/rework all those clears. Some are really easy, some would require some more rework (e.g. e1000). Of course, a lot of those drivers then will need a Tested-by, as I don't have the HW available. > So, to reiterate, OPS never have to be mindful of initializing the > ethtool result with zeros. However, anyone who calls into OPS > directly must provide said expected state. Sebastian
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 19c9eca0ef26..94234a91a50f 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol); void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) { - if (phydev->drv->get_wol) + wol->supported = wol->wolopts = 0; + + if (phydev && phydev->drv->get_wol) phydev->drv->get_wol(phydev, wol); } EXPORT_SYMBOL(phy_ethtool_get_wol);