netdev - Re: [PATCH] gianfar: Add gfar_change

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <3188a74edfe5111cfb3533de6818918206ff26bd.camel@infinera.com>
Date:   Mon, 10 Dec 2018 15:37:11 +0000
From:   Joakim Tjernlund <Joakim.Tjernlund@...inera.com>
To:     "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     "claudiu.manoil@....com" <claudiu.manoil@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] gianfar: Add gfar_change_carrier()

On Fri, 2018-12-07 at 12:40 -0800, Florian Fainelli wrote:
> 
> On 12/7/18 9:26 AM, Andrew Lunn wrote:
> > > Would you be happier if .ndo_change_carrier() only acted on Fixed PHYs?
> > 
> > I think it makes sense to allow a fixed phy carrier to be changed from
> > user space. However, i don't think you can easily plumb that to
> > .ndo_change_carrier(), since that is a MAC feature. You need to change
> > the fixed_phy_status to indicate the PHY has lost link, and then let
> > the usual mechanisms tell the MAC it is down and change the carrier
> > status.
> 
> Joakim, I still don't understand what did not work with:
> 
> - adding a ndo_change_carrier() interface which keeps a boolean flag
> whether the link was up or not
> 
> - register a fixed_link_update callback for your fixed PHY, which just
> propagates that flag back to the fixed PHY
> 
> and that should take care of having the carrier go down, as driven by
> the PHY state machine, for that fixed device.
> --
> Florian

Florian,I guess I did misunderstand you and I am still not sure. Consider
this pseudo impl., is that what you mean ? if so, is Andrew happy with this too?

--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -491,7 +491,28 @@ static int gfar_set_mac_addr(struct net_device *dev, void *p)
 
        return 0;
 }
+static int gfar_fixed_phy_link_update(struct net_device *dev,
+                                     struct fixed_phy_status *status)
+{
+       struct gfar_priviate *priv;
+
+       if (dev && dev->phydev && status) {
+               priv = netdev_priv(dev);
+               status->link = !priv->no_carrier;
+       }
+       return 0;
+}
+static int gfar_change_carrier(struct net_device *dev, bool new_carrier)
+{
+       struct phy_device *phydev = dev->phydev;
+       struct gfar_priviate *priv;
 
+       if (!phy_is_pseudo_fixed_link(phydev))
+               return -EINVAL;
+       priv = netdev_priv(dev);
+       priv->no_carrier = !new_carrier;
+       return 0;
+}
 static const struct net_device_ops gfar_netdev_ops = {
        .ndo_open = gfar_enet_open,
        .ndo_start_xmit = gfar_start_xmit,
@@ -502,6 +523,7 @@ static const struct net_device_ops gfar_netdev_ops = {
        .ndo_tx_timeout = gfar_timeout,
        .ndo_do_ioctl = gfar_ioctl,
        .ndo_get_stats = gfar_get_stats,
+       .ndo_change_carrier = gfar_change_carrier,
        .ndo_set_mac_address = gfar_set_mac_addr,
        .ndo_validate_addr = eth_validate_addr,
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -1807,6 +1829,10 @@ static int init_phy(struct net_device *dev)
                return -ENODEV;
        }
 
+       if (phy_is_pseudo_fixed_link(phydev))
+               fixed_phy_set_link_update(phydev,
+                                         gfar_fixed_phy_link_update);
+
        if (interface == PHY_INTERFACE_MODE_SGMII)
                gfar_configure_serdes(dev);