| Message ID | 1394413270-7169-1-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2014-03-10 at 02:01 +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. I think it's the caller's responsibility to zero out struct ethtool_wolinfo. That is what ethtool_get_wol() does. Maybe you could split ethtool_get_wol() like we did ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL? Ben. > Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > --- > Cc: David Miller <davem@davemloft.net> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: netdev@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/net/phy/phy.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 19c9eca0ef26..62a7cd401e1c 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -1092,6 +1092,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol); > > void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > { > + wol->supported = wol->wolopts = 0; > if (phydev->drv->get_wol) > phydev->drv->get_wol(phydev, wol); > }
On 03/10/2014 02:51 AM, Ben Hutchings wrote: > On Mon, 2014-03-10 at 02:01 +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. > > I think it's the caller's responsibility to zero out struct > ethtool_wolinfo. That is what ethtool_get_wol() does. Actually, phy_ethtool_get_wol is the caller here. This belongs to a set of helpers that deal with phy_device, not netdev. > Maybe you could split ethtool_get_wol() like we did > ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL? Looking at the other users of phy_ethtool_get_wol (mv643xx_eth and cpsw), both drivers use this helper to determine what to pass back on the corresponding ethtool_get_wol call. BTW, both drivers above do zero ethtool_wolinfo before calling phy_ethtool_get_wol. I can either zero it in phy_suspend too or we deal with it properly in phy_ethtool_get_wol instead: void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) { memset(wol, 0, sizeof(*wol)); if (phydev && phydev->drv->get_wol) phydev->drv->get_wol(phydev, wol); } That would also simplify above drivers down to e.g: static void cpsw_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol) { struct cpsw_priv *priv = netdev_priv(ndev); int slave_no = cpsw_slave_index(priv); phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol); } instead of: static void cpsw_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol) { struct cpsw_priv *priv = netdev_priv(ndev); int slave_no = cpsw_slave_index(priv); wol->supported = 0; wol->wolopts = 0; if (priv->slaves[slave_no].phy) phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol); } >> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> >> --- >> Cc: David Miller <davem@davemloft.net> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: netdev@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/net/phy/phy.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 19c9eca0ef26..62a7cd401e1c 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -1092,6 +1092,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol); >> >> void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) >> { >> + wol->supported = wol->wolopts = 0; >> if (phydev->drv->get_wol) >> phydev->drv->get_wol(phydev, wol); >> } >
From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> Date: Mon, 10 Mar 2014 10:49:53 +0000 > void phy_ethtool_get_wol(struct phy_device *phydev, struct > ethtool_wolinfo *wol) > { > memset(wol, 0, sizeof(*wol)); > > if (phydev && phydev->drv->get_wol) > phydev->drv->get_wol(phydev, wol); > } > > That would also simplify above drivers down to e.g: > > static void cpsw_get_wol(struct net_device *ndev, struct > ethtool_wolinfo *wol) > { > struct cpsw_priv *priv = netdev_priv(ndev); > int slave_no = cpsw_slave_index(priv); > phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol); > } > > instead of: > > static void cpsw_get_wol(struct net_device *ndev, struct > ethtool_wolinfo *wol) > { > struct cpsw_priv *priv = netdev_priv(ndev); > int slave_no = cpsw_slave_index(priv); > > wol->supported = 0; > wol->wolopts = 0; > > if (priv->slaves[slave_no].phy) > phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol); > } Agreed, since phy_ethtool_get_wol() is the common routine used by the drivers, we should make it clear the structure.
David, this is a small patch set based on a single patch sent earlier [1] to fix uninitalized struct ethtool_wolinfo in phy_ethtool_get_wol. Compared to the single patch, this clears the whole struct ethtool_wolinfo passed to phy_ethtool_get_wol and adds a check for non-NULL struct phy_device. I also added two cleanup patches for current users of phy_ethtool_get_wol. Those two patches are also based on v3.14-rc1 but aren't really part of the fix. They can wait for v3.15 and I'll rebase on request. [1] https://lkml.org/lkml/2014/3/9/169 Sebastian Hesselbarth (3): net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol net: mv643xx_eth: simplify phy_ethtool_get_wol call net: cpsw: simplify phy_ethtool_get_wol call drivers/net/ethernet/marvell/mv643xx_eth.c | 5 +---- drivers/net/ethernet/ti/cpsw.c | 7 +------ drivers/net/phy/phy.c | 4 +++- 3 files changed, 5 insertions(+), 11 deletions(-) --- Cc: David Miller <davem@davemloft.net> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: netdev@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org
2014-03-10 13:50 GMT-07:00 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>: > David, > > this is a small patch set based on a single patch sent earlier [1] to > fix uninitalized struct ethtool_wolinfo in phy_ethtool_get_wol. > Compared to the single patch, this clears the whole struct > ethtool_wolinfo passed to phy_ethtool_get_wol and adds a check for > non-NULL struct phy_device. > > I also added two cleanup patches for current users of > phy_ethtool_get_wol. Those two patches are also based on v3.14-rc1 but > aren't really part of the fix. They can wait for v3.15 and I'll rebase > on request. Looks good to me, thanks Sebastian: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > [1] https://lkml.org/lkml/2014/3/9/169 > > Sebastian Hesselbarth (3): > net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol > net: mv643xx_eth: simplify phy_ethtool_get_wol call > net: cpsw: simplify phy_ethtool_get_wol call > > drivers/net/ethernet/marvell/mv643xx_eth.c | 5 +---- > drivers/net/ethernet/ti/cpsw.c | 7 +------ > drivers/net/phy/phy.c | 4 +++- > 3 files changed, 5 insertions(+), 11 deletions(-) > > --- > Cc: David Miller <davem@davemloft.net> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: netdev@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > -- > 1.8.5.3 >
On Mon, 2014-03-10 at 10:49 +0000, Sebastian Hesselbarth wrote: > On 03/10/2014 02:51 AM, Ben Hutchings wrote: > > On Mon, 2014-03-10 at 02:01 +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. > > > > I think it's the caller's responsibility to zero out struct > > ethtool_wolinfo. That is what ethtool_get_wol() does. > > Actually, phy_ethtool_get_wol is the caller here. This belongs to > a set of helpers that deal with phy_device, not netdev. Right, but ethtool_get_wol() is further up the stack and is responsible for initialising the struct to defaults. > > Maybe you could split ethtool_get_wol() like we did > > ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL? > > Looking at the other users of phy_ethtool_get_wol (mv643xx_eth and > cpsw), both drivers use this helper to determine what to pass back > on the corresponding ethtool_get_wol call. > > BTW, both drivers above do zero ethtool_wolinfo before calling > phy_ethtool_get_wol. I can either zero it in phy_suspend too or we > deal with it properly in phy_ethtool_get_wol instead: > > void phy_ethtool_get_wol(struct phy_device *phydev, struct > ethtool_wolinfo *wol) > { > memset(wol, 0, sizeof(*wol)); > > if (phydev && phydev->drv->get_wol) > phydev->drv->get_wol(phydev, wol); > } [...] This trashes wol->cmd. Don't do that. Ben.
On Mon, 2014-03-10 at 16:23 -0400, David Miller wrote: > From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Date: Mon, 10 Mar 2014 10:49:53 +0000 > > > void phy_ethtool_get_wol(struct phy_device *phydev, struct > > ethtool_wolinfo *wol) > > { > > memset(wol, 0, sizeof(*wol)); > > > > if (phydev && phydev->drv->get_wol) > > phydev->drv->get_wol(phydev, wol); > > } > > > > That would also simplify above drivers down to e.g: > > > > static void cpsw_get_wol(struct net_device *ndev, struct > > ethtool_wolinfo *wol) > > { > > struct cpsw_priv *priv = netdev_priv(ndev); > > int slave_no = cpsw_slave_index(priv); > > phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol); > > } > > > > instead of: > > > > static void cpsw_get_wol(struct net_device *ndev, struct > > ethtool_wolinfo *wol) > > { > > struct cpsw_priv *priv = netdev_priv(ndev); > > int slave_no = cpsw_slave_index(priv); > > > > wol->supported = 0; > > wol->wolopts = 0; > > > > if (priv->slaves[slave_no].phy) > > phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol); > > } > > Agreed, since phy_ethtool_get_wol() is the common routine used by the drivers, > we should make it clear the structure. No, ethtool_get_wol() is the common routine used by *all* the drivers and it initialises the struct properly. Ben.
On 03/11/2014 12:17 AM, Ben Hutchings wrote: > On Mon, 2014-03-10 at 10:49 +0000, Sebastian Hesselbarth wrote: >> On 03/10/2014 02:51 AM, Ben Hutchings wrote: >>> On Mon, 2014-03-10 at 02:01 +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. >>> >>> I think it's the caller's responsibility to zero out struct >>> ethtool_wolinfo. That is what ethtool_get_wol() does. >> >> Actually, phy_ethtool_get_wol is the caller here. This belongs to >> a set of helpers that deal with phy_device, not netdev. > > Right, but ethtool_get_wol() is further up the stack and is responsible > for initialising the struct to defaults. Ok, currently we have 3 users of phy_ethtool_get_wol(): - mv643xx_eth and cpsw use that in their .get_wol callback that is called on ethtool_get_wol(). - phy_suspend to determine if it is safe to suspend a PHY With phy_suspend, I could use a kernel-compatible __ethtool_get_wol() but that requires to have an .attached_dev as __ethtool_get_wol() takes netdev. This would limit phy_suspend to attached PHYs while even non-attached PHYs should be suspended. OTOH, if we don't want phy_ethtool_get_wol to clear out ethtool_wol and no dependency on .attached_dev, phy_suspend is the only place to properly initialize ethtool_wol on this level. >>> Maybe you could split ethtool_get_wol() like we did >>> ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL? >> >> Looking at the other users of phy_ethtool_get_wol (mv643xx_eth and >> cpsw), both drivers use this helper to determine what to pass back >> on the corresponding ethtool_get_wol call. >> >> BTW, both drivers above do zero ethtool_wolinfo before calling >> phy_ethtool_get_wol. I can either zero it in phy_suspend too or we >> deal with it properly in phy_ethtool_get_wol instead: >> >> void phy_ethtool_get_wol(struct phy_device *phydev, struct >> ethtool_wolinfo *wol) >> { >> memset(wol, 0, sizeof(*wol)); >> >> if (phydev && phydev->drv->get_wol) >> phydev->drv->get_wol(phydev, wol); >> } > [...] > > This trashes wol->cmd. Don't do that. You are right on this one, I'll missed it and will fix it up. Sebastian
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 19c9eca0ef26..62a7cd401e1c 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1092,6 +1092,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol); void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) { + wol->supported = wol->wolopts = 0; if (phydev->drv->get_wol) phydev->drv->get_wol(phydev, wol); }
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> --- Cc: David Miller <davem@davemloft.net> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: netdev@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- drivers/net/phy/phy.c | 1 + 1 file changed, 1 insertion(+)