[PATCH 0/2] Loopback

[PATCH 0/2] Loopback

[Mahesh Bandewar]

First patch is the repost of the earlier loopback patch. tg3 implementation / patch demonstrates one such usage.

Mahesh Bandewar (2):
  net: Allow ethtool to set interface in loopback mode.
  tg3: Add code to allow ethtool to enable/disable loopback.

 drivers/net/tg3.c         |   32 ++++++++++++++++++++++++++++++++
 include/linux/netdevice.h |    3 ++-
 net/core/ethtool.c        |    2 +-
 3 files changed, 35 insertions(+), 2 deletions(-)

-- 
1.7.3.1

[PATCH 1/2] net: Allow ethtool to set interface in loopback mode.

[Mahesh Bandewar]

This patch enables ethtool to set the loopback mode on a given interface.
By configuring the interface in loopback mode in conjunction with a policy
route / rule, a userland application can stress the egress / ingress path
exposing the flows of the change in progress and potentially help developer(s)
understand the impact of those changes without even sending a packet out
on the network.

Following set of commands illustrates one such example -
    a) ip -4 addr add 192.168.1.1/24 dev eth1
    b) ip -4 rule add from all iif eth1 lookup 250
    c) ip -4 route add local 0/0 dev lo proto kernel scope host table 250
    d) arp -Ds 192.168.1.100 eth1
    e) arp -Ds 192.168.1.200 eth1
    f) sysctl -w net.ipv4.ip_nonlocal_bind=1
    g) sysctl -w net.ipv4.conf.all.accept_local=1
    # Assuming that the machine has 8 cores
    h) taskset 000f netserver -L 192.168.1.200
    i) taskset 00f0 netperf -t TCP_CRR -L 192.168.1.100 -H 192.168.1.200 -l 30
