* [PATCH net] gianfar: Check if phydev present on ethtool -A @ 2014-04-23 13:38 Claudiu Manoil 2014-04-23 16:21 ` Ben Hutchings 2014-04-24 17:36 ` David Miller 0 siblings, 2 replies; 5+ messages in thread From: Claudiu Manoil @ 2014-04-23 13:38 UTC (permalink / raw) To: netdev; +Cc: David S. Miller 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(+) 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))) -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] gianfar: Check if phydev present on ethtool -A 2014-04-23 13:38 [PATCH net] gianfar: Check if phydev present on ethtool -A Claudiu Manoil @ 2014-04-23 16:21 ` Ben Hutchings 2014-04-24 8:04 ` Claudiu Manoil 2014-04-24 17:36 ` David Miller 1 sibling, 1 reply; 5+ messages in thread From: Ben Hutchings @ 2014-04-23 16:21 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, David S. Miller [-- Attachment #1: Type: text/plain, Size: 1405 bytes --] 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. -- Ben Hutchings Beware of programmers who carry screwdrivers. - Leonard Brandwein [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 811 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] gianfar: Check if phydev present on ethtool -A 2014-04-23 16:21 ` Ben Hutchings @ 2014-04-24 8:04 ` Claudiu Manoil 2014-04-26 23:18 ` Ben Hutchings 0 siblings, 1 reply; 5+ messages in thread From: Claudiu Manoil @ 2014-04-24 8:04 UTC (permalink / raw) To: Ben Hutchings; +Cc: netdev, David S. Miller 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. > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] gianfar: Check if phydev present on ethtool -A 2014-04-24 8:04 ` Claudiu Manoil @ 2014-04-26 23:18 ` Ben Hutchings 0 siblings, 0 replies; 5+ messages in thread From: Ben Hutchings @ 2014-04-26 23:18 UTC (permalink / raw) To: Claudiu Manoil; +Cc: netdev, David S. Miller [-- Attachment #1: Type: text/plain, Size: 2712 bytes --] 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. -- Ben Hutchings Power corrupts. Absolute power is kind of neat. - John Lehman, Secretary of the US Navy 1981-1987 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 811 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] gianfar: Check if phydev present on ethtool -A 2014-04-23 13:38 [PATCH net] gianfar: Check if phydev present on ethtool -A Claudiu Manoil 2014-04-23 16:21 ` Ben Hutchings @ 2014-04-24 17:36 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2014-04-24 17:36 UTC (permalink / raw) To: claudiu.manoil; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-04-26 23:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-23 13:38 [PATCH net] gianfar: Check if phydev present on ethtool -A Claudiu Manoil 2014-04-23 16:21 ` Ben Hutchings 2014-04-24 8:04 ` Claudiu Manoil 2014-04-26 23:18 ` Ben Hutchings 2014-04-24 17:36 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).