| Message ID | pvmbpdtn8yh.fsf@chavey.mtv.corp.google.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: > From: Ranjit Manomohan <ranjitm@google.com> > Date: Wed, 7 Apr 2010 15:19:48 -0700 > > Add an ethtool option to use internal loopback mode for testing. > This feature is used for component and driver test coverage by putting > the device in hardware loopback mode and sending / receiving network > traffic from a user application to test the hardware and driver > transmit / receive paths. > > Signed-off-by: laurent chavey <chavey@google.com> > --- > include/linux/ethtool.h | 16 ++++++++++++++++ > net/core/ethtool.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 0 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index b33f316..df1dcc7 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -292,6 +292,18 @@ struct ethtool_stats { > __u64 data[0]; > }; > > +/* for setting the NIC into loopback mode */ > +struct ethtool_loopback { > + u32 cmd; /* ETHTOOL_SLOOPBACK */ > + u32 type; /* ethtool_loopback_type */ > +}; > + > +enum ethtool_loopback_type { > + ETH_MAC = 0x00000001, > + ETH_PHY_INT = 0x00000002, > + ETH_PHY_EXT = 0x00000004 > +}; [...] There are many different places you can loop back within a MAC or PHY, not to mention bypassing the MAC altogether. See drivers/net/sfc/mcdi_pcol.h, starting from the line '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those loopback modes on at least one board. Also are these supposed to be an enumeration or flags? In theory you could use wire-side and host-side loopback at the same time if they don't overlap, but it's probably too much trouble to bother with. Any other combination is meaningless. Ben.
On Thu, Apr 8, 2010 at 11:29 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: >> From: Ranjit Manomohan <ranjitm@google.com> >> Date: Wed, 7 Apr 2010 15:19:48 -0700 >> >> Add an ethtool option to use internal loopback mode for testing. >> This feature is used for component and driver test coverage by putting >> the device in hardware loopback mode and sending / receiving network >> traffic from a user application to test the hardware and driver >> transmit / receive paths. >> >> Signed-off-by: laurent chavey <chavey@google.com> >> --- >> include/linux/ethtool.h | 16 ++++++++++++++++ >> net/core/ethtool.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 56 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index b33f316..df1dcc7 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -292,6 +292,18 @@ struct ethtool_stats { >> __u64 data[0]; >> }; >> >> +/* for setting the NIC into loopback mode */ >> +struct ethtool_loopback { >> + u32 cmd; /* ETHTOOL_SLOOPBACK */ >> + u32 type; /* ethtool_loopback_type */ >> +}; >> + >> +enum ethtool_loopback_type { >> + ETH_MAC = 0x00000001, >> + ETH_PHY_INT = 0x00000002, >> + ETH_PHY_EXT = 0x00000004 >> +}; > [...] > > There are many different places you can loop back within a MAC or PHY, > not to mention bypassing the MAC altogether. See > drivers/net/sfc/mcdi_pcol.h, starting from the line > '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those > loopback modes on at least one board. > > Also are these supposed to be an enumeration or flags? In theory you those are enums that can be or together. > could use wire-side and host-side loopback at the same time if they > don't overlap, but it's probably too much trouble to bother with. Any > other combination is meaningless. > > Ben. > > -- > Ben Hutchings, Senior Software Engineer, Solarflare Communications > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-04-08 at 12:17 -0700, Laurent Chavey wrote: > On Thu, Apr 8, 2010 at 11:29 AM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: [...] > >> +enum ethtool_loopback_type { > >> + ETH_MAC = 0x00000001, > >> + ETH_PHY_INT = 0x00000002, > >> + ETH_PHY_EXT = 0x00000004 > >> +}; > > [...] > > > > There are many different places you can loop back within a MAC or PHY, > > not to mention bypassing the MAC altogether. See > > drivers/net/sfc/mcdi_pcol.h, starting from the line > > '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those > > loopback modes on at least one board. > > > > Also are these supposed to be an enumeration or flags? In theory you > those are enums that can be or together. I.e. they are flags. So how do you answer this: > > could use wire-side and host-side loopback at the same time if they > > don't overlap, but it's probably too much trouble to bother with. Any > > other combination is meaningless. Ben.
On 04/08/2010 02:29 PM, Ben Hutchings wrote: > On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: >> From: Ranjit Manomohan<ranjitm@google.com> >> Date: Wed, 7 Apr 2010 15:19:48 -0700 >> >> Add an ethtool option to use internal loopback mode for testing. >> This feature is used for component and driver test coverage by putting >> the device in hardware loopback mode and sending / receiving network >> traffic from a user application to test the hardware and driver >> transmit / receive paths. >> >> Signed-off-by: laurent chavey<chavey@google.com> >> --- >> include/linux/ethtool.h | 16 ++++++++++++++++ >> net/core/ethtool.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 56 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index b33f316..df1dcc7 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -292,6 +292,18 @@ struct ethtool_stats { >> __u64 data[0]; >> }; >> >> +/* for setting the NIC into loopback mode */ >> +struct ethtool_loopback { >> + u32 cmd; /* ETHTOOL_SLOOPBACK */ >> + u32 type; /* ethtool_loopback_type */ >> +}; >> + >> +enum ethtool_loopback_type { >> + ETH_MAC = 0x00000001, >> + ETH_PHY_INT = 0x00000002, >> + ETH_PHY_EXT = 0x00000004 >> +}; > [...] > > There are many different places you can loop back within a MAC or PHY, > not to mention bypassing the MAC altogether. See > drivers/net/sfc/mcdi_pcol.h, starting from the line > '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those > loopback modes on at least one board. > > Also are these supposed to be an enumeration or flags? In theory you > could use wire-side and host-side loopback at the same time if they > don't overlap, but it's probably too much trouble to bother with. Any > other combination is meaningless. Additionally, ethtool is intended as an interface that exports capabilities generally useful to users. Is this feature really something that users will make use of? It seems more in the realm of design validation The existing ETHTOOL_TEST ioctl was intended as a coarse-grained method of verifying that the NIC is working, because fine-grained tests are inevitably both NIC-specific, and too involved for all but the most sophisticated users (read: the designers who built the NIC). So, consider this a _weak_ objection. Sure we could implement this, but will it really be useful to most users? Jeff -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Additionally, ethtool is intended as an interface that exports > capabilities generally useful to users. Is this feature really > something that users will make use of? It seems more in the realm of > design validation In enterprise days gone by, performing loopback tests were a part of hardware problem diagnosis in the field - either by SEs or customers. rick jones -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 8, 2010 at 12:51 PM, Jeff Garzik <jeff@garzik.org> wrote: > On 04/08/2010 02:29 PM, Ben Hutchings wrote: >> >> On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: >>> >>> From: Ranjit Manomohan<ranjitm@google.com> >>> Date: Wed, 7 Apr 2010 15:19:48 -0700 >>> >>> Add an ethtool option to use internal loopback mode for testing. >>> This feature is used for component and driver test coverage by putting >>> the device in hardware loopback mode and sending / receiving network >>> traffic from a user application to test the hardware and driver >>> transmit / receive paths. >>> >>> Signed-off-by: laurent chavey<chavey@google.com> >>> --- >>> include/linux/ethtool.h | 16 ++++++++++++++++ >>> net/core/ethtool.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 56 insertions(+), 0 deletions(-) >>> >>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >>> index b33f316..df1dcc7 100644 >>> --- a/include/linux/ethtool.h >>> +++ b/include/linux/ethtool.h >>> @@ -292,6 +292,18 @@ struct ethtool_stats { >>> __u64 data[0]; >>> }; >>> >>> +/* for setting the NIC into loopback mode */ >>> +struct ethtool_loopback { >>> + u32 cmd; /* ETHTOOL_SLOOPBACK */ >>> + u32 type; /* ethtool_loopback_type */ >>> +}; >>> + >>> +enum ethtool_loopback_type { >>> + ETH_MAC = 0x00000001, >>> + ETH_PHY_INT = 0x00000002, >>> + ETH_PHY_EXT = 0x00000004 >>> +}; >> >> [...] >> >> There are many different places you can loop back within a MAC or PHY, >> not to mention bypassing the MAC altogether. See >> drivers/net/sfc/mcdi_pcol.h, starting from the line >> '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those >> loopback modes on at least one board. >> >> Also are these supposed to be an enumeration or flags? In theory you >> could use wire-side and host-side loopback at the same time if they >> don't overlap, but it's probably too much trouble to bother with. Any >> other combination is meaningless. > > > Additionally, ethtool is intended as an interface that exports capabilities > generally useful to users. Is this feature really something that users will > make use of? It seems more in the realm of design validation > > The existing ETHTOOL_TEST ioctl was intended as a coarse-grained method of > verifying that the NIC is working, because fine-grained tests are inevitably > both NIC-specific, and too involved for all but the most sophisticated users > (read: the designers who built the NIC). > > So, consider this a _weak_ objection. Sure we could implement this, but > will it really be useful to most users? we have some autotests that make use of this feature for e1000(e), forcedeth and tg3 that we would like to share with the community to do perf / functional tests regression. > > Jeff > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 8, 2010 at 12:35 PM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2010-04-08 at 12:17 -0700, Laurent Chavey wrote: >> On Thu, Apr 8, 2010 at 11:29 AM, Ben Hutchings >> <bhutchings@solarflare.com> wrote: >> > On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: > [...] >> >> +enum ethtool_loopback_type { >> >> + ETH_MAC = 0x00000001, >> >> + ETH_PHY_INT = 0x00000002, >> >> + ETH_PHY_EXT = 0x00000004 >> >> +}; >> > [...] >> > >> > There are many different places you can loop back within a MAC or PHY, >> > not to mention bypassing the MAC altogether. See >> > drivers/net/sfc/mcdi_pcol.h, starting from the line >> > '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those >> > loopback modes on at least one board. >> > >> > Also are these supposed to be an enumeration or flags? In theory you >> those are enums that can be or together. > > I.e. they are flags. So how do you answer this: > >> > could use wire-side and host-side loopback at the same time if they >> > don't overlap, but it's probably too much trouble to bother with. Any >> > other combination is meaningless. since the intent is to enable the sending and receiving of packets at the hw/driver interfaces, a simple loopback mode on/off is sufficient and the ethtool_loopback_type are not necessary. the implementor can choose how to implement the loopback. From drivers/net/sfc/mcdi_pcol.h it is clear that unless ethtool_loopback_type as defined are meaningless. > Ben. > > -- > Ben Hutchings, Senior Software Engineer, Solarflare Communications > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/08/2010 06:43 PM, Laurent Chavey wrote: > On Thu, Apr 8, 2010 at 12:35 PM, Ben Hutchings > <bhutchings@solarflare.com> wrote: >> On Thu, 2010-04-08 at 12:17 -0700, Laurent Chavey wrote: >>> On Thu, Apr 8, 2010 at 11:29 AM, Ben Hutchings >>> <bhutchings@solarflare.com> wrote: >>>> On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: >> [...] >>>>> +enum ethtool_loopback_type { >>>>> + ETH_MAC = 0x00000001, >>>>> + ETH_PHY_INT = 0x00000002, >>>>> + ETH_PHY_EXT = 0x00000004 >>>>> +}; >>>> [...] >>>> >>>> There are many different places you can loop back within a MAC or PHY, >>>> not to mention bypassing the MAC altogether. See >>>> drivers/net/sfc/mcdi_pcol.h, starting from the line >>>> '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those >>>> loopback modes on at least one board. >>>> >>>> Also are these supposed to be an enumeration or flags? In theory you >>> those are enums that can be or together. >> >> I.e. they are flags. So how do you answer this: >> >>>> could use wire-side and host-side loopback at the same time if they >>>> don't overlap, but it's probably too much trouble to bother with. Any >>>> other combination is meaningless. > since the intent is to enable the sending and receiving of packets at > the hw/driver interfaces, a simple loopback mode on/off is sufficient > and the ethtool_loopback_type are not necessary. the implementor can choose > how to implement the loopback. From drivers/net/sfc/mcdi_pcol.h it is clear > that unless ethtool_loopback_type as defined are meaningless. If an off/on switch is sufficient, the existing ethtool flags interface should work just fine. Jeff -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
isn't the existing ETHTOOL_TEST ioctl use for something like self test ? the intent of this patch is to enable a mode whereby one could run netperf / iperf and other application and have the packets sent and received by the driver. On Thu, Apr 8, 2010 at 5:46 PM, Jeff Garzik <jeff@garzik.org> wrote: > On 04/08/2010 06:43 PM, Laurent Chavey wrote: >> >> On Thu, Apr 8, 2010 at 12:35 PM, Ben Hutchings >> <bhutchings@solarflare.com> wrote: >>> >>> On Thu, 2010-04-08 at 12:17 -0700, Laurent Chavey wrote: >>>> >>>> On Thu, Apr 8, 2010 at 11:29 AM, Ben Hutchings >>>> <bhutchings@solarflare.com> wrote: >>>>> >>>>> On Thu, 2010-04-08 at 10:35 -0700, chavey@google.com wrote: >>> >>> [...] >>>>>> >>>>>> +enum ethtool_loopback_type { >>>>>> + ETH_MAC = 0x00000001, >>>>>> + ETH_PHY_INT = 0x00000002, >>>>>> + ETH_PHY_EXT = 0x00000004 >>>>>> +}; >>>>> >>>>> [...] >>>>> >>>>> There are many different places you can loop back within a MAC or PHY, >>>>> not to mention bypassing the MAC altogether. See >>>>> drivers/net/sfc/mcdi_pcol.h, starting from the line >>>>> '#define MC_CMD_LOOPBACK_NONE 0'. I believe we implement all of those >>>>> loopback modes on at least one board. >>>>> >>>>> Also are these supposed to be an enumeration or flags? In theory you >>>> >>>> those are enums that can be or together. >>> >>> I.e. they are flags. So how do you answer this: >>> >>>>> could use wire-side and host-side loopback at the same time if they >>>>> don't overlap, but it's probably too much trouble to bother with. Any >>>>> other combination is meaningless. >> >> since the intent is to enable the sending and receiving of packets at >> the hw/driver interfaces, a simple loopback mode on/off is sufficient >> and the ethtool_loopback_type are not necessary. the implementor can >> choose >> how to implement the loopback. From drivers/net/sfc/mcdi_pcol.h it is >> clear >> that unless ethtool_loopback_type as defined are meaningless. > > If an off/on switch is sufficient, the existing ethtool flags interface > should work just fine. > > Jeff > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Don't top-post. On Fri, 2010-04-09 at 09:43 -0700, Laurent Chavey wrote: > isn't the existing ETHTOOL_TEST ioctl use for something like self test ? > > the intent of this patch is to enable a mode whereby one could run > netperf / iperf and other application and have the packets sent and > received by the driver. [...] If you send to a local address, the traffic will be routed over the internal loopback interface. Applications will not use a network interface in loopback unless they override routing or send raw packets. netperf and iperf don't allow you todo that, so far as I'm ware. Also those applications are *performance* tests; they are not very useful for fault-finding. Ben.
On 04/09/2010 12:43 PM, Laurent Chavey wrote: > isn't the existing ETHTOOL_TEST ioctl use for something like self test ? > > the intent of this patch is to enable a mode whereby one could run > netperf / iperf and other application and have the packets sent and > received by the driver. I said "ethtool flags interface", which is ETHTOOL_[GS]FLAGS. ethtool private flags interface would also work, ETHTOOL_[GS]PFLAGS. Both are interfaces enabling user setting/clearing of 32 on/off switches (bits). Jeff -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 9, 2010 at 10:01 AM, Jeff Garzik <jeff@garzik.org> wrote: > On 04/09/2010 12:43 PM, Laurent Chavey wrote: >> >> isn't the existing ETHTOOL_TEST ioctl use for something like self test ? >> >> the intent of this patch is to enable a mode whereby one could run >> netperf / iperf and other application and have the packets sent and >> received by the driver. > > I said "ethtool flags interface", which is ETHTOOL_[GS]FLAGS. > > ethtool private flags interface would also work, ETHTOOL_[GS]PFLAGS. > > Both are interfaces enabling user setting/clearing of 32 on/off switches > (bits). agree, no need to patch > > Jeff > > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index b33f316..df1dcc7 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -292,6 +292,18 @@ struct ethtool_stats { __u64 data[0]; }; +/* for setting the NIC into loopback mode */ +struct ethtool_loopback { + u32 cmd; /* ETHTOOL_SLOOPBACK */ + u32 type; /* ethtool_loopback_type */ +}; + +enum ethtool_loopback_type { + ETH_MAC = 0x00000001, + ETH_PHY_INT = 0x00000002, + ETH_PHY_EXT = 0x00000004 +}; + struct ethtool_perm_addr { __u32 cmd; /* ETHTOOL_GPERMADDR */ __u32 size; @@ -566,6 +578,8 @@ struct ethtool_ops { int (*reset)(struct net_device *, u32 *); int (*set_rx_ntuple)(struct net_device *, struct ethtool_rx_ntuple *); int (*get_rx_ntuple)(struct net_device *, u32 stringset, void *); + int (*set_loopback)(struct net_device *, struct ethtool_loopback *); + int (*get_loopback)(struct net_device *, struct ethtool_loopback *); }; #endif /* __KERNEL__ */ @@ -627,6 +641,8 @@ struct ethtool_ops { #define ETHTOOL_SRXNTUPLE 0x00000035 /* Add an n-tuple filter to device */ #define ETHTOOL_GRXNTUPLE 0x00000036 /* Get n-tuple filters from device */ #define ETHTOOL_GSSET_INFO 0x00000037 /* Get string set info */ +#define ETHTOOL_SLOOPBACK 0x00000038 /* Set loopback mode on/off */ +#define ETHTOOL_GLOOPBACK 0x00000039 /* Get loopback mode setting */ /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET diff --git a/net/core/ethtool.c b/net/core/ethtool.c index f4cb6b6..89b4ecb 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1289,6 +1289,40 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, char return dev->ethtool_ops->flash_device(dev, &efl); } +static int ethtool_set_loopback(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_loopback lpbk; + const struct ethtool_ops *ops = dev->ethtool_ops; + + if (!ops->set_loopback) + return -EOPNOTSUPP; + + if (copy_from_user(&lpbk, useraddr, sizeof(lpbk))) + return -EFAULT; + + return ops->set_loopback(dev, &lpbk); +} + +static int ethtool_get_loopback(struct net_device *dev, void __user *useraddr) +{ + struct ethtool_loopback lpbk; + const struct ethtool_ops *ops = dev->ethtool_ops; + int err; + + if (!ops->get_loopback) + return -EOPNOTSUPP; + + err = ops->get_loopback(dev, &lpbk); + + if (err) + return err; + + if (copy_to_user(useraddr, &lpbk, sizeof(lpbk))) + return -EFAULT; + + return 0; +} + /* The main entry point in this file. Called from net/core/dev.c */ int dev_ethtool(struct net *net, struct ifreq *ifr) @@ -1518,6 +1552,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) case ETHTOOL_GSSET_INFO: rc = ethtool_get_sset_info(dev, useraddr); break; + case ETHTOOL_SLOOPBACK: + rc = ethtool_set_loopback(dev, useraddr); + break; + case ETHTOOL_GLOOPBACK: + rc = ethtool_get_loopback(dev, useraddr); + break; default: rc = -EOPNOTSUPP; }