---
 include/linux/netdevice.h |    3 ++-
 net/core/ethtool.c        |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e03af35..0e17c81 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1067,6 +1067,7 @@ struct net_device {
 #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
 #define NETIF_F_RXCSUM		(1 << 29) /* Receive checksumming offload */
 #define NETIF_F_NOCACHE_COPY	(1 << 30) /* Use no-cache copyfromuser */
+#define NETIF_F_LOOPBACK	(1 << 31) /* Enable loopback */
 
 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
@@ -1082,7 +1083,7 @@ struct net_device {
 	/* = all defined minus driver/device-class-related */
 #define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
 				  NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
-#define NETIF_F_ETHTOOL_BITS	(0x7f3fffff & ~NETIF_F_NEVER_CHANGE)
+#define NETIF_F_ETHTOOL_BITS	(0xff3fffff & ~NETIF_F_NEVER_CHANGE)
 
 	/* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index d8b1a8d..f26649d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -362,7 +362,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
 	/* NETIF_F_RXHASH */          "rx-hashing",
 	/* NETIF_F_RXCSUM */          "rx-checksum",
 	/* NETIF_F_NOCACHE_COPY */    "tx-nocache-copy"
-	"",
+	/* NETIF_F_LOOPBACK */        "loopback",
 };
 
 static int __ethtool_get_sset_count(struct net_device *dev, int sset)
-- 
1.7.3.1

[PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

---
 drivers/net/tg3.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index fa57e3d..208884d 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -6319,6 +6319,33 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
 	return features;
 }
 
+static int tg3_set_features(struct net_device *dev, u32 features)
+{
+	struct tg3 *tp = netdev_priv(dev);
+	u32 cur_mode = 0;
+	int err = 0;
+
+	spin_lock_bh(&tp->lock);
+	cur_mode = tr32(MAC_MODE);
+
+	if (features & NETIF_F_LOOPBACK) {
+		/* Enable internal MAC loopback mode */
+		tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);
+		netif_carrier_on(tp->dev);
+		netdev_info(dev, "Internal MAC loopback mode enabled.\n");
+	} else {
+		/* Disable internal MAC loopback mode */
+		tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK);
+		/* Force link status check */
+		if (netif_running(dev))
+			tg3_setup_phy(tp, 1);
+		netdev_info(dev, "Internal MAC loopback mode disabled.\n");
+	}
+	spin_unlock_bh(&tp->lock);
+
+	return err;
+}
+
 static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
 			       int new_mtu)
 {
@@ -15028,6 +15055,7 @@ static const struct net_device_ops tg3_netdev_ops = {
 	.ndo_tx_timeout		= tg3_tx_timeout,
 	.ndo_change_mtu		= tg3_change_mtu,
 	.ndo_fix_features	= tg3_fix_features,
+	.ndo_set_features	= tg3_set_features,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= tg3_poll_controller,
 #endif
@@ -15044,6 +15072,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
 	.ndo_do_ioctl		= tg3_ioctl,
 	.ndo_tx_timeout		= tg3_tx_timeout,
 	.ndo_change_mtu		= tg3_change_mtu,
+	.ndo_set_features	= tg3_set_features,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= tg3_poll_controller,
 #endif
@@ -15241,6 +15270,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 	dev->features |= hw_features;
 	dev->vlan_features |= hw_features;
 
+	/* Add the loopback capability */
+	dev->hw_features |= NETIF_F_LOOPBACK;
+
 	if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
 	    !tg3_flag(tp, TSO_CAPABLE) &&
 	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
-- 
1.7.3.1

Re: [PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Matt Carlson]

On Thu, Apr 28, 2011 at 04:33:19PM -0700, Mahesh Bandewar wrote:
> ---
>  drivers/net/tg3.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index fa57e3d..208884d 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -6319,6 +6319,33 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
>  	return features;
>  }
>  
> +static int tg3_set_features(struct net_device *dev, u32 features)
> +{
> +	struct tg3 *tp = netdev_priv(dev);
> +	u32 cur_mode = 0;
> +	int err = 0;
> +
> +	spin_lock_bh(&tp->lock);
> +	cur_mode = tr32(MAC_MODE);
> +
> +	if (features & NETIF_F_LOOPBACK) {
> +		/* Enable internal MAC loopback mode */
> +		tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);
> +		netif_carrier_on(tp->dev);
> +		netdev_info(dev, "Internal MAC loopback mode enabled.\n");
> +	} else {
> +		/* Disable internal MAC loopback mode */
> +		tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK);
> +		/* Force link status check */
> +		if (netif_running(dev))
> +			tg3_setup_phy(tp, 1);
> +		netdev_info(dev, "Internal MAC loopback mode disabled.\n");
> +	}

I think we should be checking netif_running() before touching the
hardware at all.  It might be in a low power state.

Also, wouldn't it be better to verify this particular bit changed before
doing any of these operations?

