[PATCH] gianfar: Add gfar_change_carrier()

[PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

This allows to control carrier from /sys/class/net/ethX/carrier

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 63daae120b2d..bdf9c226f8ef 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -492,6 +492,16 @@ static int gfar_set_mac_addr(struct net_device *dev, void *p)
 	return 0;
 }
 
+static int gfar_change_carrier(struct net_device *dev, bool new_carrier)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (phydev && phydev->phy_link_change)
+		phydev->phy_link_change(phydev, new_carrier, 1);
+
+	return 0;
+}
+
 static const struct net_device_ops gfar_netdev_ops = {
 	.ndo_open = gfar_enet_open,
 	.ndo_start_xmit = gfar_start_xmit,
@@ -502,6 +512,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
-- 
2.18.1

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Andrew Lunn]

On Thu, Dec 06, 2018 at 04:31:25PM +0100, Joakim Tjernlund wrote:
> This allows to control carrier from /sys/class/net/ethX/carrier

Hi Joakim

Please could you explain the use case for this.

      Andrew

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

On Thu, 2018-12-06 at 16:47 +0100, Andrew Lunn wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Thu, Dec 06, 2018 at 04:31:25PM +0100, Joakim Tjernlund wrote:
> > This allows to control carrier from /sys/class/net/ethX/carrier
> 
> Hi Joakim
> 
> Please could you explain the use case for this.

I have an eth I/F connected to an internal (on board) switch which has an external port to a mgmt network.
Whenever the external link is broken I want inform linux IP stack that the link is down on the internal eth
I/F as well. The two interfaces are isolated from the rest of the switch ports using VLANs. 

 Jocke

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Andrew Lunn]

> > Hi Joakim
> > 
> > Please could you explain the use case for this.
> 
> I have an eth I/F connected to an internal (on board) switch which has an external port to a mgmt network.
> Whenever the external link is broken I want inform linux IP stack that the link is down on the internal eth
> I/F as well. The two interfaces are isolated from the rest of the switch ports using VLANs. 

Hi Jockim

What type of switch is it?

What i would expect is you use a switch driver, and that driver would
allow access to the switches PHYs. When the external port goes down,
the Linux interface for that port would go down. There is no need to
change the carrier on the master interface.

       Andrew

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

On Thu, 2018-12-06 at 17:21 +0100, Andrew Lunn wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> > > Hi Joakim
> > > 
> > > Please could you explain the use case for this.
> > 
> > I have an eth I/F connected to an internal (on board) switch which has an external port to a mgmt network.
> > Whenever the external link is broken I want inform linux IP stack that the link is down on the internal eth
> > I/F as well. The two interfaces are isolated from the rest of the switch ports using VLANs.
> 
> Hi Jockim
> 
> What type of switch is it?
> 
> What i would expect is you use a switch driver, and that driver would
> allow access to the switches PHYs. When the external port goes down,
> the Linux interface for that port would go down. There is no need to
> change the carrier on the master interface.

I wish I had a proper DSA/Switchdev driver in place but I don't :(
Adding one is not impossible but then a lot of our user space app needs fixing so all
in all it it a fairly big project.
Anyhow, these carrier additions should be fine I think?

 Jocke
 

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Andrew Lunn]

> I wish I had a proper DSA/Switchdev driver in place but I don't :(
> Adding one is not impossible but then a lot of our user space app needs fixing so all
> in all it it a fairly big project.
> Anyhow, these carrier additions should be fine I think?

I'm not too sure about that. You are potentially messing up the state
machine, and the MAC driver could be looking at phydev->link, which
says up, but the carrier is down.

https://www.kernel.org/doc/Documentation/networking/operstates.txt

Could you set the interface to dormant? That seems like a better fit
anyway:

IF_OPER_DORMANT (5):
 Interface is L1 up, but waiting for an external event, f.e. for a
 protocol to establish. (802.1X)

The interface does have L1 to the switch, but you are waiting for the
external interface to go up. You can set this from user space without
needing any kernel changes.

	 Andrew

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

On Thu, 2018-12-06 at 17:54 +0100, Andrew Lunn wrote:
> 
> > I wish I had a proper DSA/Switchdev driver in place but I don't :(
> > Adding one is not impossible but then a lot of our user space app needs fixing so all
> > in all it it a fairly big project.
> > Anyhow, these carrier additions should be fine I think?
> 
> I'm not too sure about that. You are potentially messing up the state
> machine, and the MAC driver could be looking at phydev->link, which
> says up, but the carrier is down.
> 
> https://www.kernel.org/doc/Documentation/networking/operstates.txt
> 
> Could you set the interface to dormant? That seems like a better fit
> anyway:
> 
> IF_OPER_DORMANT (5):
>  Interface is L1 up, but waiting for an external event, f.e. for a
>  protocol to establish. (802.1X)
> 
> The interface does have L1 to the switch, but you are waiting for the
> external interface to go up. You can set this from user space without
> needing any kernel changes.

