netdev - Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 27 Apr 2014 00:18:38 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Claudiu Manoil <claudiu.manoil@...escale.com>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] gianfar: Check if phydev present on ethtool -A

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

Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)