> +	spin_unlock_bh(&tp->lock);
> +
> +	return err;
> +}
> +
>  static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
>  			       int new_mtu)
>  {
> @@ -15028,6 +15055,7 @@ static const struct net_device_ops tg3_netdev_ops = {
>  	.ndo_tx_timeout		= tg3_tx_timeout,
>  	.ndo_change_mtu		= tg3_change_mtu,
>  	.ndo_fix_features	= tg3_fix_features,
> +	.ndo_set_features	= tg3_set_features,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= tg3_poll_controller,
>  #endif
> @@ -15044,6 +15072,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
>  	.ndo_do_ioctl		= tg3_ioctl,
>  	.ndo_tx_timeout		= tg3_tx_timeout,
>  	.ndo_change_mtu		= tg3_change_mtu,
> +	.ndo_set_features	= tg3_set_features,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= tg3_poll_controller,
>  #endif
> @@ -15241,6 +15270,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>  	dev->features |= hw_features;
>  	dev->vlan_features |= hw_features;
>  
> +	/* Add the loopback capability */
> +	dev->hw_features |= NETIF_F_LOOPBACK;

Not all tg3 devices can do MAC loopback.  I'd suggest qualifying this
with:

		if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
		    (tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))

But that will exclude a lot of our newer devices.  Does it matter what
type of loopback is used?  Newer devices prefer internal phy loopback
over MAC loopback.

>  	if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
>  	    !tg3_flag(tp, TSO_CAPABLE) &&
>  	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
> -- 
> 1.7.3.1
> 
> 

Re: [PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Thu, Apr 28, 2011 at 5:28 PM, Matt Carlson <mcarlson@broadcom.com> wrote:
> On Thu, Apr 28, 2011 at 04:33:19PM -0700, Mahesh Bandewar wrote:
>> ---
>>  drivers/net/tg3.c |   32 ++++++++++++++++++++++++++++++++
>>  1 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> index fa57e3d..208884d 100644
>> --- a/drivers/net/tg3.c
>> +++ b/drivers/net/tg3.c
>> @@ -6319,6 +6319,33 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
>>       return features;
>>  }
>>
>> +static int tg3_set_features(struct net_device *dev, u32 features)
>> +{
>> +     struct tg3 *tp = netdev_priv(dev);
>> +     u32 cur_mode = 0;
>> +     int err = 0;
>> +
>> +     spin_lock_bh(&tp->lock);
>> +     cur_mode = tr32(MAC_MODE);
>> +
>> +     if (features & NETIF_F_LOOPBACK) {
>> +             /* Enable internal MAC loopback mode */
>> +             tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);
>> +             netif_carrier_on(tp->dev);
>> +             netdev_info(dev, "Internal MAC loopback mode enabled.\n");
>> +     } else {
>> +             /* Disable internal MAC loopback mode */
>> +             tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK);
>> +             /* Force link status check */
>> +             if (netif_running(dev))
>> +                     tg3_setup_phy(tp, 1);
>> +             netdev_info(dev, "Internal MAC loopback mode disabled.\n");
>> +     }
>
> I think we should be checking netif_running() before touching the
> hardware at all.  It might be in a low power state.
>
makes sense. I'll change that.

> Also, wouldn't it be better to verify this particular bit changed before
> doing any of these operations?
>
Right! Since netdev infrastructure initiates ndo_set_features() only
when the feature-set is changed, and loopback is the only thing that
is set; I lazily ignored that. But in future additional features could
be set and it's not going to hurt checking before setting that bit.

