Message ID | 1480339472-5823-2-git-send-email-allan.nielsen@microsemi.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, Nov 28, 2016 at 02:24:30PM +0100, Allan W. Nielsen wrote: > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> > > 3 types of PHY loopback are supported. > i.e. Near-End Loopback, Far-End Loopback and External Loopback. Hi Allan Is this integrated with ethtool --test? You only want the PHY to go into loopback mode when running ethtool --test external_lb or maybe ethtool --test offline. What i think should happen is that this tunable sets the mode the PHY will go into when loopback is enabled. It does not however enable loopback. It is running ethtool --test which actually enables the loopback, probably by setting BMCR_LOOPBACK. Once the test is finished, the bit is cleared and the PHY goes back into normal operation. Andrew
On 11/28/2016 06:14 AM, Andrew Lunn wrote: > On Mon, Nov 28, 2016 at 02:24:30PM +0100, Allan W. Nielsen wrote: >> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> >> >> 3 types of PHY loopback are supported. >> i.e. Near-End Loopback, Far-End Loopback and External Loopback. > > Hi Allan > > Is this integrated with ethtool --test? You only want the PHY to go > into loopback mode when running ethtool --test external_lb or maybe > ethtool --test offline. > > What i think should happen is that this tunable sets the mode the PHY > will go into when loopback is enabled. It does not however enable > loopback. It is running ethtool --test which actually enables the > loopback, probably by setting BMCR_LOOPBACK. Once the test is > finished, the bit is cleared and the PHY goes back into normal > operation. Agreed with Andrew, there needs to be a tight coupling between the existing ethtool test infrastructure, and how to perform PHY loopback testing. It makes sense for the PHY drivers to be able to implement specific loopback/test mode, but we need to clarify how these get called from ethtool --test for instance, and if the Ethernet MAC driver needs to do something specific as well.
Hi Andrew and Florian, On 28/11/16 15:14, Andrew Lunn wrote: > On Mon, Nov 28, 2016 at 02:24:30PM +0100, Allan W. Nielsen wrote: > > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> > > > > 3 types of PHY loopback are supported. > > i.e. Near-End Loopback, Far-End Loopback and External Loopback. > Is this integrated with ethtool --test? You only want the PHY to go > into loopback mode when running ethtool --test external_lb or maybe > ethtool --test offline. There are other use-cases for enabling PHY loopback: 1) If the PHY is connected to a switch then a loop-port is sometime used to "force/enable" one or more extra pass through the switch core. This "hack" can sometime be used to achieve new functionality with existing silicon. 2) Existing user-space application may expect to be able to do the testing on its own (generate/validate the test traffic). > What i think should happen is that this tunable sets the mode the > PHY will go into when loopback is enabled. It does not however > enable loopback. That does not make a lot of sense with the "FAR" loopback (it is looping received traffic back into the wire). > It is running ethtool --test which actually enables > the loopback, probably by setting BMCR_LOOPBACK. Once the test is > finished, the bit is cleared and the PHY goes back into normal > operation. We are always happy to integrate with any existing functionality, but as I understand "ethtool --test" then intention is to perform a test and then bring back the PHY in to a "normal" state (I may be wrong...). The idea with this patch is to allow configuring loopback more "permanently" (userspace can decide when to activate and when to de-activate). I should properly have made that clear in the cover letter. Please let me know what you think. /Allan
On Mon, Nov 28, 2016 at 08:23:06PM +0100, Allan W. Nielsen wrote: > Hi Andrew and Florian, > > On 28/11/16 15:14, Andrew Lunn wrote: > > On Mon, Nov 28, 2016 at 02:24:30PM +0100, Allan W. Nielsen wrote: > > > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> > > > > > > 3 types of PHY loopback are supported. > > > i.e. Near-End Loopback, Far-End Loopback and External Loopback. > > Is this integrated with ethtool --test? You only want the PHY to go > > into loopback mode when running ethtool --test external_lb or maybe > > ethtool --test offline. > There are other use-cases for enabling PHY loopback: > > 1) If the PHY is connected to a switch then a loop-port is sometime > used to "force/enable" one or more extra pass through the switch > core. This "hack" can sometime be used to achieve new functionality > with existing silicon. With Linux, switches are managed via switchdev, or DSA. You will have to teach this infrastructure that something really odd is going on with one of its ports before you do anything like this in the PHY layer. I suggest you leave this use case alone until somebody really-really wants it. From my knowledge of the Marvell DSA driver, this is not easy. > 2) Existing user-space application may expect to be able to do the > testing on its own (generate/validate the test traffic). Please can you reference some existing user-space application and the kernel API it uses to put the PHY into loopback mode? > We are always happy to integrate with any existing functionality, but > as I understand "ethtool --test" then intention is to perform a test > and then bring back the PHY in to a "normal" state (I may be > wrong...). Correct. > The idea with this patch is to allow configuring loopback more > "permanently" (userspace can decide when to activate and when to > de-activate). I should properly have made that clear in the cover > letter. Leaving it in loopback is a really bad idea. I've spent days once working out why an Ethernet did not work. Turned out the PHY powered up in loopback mode, and the embedded OS running on it did not initialise it to sensible defaults on probe. Packets we going out, dhcp server was replying but all incoming packets were discarded. It is really not obvious when everything looks O.K, but nothing works, because the PHY is in loopback. There needs to be a big red flag to warn you. If you really do what to do this, you should look at NETIF_F_LOOPBACK and consider how this concept can be applied at the PHY, not the MAC. But you need the full concept, so you see things like: 2: eth0: <PHY_LOOPBACK,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff Humm, i've no idea how you actually enable the MAC loopback NETIF_F_LOOPBACK represents. I don't see anything with ip link set. Andrew
On 11/28/2016 12:21 PM, Andrew Lunn wrote: > On Mon, Nov 28, 2016 at 08:23:06PM +0100, Allan W. Nielsen wrote: >> Hi Andrew and Florian, >> >> On 28/11/16 15:14, Andrew Lunn wrote: >>> On Mon, Nov 28, 2016 at 02:24:30PM +0100, Allan W. Nielsen wrote: >>>> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> >>>> >>>> 3 types of PHY loopback are supported. >>>> i.e. Near-End Loopback, Far-End Loopback and External Loopback. >>> Is this integrated with ethtool --test? You only want the PHY to go >>> into loopback mode when running ethtool --test external_lb or maybe >>> ethtool --test offline. >> There are other use-cases for enabling PHY loopback: >> >> 1) If the PHY is connected to a switch then a loop-port is sometime >> used to "force/enable" one or more extra pass through the switch >> core. This "hack" can sometime be used to achieve new functionality >> with existing silicon. > > With Linux, switches are managed via switchdev, or DSA. You will have > to teach this infrastructure that something really odd is going on > with one of its ports before you do anything like this in the PHY > layer. I suggest you leave this use case alone until somebody > really-really wants it. From my knowledge of the Marvell DSA driver, > this is not easy. Agree with Andrew here, this particular use case with switches does not need to be solved now, but if we imagine we need to support that, chances are that we will want the network device as a configuration entry point, more than the PHY device itself. > >> 2) Existing user-space application may expect to be able to do the >> testing on its own (generate/validate the test traffic). > > Please can you reference some existing user-space application and the > kernel API it uses to put the PHY into loopback mode? > >> We are always happy to integrate with any existing functionality, but >> as I understand "ethtool --test" then intention is to perform a test >> and then bring back the PHY in to a "normal" state (I may be >> wrong...). > > Correct. > >> The idea with this patch is to allow configuring loopback more >> "permanently" (userspace can decide when to activate and when to >> de-activate). I should properly have made that clear in the cover >> letter. > > Leaving it in loopback is a really bad idea. I've spent days once > working out why an Ethernet did not work. Turned out the PHY powered > up in loopback mode, and the embedded OS running on it did not > initialise it to sensible defaults on probe. Packets we going out, > dhcp server was replying but all incoming packets were discarded. > > It is really not obvious when everything looks O.K, but nothing works, > because the PHY is in loopback. There needs to be a big red flag to > warn you. > > If you really do what to do this, you should look at NETIF_F_LOOPBACK > and consider how this concept can be applied at the PHY, not the MAC. > But you need the full concept, so you see things like: > > 2: eth0: <PHY_LOOPBACK,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 > link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff > > Humm, i've no idea how you actually enable the MAC loopback > NETIF_F_LOOPBACK represents. I don't see anything with ip link set. I am afraid you lost me on this, NETIF_F_LOOPBACK is a netdev_features_t bit, so it tells ethtool that this is a potential feature to be turned on with ethtool -K <iface>. The semantics of this loopack feature are not defined AFAICT, but a reasonable behavior from the driver is to put itself in a mode where packets send by a socket-level application are looped through the Ethernet adapter itself. Whether this happens at the DMA level, the MII signals, or somewhere in the PHY, is driver specific unfortunately. Now, there is another way to toggle a loopback for a given Ethernet adapter which is to actually set IFF_LOOPBACK in dev->flags for this interface. Some drivers seem to be able to properly react to that as well, although I see no way this can be done looking at the iproute2 or ifconfig man pages..
> > If you really do what to do this, you should look at NETIF_F_LOOPBACK > > and consider how this concept can be applied at the PHY, not the MAC. > > But you need the full concept, so you see things like: > > > > 2: eth0: <LOOPBACK,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 > > link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff > > > > Humm, i've no idea how you actually enable the MAC loopback > > NETIF_F_LOOPBACK represents. I don't see anything with ip link set. > > I am afraid you lost me on this, NETIF_F_LOOPBACK is a netdev_features_t > bit, so it tells ethtool that this is a potential feature to be turned > on with ethtool -K <iface>. Yep, i'm talking rubbish after jumping to a wrong conclusion. > The semantics of this loopack feature are > not defined AFAICT, but a reasonable behavior from the driver is to put > itself in a mode where packets send by a socket-level application are > looped through the Ethernet adapter itself. Whether this happens at the > DMA level, the MII signals, or somewhere in the PHY, is driver specific > unfortunately. Yes, the interesting one might be forcedeth which writes to the PHY BMCR_LOOPBACK | BMCR_FULLDPLX | BMCR_SPEED1000; when the NETIF_F_LOOPBACK feature is set. Maybe this could be generalised and made available for all MACs which don't support MAC loopback? What needs considering is the correct duplex and speed. We need to ensure the MAC and PHY agree on this. > Now, there is another way to toggle a loopback for a given Ethernet > adapter which is to actually set IFF_LOOPBACK in dev->flags for this > interface. Some drivers seem to be able to properly react to that as > well, although I see no way this can be done looking at the iproute2 or > ifconfig man pages.. I prefer this one, since i expect this will set LOOPBACK in 2: eth0: <LOOPBACK,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 making it lot more obvious something funny is going on. Andrew
On 28/11/16 21:21, Andrew Lunn wrote: > On Mon, Nov 28, 2016 at 08:23:06PM +0100, Allan W. Nielsen wrote: > > On 28/11/16 15:14, Andrew Lunn wrote: > > > On Mon, Nov 28, 2016 at 02:24:30PM +0100, Allan W. Nielsen wrote: > > > > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> > > > > 3 types of PHY loopback are supported. > > > > i.e. Near-End Loopback, Far-End Loopback and External Loopback. > > > Is this integrated with ethtool --test? You only want the PHY to go > > > into loopback mode when running ethtool --test external_lb or maybe > > > ethtool --test offline. > > There are other use-cases for enabling PHY loopback: > > 1) If the PHY is connected to a switch then a loop-port is sometime > > used to "force/enable" one or more extra pass through the switch > > core. This "hack" can sometime be used to achieve new functionality > > with existing silicon. > With Linux, switches are managed via switchdev, or DSA. You will have > to teach this infrastructure that something really odd is going on > with one of its ports before you do anything like this in the PHY > layer. I suggest you leave this use case alone until somebody > really-really wants it. From my knowledge of the Marvell DSA driver, > this is not easy. > > 2) Existing user-space application may expect to be able to do the > > testing on its own (generate/validate the test traffic). > Please can you reference some existing user-space application and the > kernel API it uses to put the PHY into loopback mode? The application I had in mind, is the switch application that I'm normally working on (a product of MSCC). This application was originally written for eCos, but is now moved to Linux. The application is currently doing almost all drivers in user-space (reaching the HW through a UIO driver). This means that we have an existing set of APIs and associated applications which among many other things tests the PHYs using loopback facilities. There are also cases where the loopports are being used as I described earlier. We are looking at moving some of the drivers into the kernel, and that require us to find a solution to such issues (nothing have been decided, and nothing will be decided anytime soon). I also understand your point of view, you have presented pretty good arguments, and I do not expect this to change your view on this topic. On 29/11/16 01:46, Andrew Lunn wrote: > > > If you really do what to do this, you should look at NETIF_F_LOOPBACK > > > and consider how this concept can be applied at the PHY, not the MAC. > > > But you need the full concept, so you see things like: > > > > > > 2: eth0: <LOOPBACK,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 > > > link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff > > > > > > Humm, i've no idea how you actually enable the MAC loopback > > > NETIF_F_LOOPBACK represents. I don't see anything with ip link set. > > > > I am afraid you lost me on this, NETIF_F_LOOPBACK is a netdev_features_t > > bit, so it tells ethtool that this is a potential feature to be turned > > on with ethtool -K <iface>. > Yep, i'm talking rubbish after jumping to a wrong conclusion. > > The semantics of this loopack feature are > > not defined AFAICT, but a reasonable behavior from the driver is to put > > itself in a mode where packets send by a socket-level application are > > looped through the Ethernet adapter itself. Whether this happens at the > > DMA level, the MII signals, or somewhere in the PHY, is driver specific > > unfortunately. > > Yes, the interesting one might be forcedeth which writes to the PHY > BMCR_LOOPBACK | BMCR_FULLDPLX | BMCR_SPEED1000; > when the NETIF_F_LOOPBACK feature is set. > > Maybe this could be generalised and made available for all MACs which > don't support MAC loopback? > > What needs considering is the correct duplex and speed. We need to > ensure the MAC and PHY agree on this. > > > Now, there is another way to toggle a loopback for a given Ethernet > > adapter which is to actually set IFF_LOOPBACK in dev->flags for this > > interface. Some drivers seem to be able to properly react to that as > > well, although I see no way this can be done looking at the iproute2 or > > ifconfig man pages.. > > I prefer this one, since i expect this will set LOOPBACK in > > 2: eth0: <LOOPBACK,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP mode DEFAULT group default qlen 1000 > > making it lot more obvious something funny is going on. Raju and I will need to dive deeper into this to understand the details of what you are suggesting. But I think it points in a different direction... The approach you are describing is to use existing flags (which will make the loop-back state visible to the user, which is a good thing) to set the loopback state. Where/how the frames are looped depends on the drivers. The suggested tunable could be kept, but it will only allow the user to say, "if the frames are looped in the PHY, then this is how I want them to be looped". Also, it does not make a lot of sense for the "FAR" loopback patch (which I admit is a bit strange...). The facility we are seeking is much more specific: "Allow the user to bring the PHY in and out of specific loopback modes" assuming that the user have reasons to do so. I'm not sure if your main dislike with this feature is the lack of transparency/visibility to the end-user, or if is the concept of allowing the user to control where and how frames are looped (or both). Best regards Allan W. Nielsen
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index f0db778..59629f5 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -254,6 +254,7 @@ struct ethtool_tunable { enum phy_tunable_id { ETHTOOL_PHY_ID_UNSPEC, ETHTOOL_PHY_DOWNSHIFT, + ETHTOOL_PHY_LOOPBACK, /* * Add your fresh new phy tunable attribute above and remember to update * phy_tunable_strings[] in net/core/ethtool.c @@ -261,6 +262,13 @@ enum phy_tunable_id { __ETHTOOL_PHY_TUNABLE_COUNT, }; +enum phy_loopback_type { + ETHTOOL_PHY_LOOPBACK_DISABLE, + ETHTOOL_PHY_LOOPBACK_NEAR, + ETHTOOL_PHY_LOOPBACK_FAR, + ETHTOOL_PHY_LOOPBACK_EXTN +}; + /** * struct ethtool_regs - hardware register dump * @cmd: Command number = %ETHTOOL_GREGS