I can have a look at using dormant, but what is change_carrier supposed to do if
not this?

I didn't find a tool for DORMANT, I guess i will have to write one myself(using SIOCGIFFLAGS, SIOCSIFFLAGS)?

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Andrew Lunn]

> I can have a look at using dormant, but what is change_carrier
> supposed to do if not this?

It is intended for interfaces which are stacked, like the team driver,
and for devices which don't have a phy, e.g. tun, and dummy.

> I didn't find a tool for DORMANT, I guess i will have to write one
> myself(using SIOCGIFFLAGS, SIOCSIFFLAGS)?

ip link should be able to set it.

Try ip link set mode dormant dev eth0

    Andrew

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

On Thu, 2018-12-06 at 20:43 +0100, Andrew Lunn wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> > I can have a look at using dormant, but what is change_carrier
> > supposed to do if not this?
> 
> It is intended for interfaces which are stacked, like the team driver,
> and for devices which don't have a phy, e.g. tun, and dummy.
> 
> > I didn't find a tool for DORMANT, I guess i will have to write one
> > myself(using SIOCGIFFLAGS, SIOCSIFFLAGS)?
> 
> ip link should be able to set it.
> 
> Try ip link set mode dormant dev eth0
> 
>     Andrew

Been a bit busy today but now I have played with dormant using ip link and got some odd results:
# > ifconfig eth0
eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 172.20.0.246  netmask 255.255.0.0  broadcast 172.20.255.255
        inet6 fe80::ad9c:b230:1da8:1821  prefixlen 64  scopeid 0x20<link>
        ether 8c:16:45:89:cf:c6  txqueuelen 1000  (Ethernet)
        RX packets 1848903  bytes 736764445 (702.6 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 627462  bytes 222453345 (212.1 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 16  memory 0xdc200000-dc220000  
# > ip link set mode dormant dev eth0
 ping sunet.se
PING sunet.se (192.36.171.231) 56(84) bytes of data.
64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.22 ms
64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.17 ms

# > ifconfig eth0
eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet 172.20.0.246  netmask 255.255.0.0  broadcast 172.20.255.255
        inet6 fe80::ad9c:b230:1da8:1821  prefixlen 64  scopeid 0x20<link>
        ether 8c:16:45:89:cf:c6  txqueuelen 1000  (Ethernet)
        RX packets 1905479  bytes 753549572 (718.6 MiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 648266  bytes 224421617 (214.0 MiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
        device interrupt 16  memory 0xdc200000-dc220000  
still RUNNING ..

#> ip link show eth0
5: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DORMANT group default qlen 

state is still UP ?

# > ip link set state dormant dev eth0
# > ip link show eth0
5: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state DORMANT mode DORMANT group default qlen 1000
# > ifconfig eth0
eth0: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
...

Now both state and not RUNNING :)
but ...
 # ping sunet.se
PING sunet.se (192.36.171.231) 56(84) bytes of data.
64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.43 ms
64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.31 ms

I can still ping though. Is this how it is supposed to work ? No sure how state and mode relate to each other either.

 Jocke

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Andrew Lunn]

> Been a bit busy today but now I have played with dormant using ip link and got some odd results:
> # > ifconfig eth0
> eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
>         inet 172.20.0.246  netmask 255.255.0.0  broadcast 172.20.255.255
>         inet6 fe80::ad9c:b230:1da8:1821  prefixlen 64  scopeid 0x20<link>
>         ether 8c:16:45:89:cf:c6  txqueuelen 1000  (Ethernet)
>         RX packets 1848903  bytes 736764445 (702.6 MiB)
>         RX errors 0  dropped 0  overruns 0  frame 0
>         TX packets 627462  bytes 222453345 (212.1 MiB)
>         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
>         device interrupt 16  memory 0xdc200000-dc220000  
> # > ip link set mode dormant dev eth0
>  ping sunet.se
> PING sunet.se (192.36.171.231) 56(84) bytes of data.
> 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.22 ms
> 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.17 ms
> 
> # > ifconfig eth0
> eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
>         inet 172.20.0.246  netmask 255.255.0.0  broadcast 172.20.255.255
>         inet6 fe80::ad9c:b230:1da8:1821  prefixlen 64  scopeid 0x20<link>
>         ether 8c:16:45:89:cf:c6  txqueuelen 1000  (Ethernet)
>         RX packets 1905479  bytes 753549572 (718.6 MiB)
>         RX errors 0  dropped 0  overruns 0  frame 0
>         TX packets 648266  bytes 224421617 (214.0 MiB)
>         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
>         device interrupt 16  memory 0xdc200000-dc220000  
> still RUNNING ..
> 
> #> ip link show eth0
> 5: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DORMANT group default qlen 
> 
> state is still UP ?
> 
> # > ip link set state dormant dev eth0
> # > ip link show eth0
> 5: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state DORMANT mode DORMANT group default qlen 1000
> # > ifconfig eth0
> eth0: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
> ...
> 
> Now both state and not RUNNING :)
> but ...
>  # ping sunet.se
> PING sunet.se (192.36.171.231) 56(84) bytes of data.
> 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.43 ms
> 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.31 ms
> 
> I can still ping though. Is this how it is supposed to work ? No sure how state and mode relate to each other either.

I don't actually know. I know some things are supposed to work while
dormant. You should be able to perform 802.1x negotiation, etc.

You might find the people on the wireless list know more. I think it
is used by wpa_supplicant and hostapd during device authentication.

   Andrew

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

On Fri, 2018-12-07 at 15:15 +0100, Andrew Lunn wrote:
> 
> 
> > Been a bit busy today but now I have played with dormant using ip link and got some odd results:
> > # > ifconfig eth0
> > eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
> >         inet 172.20.0.246  netmask 255.255.0.0  broadcast 172.20.255.255
> >         inet6 fe80::ad9c:b230:1da8:1821  prefixlen 64  scopeid 0x20<link>
> >         ether 8c:16:45:89:cf:c6  txqueuelen 1000  (Ethernet)
> >         RX packets 1848903  bytes 736764445 (702.6 MiB)
> >         RX errors 0  dropped 0  overruns 0  frame 0
> >         TX packets 627462  bytes 222453345 (212.1 MiB)
> >         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
> >         device interrupt 16  memory 0xdc200000-dc220000
> > # > ip link set mode dormant dev eth0
> >  ping sunet.se
> > PING sunet.se (192.36.171.231) 56(84) bytes of data.
> > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.22 ms
> > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.17 ms
> > 
> > # > ifconfig eth0
> > eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
> >         inet 172.20.0.246  netmask 255.255.0.0  broadcast 172.20.255.255
> >         inet6 fe80::ad9c:b230:1da8:1821  prefixlen 64  scopeid 0x20<link>
> >         ether 8c:16:45:89:cf:c6  txqueuelen 1000  (Ethernet)
> >         RX packets 1905479  bytes 753549572 (718.6 MiB)
> >         RX errors 0  dropped 0  overruns 0  frame 0
> >         TX packets 648266  bytes 224421617 (214.0 MiB)
> >         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
> >         device interrupt 16  memory 0xdc200000-dc220000
> > still RUNNING ..
> > 
> > #> ip link show eth0
> > 5: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DORMANT group default qlen
> > 
> > state is still UP ?
> > 
> > # > ip link set state dormant dev eth0
> > # > ip link show eth0
> > 5: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state DORMANT mode DORMANT group default qlen 1000
> > # > ifconfig eth0
> > eth0: flags=4099<UP,BROADCAST,MULTICAST>  mtu 1500
> > ...
> > 
> > Now both state and not RUNNING :)
> > but ...
> >  # ping sunet.se
> > PING sunet.se (192.36.171.231) 56(84) bytes of data.
> > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=1 ttl=54 time=2.43 ms
> > 64 bytes from webc.sunet.se (192.36.171.231): icmp_seq=2 ttl=54 time=2.31 ms
> > 
> > I can still ping though. Is this how it is supposed to work ? No sure how state and mode relate to each other either.
> 
> I don't actually know. I know some things are supposed to work while
> dormant. You should be able to perform 802.1x negotiation, etc.
> 
> You might find the people on the wireless list know more. I think it
> is used by wpa_supplicant and hostapd during device authentication.


I am not sure why .ndo_change_carrier() is a no go in real drivers. Consider PHY less (aka Fixed PHYs)
devices. Here it makes sense to be able to control carrier from /sys I think.

Would you be happier if .ndo_change_carrier() only acted on Fixed PHYs?
I could also rework ndo_change_carrier to only use netif_carrier_on/off like team and dummy do.

 Jocke

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Andrew Lunn]

> 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.

	Andrew

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Florian Fainelli]

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

Re: [PATCH] gianfar: Add gfar_change_carrier()

[Joakim Tjernlund]

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);