>> +     spin_unlock_bh(&tp->lock);
>> +
>> +     return err;
>> +}
>> +
>>  static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
>>                              int new_mtu)
>>  {
>> @@ -15028,6 +15055,7 @@ static const struct net_device_ops tg3_netdev_ops = {
>>       .ndo_tx_timeout         = tg3_tx_timeout,
>>       .ndo_change_mtu         = tg3_change_mtu,
>>       .ndo_fix_features       = tg3_fix_features,
>> +     .ndo_set_features       = tg3_set_features,
>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>>       .ndo_poll_controller    = tg3_poll_controller,
>>  #endif
>> @@ -15044,6 +15072,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
>>       .ndo_do_ioctl           = tg3_ioctl,
>>       .ndo_tx_timeout         = tg3_tx_timeout,
>>       .ndo_change_mtu         = tg3_change_mtu,
>> +     .ndo_set_features       = tg3_set_features,
>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>>       .ndo_poll_controller    = tg3_poll_controller,
>>  #endif
>> @@ -15241,6 +15270,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>>       dev->features |= hw_features;
>>       dev->vlan_features |= hw_features;
>>
>> +     /* Add the loopback capability */
>> +     dev->hw_features |= NETIF_F_LOOPBACK;
>
> Not all tg3 devices can do MAC loopback.  I'd suggest qualifying this
> with:
>
>                if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
>                    (tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
>
> But that will exclude a lot of our newer devices.  Does it matter what
> type of loopback is used?  Newer devices prefer internal phy loopback
> over MAC loopback.
>
As long as device supports some sort of loopback, we should be setting
this capability and move this logic (or similar) to set_features() and
choose the method that is supported. Since several devices support
loopback at various levels, to keep it consistent, we should be
setting the loopback closest to the host. So can I simply set the
int-phy loopback in the else part of the above 'provided if' or would
need other logic? or in other words -

 if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
(tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
    supported_mode = MAC;
else
    supported_mode = INTPHY;

if (supported_mode == MAC)
    cur_mode = tr32(MAC_MODE);
else
    tg3_readphy(tp, MII_BMCR, &cur_mode);

Would something like this work?

Thanks,
--mahesh..

>>       if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
>>           !tg3_flag(tp, TSO_CAPABLE) &&
>>           !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
>> --
>> 1.7.3.1
>>
>>
>
>

Re: [PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Matt Carlson]

On Thu, Apr 28, 2011 at 06:42:02PM -0700, Mahesh Bandewar wrote:
> >> + ? ? spin_unlock_bh(&tp->lock);
> >> +
> >> + ? ? return err;
> >> +}
> >> +
> >> ?static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int new_mtu)
> >> ?{
> >> @@ -15028,6 +15055,7 @@ static const struct net_device_ops tg3_netdev_ops = {
> >> ? ? ? .ndo_tx_timeout ? ? ? ? = tg3_tx_timeout,
> >> ? ? ? .ndo_change_mtu ? ? ? ? = tg3_change_mtu,
> >> ? ? ? .ndo_fix_features ? ? ? = tg3_fix_features,
> >> + ? ? .ndo_set_features ? ? ? = tg3_set_features,
> >> ?#ifdef CONFIG_NET_POLL_CONTROLLER
> >> ? ? ? .ndo_poll_controller ? ?= tg3_poll_controller,
> >> ?#endif
> >> @@ -15044,6 +15072,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
> >> ? ? ? .ndo_do_ioctl ? ? ? ? ? = tg3_ioctl,
> >> ? ? ? .ndo_tx_timeout ? ? ? ? = tg3_tx_timeout,
> >> ? ? ? .ndo_change_mtu ? ? ? ? = tg3_change_mtu,
> >> + ? ? .ndo_set_features ? ? ? = tg3_set_features,
> >> ?#ifdef CONFIG_NET_POLL_CONTROLLER
> >> ? ? ? .ndo_poll_controller ? ?= tg3_poll_controller,
> >> ?#endif
> >> @@ -15241,6 +15270,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
> >> ? ? ? dev->features |= hw_features;
> >> ? ? ? dev->vlan_features |= hw_features;
> >>
> >> + ? ? /* Add the loopback capability */
> >> + ? ? dev->hw_features |= NETIF_F_LOOPBACK;
> >
> > Not all tg3 devices can do MAC loopback. ?I'd suggest qualifying this
> > with:
> >
> > ? ? ? ? ? ? ? ?if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
> > ? ? ? ? ? ? ? ? ? ?(tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
> >
> > But that will exclude a lot of our newer devices. ?Does it matter what
> > type of loopback is used? ?Newer devices prefer internal phy loopback
> > over MAC loopback.
> >
> As long as device supports some sort of loopback, we should be setting
> this capability and move this logic (or similar) to set_features() and
> choose the method that is supported. Since several devices support
> loopback at various levels, to keep it consistent, we should be
> setting the loopback closest to the host. So can I simply set the
> int-phy loopback in the else part of the above 'provided if' or would
> need other logic? or in other words -
> 
>  if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
> (tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
>     supported_mode = MAC;
> else
>     supported_mode = INTPHY;
> 
> if (supported_mode == MAC)
>     cur_mode = tr32(MAC_MODE);
> else
>     tg3_readphy(tp, MII_BMCR, &cur_mode);
> 
> Would something like this work?

That might be where we want to end up, but it'll be some work to get
there.  Maybe it makes sense to just keep MAC loopback mode for now and
only enable it for a subset of devices.  Adding phy loopback mode sounds
like it will require some surgery.

Re: [PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Thu, Apr 28, 2011 at 7:46 PM, Matt Carlson <mcarlson@broadcom.com> wrote:
> On Thu, Apr 28, 2011 at 06:42:02PM -0700, Mahesh Bandewar wrote:
>> >> + ? ? spin_unlock_bh(&tp->lock);
>> >> +
>> >> + ? ? return err;
>> >> +}
>> >> +
>> >> ?static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int new_mtu)
>> >> ?{
>> >> @@ -15028,6 +15055,7 @@ static const struct net_device_ops tg3_netdev_ops = {
>> >> ? ? ? .ndo_tx_timeout ? ? ? ? = tg3_tx_timeout,
>> >> ? ? ? .ndo_change_mtu ? ? ? ? = tg3_change_mtu,
>> >> ? ? ? .ndo_fix_features ? ? ? = tg3_fix_features,
>> >> + ? ? .ndo_set_features ? ? ? = tg3_set_features,
>> >> ?#ifdef CONFIG_NET_POLL_CONTROLLER
>> >> ? ? ? .ndo_poll_controller ? ?= tg3_poll_controller,
>> >> ?#endif
>> >> @@ -15044,6 +15072,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
>> >> ? ? ? .ndo_do_ioctl ? ? ? ? ? = tg3_ioctl,
>> >> ? ? ? .ndo_tx_timeout ? ? ? ? = tg3_tx_timeout,
>> >> ? ? ? .ndo_change_mtu ? ? ? ? = tg3_change_mtu,
>> >> + ? ? .ndo_set_features ? ? ? = tg3_set_features,
>> >> ?#ifdef CONFIG_NET_POLL_CONTROLLER
>> >> ? ? ? .ndo_poll_controller ? ?= tg3_poll_controller,
>> >> ?#endif
>> >> @@ -15241,6 +15270,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>> >> ? ? ? dev->features |= hw_features;
>> >> ? ? ? dev->vlan_features |= hw_features;
>> >>
>> >> + ? ? /* Add the loopback capability */
>> >> + ? ? dev->hw_features |= NETIF_F_LOOPBACK;
>> >
>> > Not all tg3 devices can do MAC loopback. ?I'd suggest qualifying this
>> > with:
>> >
>> > ? ? ? ? ? ? ? ?if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
>> > ? ? ? ? ? ? ? ? ? ?(tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
>> >
>> > But that will exclude a lot of our newer devices. ?Does it matter what
>> > type of loopback is used? ?Newer devices prefer internal phy loopback
>> > over MAC loopback.
>> >
>> As long as device supports some sort of loopback, we should be setting
>> this capability and move this logic (or similar) to set_features() and
>> choose the method that is supported. Since several devices support
>> loopback at various levels, to keep it consistent, we should be
>> setting the loopback closest to the host. So can I simply set the
>> int-phy loopback in the else part of the above 'provided if' or would
>> need other logic? or in other words -
>>
>>  if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5780 ||
>> (tp->tg3_flags & TG3_FLAG_CPMU_PRESENT))
>>     supported_mode = MAC;
>> else
>>     supported_mode = INTPHY;
>>
>> if (supported_mode == MAC)
>>     cur_mode = tr32(MAC_MODE);
>> else
>>     tg3_readphy(tp, MII_BMCR, &cur_mode);
>>
>> Would something like this work?
>
> That might be where we want to end up, but it'll be some work to get
> there.  Maybe it makes sense to just keep MAC loopback mode for now and
> only enable it for a subset of devices.  Adding phy loopback mode sounds
> like it will require some surgery.
>
Thanks for your comments Matt, I'll post the updated patch soon.

--mahesh..
>

Re: [PATCH 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Matt Carlson]

On Thu, Apr 28, 2011 at 04:33:19PM -0700, Mahesh Bandewar wrote:
> ---
>  drivers/net/tg3.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index fa57e3d..208884d 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -6319,6 +6319,33 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
>  	return features;
>  }
>  
> +static int tg3_set_features(struct net_device *dev, u32 features)
> +{
> +	struct tg3 *tp = netdev_priv(dev);
> +	u32 cur_mode = 0;
> +	int err = 0;
> +
> +	spin_lock_bh(&tp->lock);
> +	cur_mode = tr32(MAC_MODE);
> +
> +	if (features & NETIF_F_LOOPBACK) {
> +		/* Enable internal MAC loopback mode */
> +		tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);

I didn't notice this before, but you will want to clear
MAC_MODE_HALF_DUPLEX when going into loopback mode.  You won't get
packets back if you happen to negotiate to HD.

[PATCHv2 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v1:
 Implemented Matt's suggestions.
   (a) Enable this capability on the devices which are capable of MAC-loopback
       mode.
   (b) check if the device is running before making changes.
   (c) check bits before making changes.

 drivers/net/tg3.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index fa57e3d..f6e4c10 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -6319,6 +6319,51 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
 	return features;
 }
 
+static int tg3_set_features(struct net_device *dev, u32 features)
+{
+	struct tg3 *tp = netdev_priv(dev);
+	u32 cur_mode = 0;
+	int err = 0;
+
+	if (!netif_running(dev)) {
+		err = -EAGAIN;
+		goto sfeatures_out;
+	}
+
+	spin_lock_bh(&tp->lock);
+	cur_mode = tr32(MAC_MODE);
+
+	if (features & NETIF_F_LOOPBACK) {
+		if (cur_mode & MAC_MODE_PORT_INT_LPBACK)
+			goto sfeatures_unlock;
+
+		/*
+		 * Clear MAC_MODE_HALF_DUPLEX or you won't get packets back in
+		 * loopback mode if Half-Duplex mode was negotiated earlier.
+		 */
+		cur_mode &= ~MAC_MODE_HALF_DUPLEX;
+
+		/* Enable internal MAC loopback mode */
+		tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);
+		netif_carrier_on(tp->dev);
+		netdev_info(dev, "Internal MAC loopback mode enabled.\n");
+	} else {
+		if (!(cur_mode & MAC_MODE_PORT_INT_LPBACK))
+			goto sfeatures_unlock;
+
+		/* Disable internal MAC loopback mode */
+		tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK);
+		/* Force link status check */
+		tg3_setup_phy(tp, 1);
+		netdev_info(dev, "Internal MAC loopback mode disabled.\n");
+	}
+sfeatures_unlock:
+	spin_unlock_bh(&tp->lock);
+
+sfeatures_out:
+	return err;
+}
+
 static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
 			       int new_mtu)
 {
@@ -15028,6 +15073,7 @@ static const struct net_device_ops tg3_netdev_ops = {
 	.ndo_tx_timeout		= tg3_tx_timeout,
 	.ndo_change_mtu		= tg3_change_mtu,
 	.ndo_fix_features	= tg3_fix_features,
+	.ndo_set_features	= tg3_set_features,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= tg3_poll_controller,
 #endif
@@ -15044,6 +15090,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
 	.ndo_do_ioctl		= tg3_ioctl,
 	.ndo_tx_timeout		= tg3_tx_timeout,
 	.ndo_change_mtu		= tg3_change_mtu,
+	.ndo_set_features	= tg3_set_features,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= tg3_poll_controller,
 #endif
@@ -15241,6 +15288,16 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 	dev->features |= hw_features;
 	dev->vlan_features |= hw_features;
 
+	/*
+	 * Add loopback capability only for a subset of devices that support
+	 * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
+	 * loopback for the remaining devices.
+	 */
+	if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5780 ||
+			!tg3_flag(tp, CPMU_PRESENT))
+		/* Add the loopback capability */
+		dev->hw_features |= NETIF_F_LOOPBACK;
+
 	if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
 	    !tg3_flag(tp, TSO_CAPABLE) &&
 	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
-- 
1.7.3.1

Re: [PATCHv2 2/2] tg3: Add code to allow ethtool to enable/disable loopback.

[Joe Perches]

On Fri, 2011-04-29 at 18:03 -0700, Mahesh Bandewar wrote:
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

trivia:

>  drivers/net/tg3.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
[]
> @@ -15241,6 +15288,16 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
>  	dev->features |= hw_features;
>  	dev->vlan_features |= hw_features;
>  
> +	/*
> +	 * Add loopback capability only for a subset of devices that support
> +	 * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
> +	 * loopback for the remaining devices.
> +	 */
> +	if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5780 ||
> +			!tg3_flag(tp, CPMU_PRESENT))

The indentation style is a bit out of character for the file.
Aligned to open parenthesis is the style used elsewhere:

	if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5780 ||
	    !tg3_flag(tp, CPMU_PRESENT))

like this:

>  	if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
>  	    !tg3_flag(tp, TSO_CAPABLE) &&
>  	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {