Re: [PATCH] gianfar: Add gfar_change_carrier()
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()
> 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()
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 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 > > 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 0xdc20-dc22 > > # > 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 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 > > 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 0xdc20-dc22 > > still RUNNING .. > > > > #> ip link show eth0 > > 5: eth0: 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: mtu 1500 qdisc > > fq_codel state DORMANT mode DORMANT group default qlen 1000 > > # > ifconfig eth0 > > eth0: flags=4099 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()
> 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 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 > 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 0xdc20-dc22 > # > 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 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 > 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 0xdc20-dc22 > still RUNNING .. > > #> ip link show eth0 > 5: eth0: 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: mtu 1500 qdisc fq_codel > state DORMANT mode DORMANT group default qlen 1000 > # > ifconfig eth0 > eth0: flags=4099 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()
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 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 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 0xdc20-dc22 # > 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 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 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 0xdc20-dc22 still RUNNING .. #> ip link show eth0 5: eth0: 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: mtu 1500 qdisc fq_codel state DORMANT mode DORMANT group default qlen 1000 # > ifconfig eth0 eth0: flags=4099 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()
> 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()
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()
> 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()
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()
> > 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()
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()
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
[PATCH] gianfar: Add gfar_change_carrier()
This allows to control carrier from /sys/class/net/ethX/carrier
Signed-off-by: Joakim Tjernlund
---
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
