| Message ID | 1398260327-23381-1-git-send-email-claudiu.manoil@freescale.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2014-04-23 at 16:38 +0300, Claudiu Manoil wrote: > This fixes a seg fault on 'ethtool -A' entry if the > interface is down. Obviously we need to have the > phy device initialized / "connected" (see of_phy_connect()) > to be able to advertise pause frame capabilities. Why is the phydev detached while the interface is down?! > Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58 > Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> > --- > drivers/net/ethernet/freescale/gianfar_ethtool.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c > index 891dbee..76d7070 100644 > --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c > +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c > @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev, > struct gfar __iomem *regs = priv->gfargrp[0].regs; > u32 oldadv, newadv; > > + if (!phydev) > + return -ENODEV; > + > if (!(phydev->supported & SUPPORTED_Pause) || > (!(phydev->supported & SUPPORTED_Asym_Pause) && > (epause->rx_pause != epause->tx_pause))) I think you can instead remove the following check, as pause support is a property of the MAC not the PHY. Ben.
On 4/23/2014 7:21 PM, Ben Hutchings wrote: > On Wed, 2014-04-23 at 16:38 +0300, Claudiu Manoil wrote: >> This fixes a seg fault on 'ethtool -A' entry if the >> interface is down. Obviously we need to have the >> phy device initialized / "connected" (see of_phy_connect()) >> to be able to advertise pause frame capabilities. > > Why is the phydev detached while the interface is down?! Hi Ben, Gianfar uses the phylib framework, and it's been always calling phy_connect() during net_device open and the complementary phy_disconnect() during net_device close (ndo_stop). I think this is the general recommendation on how the net_device driver should integrate with the phylib: i.e. "phy_disconnect - disable interrupts, stop state machine, and detach a PHY device" while the interface is down. Many other net_device drivers call phy_disconnect() on device close. > >> Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58 >> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> >> --- >> drivers/net/ethernet/freescale/gianfar_ethtool.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c >> index 891dbee..76d7070 100644 >> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c >> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c >> @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev, >> struct gfar __iomem *regs = priv->gfargrp[0].regs; >> u32 oldadv, newadv; >> >> + if (!phydev) >> + return -ENODEV; >> + >> if (!(phydev->supported & SUPPORTED_Pause) || >> (!(phydev->supported & SUPPORTED_Asym_Pause) && >> (epause->rx_pause != epause->tx_pause))) > > I think you can instead remove the following check, as pause support is > a property of the MAC not the PHY. > I wish it was as easy as that. But my understanding is that link partners need to negotiate their pause frame capabilities at PHY level. If a phy is not able to signal (advertise) to the link partner that the MAC supports pause frame handling then the flow control btw link partners won't work properly (at least this is my understanding from looking at other implementations, like tg3). If it weren't so then the phydev's pause support and advertising flags and mii_advertise_flowctrl()/ mii_resolve_flowctrl_fdx() would be useless, right? Thanks, Claudiu > Ben. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Claudiu Manoil <claudiu.manoil@freescale.com> Date: Wed, 23 Apr 2014 16:38:47 +0300 > This fixes a seg fault on 'ethtool -A' entry if the > interface is down. Obviously we need to have the > phy device initialized / "connected" (see of_phy_connect()) > to be able to advertise pause frame capabilities. > > Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58 > Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> Applied, thanks Claudiu. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-04-24 at 11:04 +0300, Claudiu Manoil wrote: > On 4/23/2014 7:21 PM, Ben Hutchings wrote: > > On Wed, 2014-04-23 at 16:38 +0300, Claudiu Manoil wrote: > >> This fixes a seg fault on 'ethtool -A' entry if the > >> interface is down. Obviously we need to have the > >> phy device initialized / "connected" (see of_phy_connect()) > >> to be able to advertise pause frame capabilities. > > > > Why is the phydev detached while the interface is down?! > > Hi Ben, > Gianfar uses the phylib framework, and it's been always calling > phy_connect() during net_device open and the complementary > phy_disconnect() during net_device close (ndo_stop). > I think this is the general recommendation on how the net_device > driver should integrate with the phylib: i.e. "phy_disconnect - disable > interrupts, stop state machine, and detach a PHY device" while the > interface is down. Many other net_device drivers call phy_disconnect() > on device close. OK, I never actually used phylib so this is just surprising to me. [...] > >> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c > >> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c > >> @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev, > >> struct gfar __iomem *regs = priv->gfargrp[0].regs; > >> u32 oldadv, newadv; > >> > >> + if (!phydev) > >> + return -ENODEV; > >> + > >> if (!(phydev->supported & SUPPORTED_Pause) || > >> (!(phydev->supported & SUPPORTED_Asym_Pause) && > >> (epause->rx_pause != epause->tx_pause))) > > > > I think you can instead remove the following check, as pause support is > > a property of the MAC not the PHY. > > > > I wish it was as easy as that. But my understanding is that link > partners need to negotiate their pause frame capabilities at PHY > level. If a phy is not able to signal (advertise) to the link > partner that the MAC supports pause frame handling then the flow > control btw link partners won't work properly (at least this is my > understanding from looking at other implementations, like tg3). This is true but I don't see any reason why an MDIO-manageable autonegotiating PHY would limit which pause flags you can set. > If it weren't so then the phydev's pause support and advertising > flags and mii_advertise_flowctrl()/ mii_resolve_flowctrl_fdx() > would be useless, right? The pause flags in phy_device::supported do seem to be useless. The advertising flags are not as they should be changeable through ethtool. Ben.
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c index 891dbee..76d7070 100644 --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c @@ -533,6 +533,9 @@ static int gfar_spauseparam(struct net_device *dev, struct gfar __iomem *regs = priv->gfargrp[0].regs; u32 oldadv, newadv; + if (!phydev) + return -ENODEV; + if (!(phydev->supported & SUPPORTED_Pause) || (!(phydev->supported & SUPPORTED_Asym_Pause) && (epause->rx_pause != epause->tx_pause)))
This fixes a seg fault on 'ethtool -A' entry if the interface is down. Obviously we need to have the phy device initialized / "connected" (see of_phy_connect()) to be able to advertise pause frame capabilities. Fixes: 23402bddf9e56eecb27bbd1e5467b3b79b3dbe58 Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com> --- drivers/net/ethernet/freescale/gianfar_ethtool.c | 3 +++ 1 file changed, 3 insertions(+)