[PATCHv3 0/2] Loopback

[PATCHv3 0/2] Loopback

[Mahesh Bandewar]

First patch is the repost of the earlier loopback patch. tg3 implementation / patch demonstrates one such usage and I have incarporated changes suggested by Matt and Joe.

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

 drivers/net/tg3.c              |   57 ++++++++++++++++++++++++++++++++++++++++
 include/linux/netdevice.h      |    3 +-
 net/core/ethtool.c             |    2 +-
 3 files changed, 59 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

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 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 d5de66a..e7244ed 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

[PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

This patch adds tg3_set_features() to handle loopback mode. Currently the
capability is added for the devices which support internal MAC loopback mode.
So when enabled, it enables internal-MAC loopback.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v2:
 Implemtned Joe Perches's style change.

Changes since v1:
 Implemented Matt Carlson'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 7c7c9a8..46de633 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)
 {
@@ -15029,6 +15074,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
@@ -15045,6 +15091,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
@@ -15242,6 +15289,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: [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.

[Matt Carlson]

On Tue, May 03, 2011 at 06:18:55PM -0700, Mahesh Bandewar wrote:
> +	/*
> +	 * 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;

s/||/&&/

Re: [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

On Tue, May 03, 2011 at 06:18:55PM -0700, Mahesh Bandewar wrote:
> This patch adds tg3_set_features() to handle loopback mode. Currently the
> capability is added for the devices which support internal MAC loopback mode.
> So when enabled, it enables internal-MAC loopback.
[...]
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 7c7c9a8..46de633 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;
> +	}

netdev_update_features() is not designed to handle -EAGAIN from
ndo_set_features callback. It might be useful to implement this
handling, but in this case you should just return 0 and check
dev->features in ndo_open callback.

Best Regards,
Michał Mirosław

Re: [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.

[Stephen Hemminger]

On Wed, 4 May 2011 13:11:12 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> On Tue, May 03, 2011 at 06:18:55PM -0700, Mahesh Bandewar wrote:
> > This patch adds tg3_set_features() to handle loopback mode. Currently the
> > capability is added for the devices which support internal MAC loopback mode.
> > So when enabled, it enables internal-MAC loopback.
> [...]
> > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > index 7c7c9a8..46de633 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;
> > +	}
> 
> netdev_update_features() is not designed to handle -EAGAIN from
> ndo_set_features callback. It might be useful to implement this
> handling, but in this case you should just return 0 and check
> dev->features in ndo_open callback.
> 
> Best Regards,
> Michał Mirosław

EAGAIN is a bad choice of error code anyway. It implies that the
application should retry, which in this case is not true.

This error code is used for things like flow controlled sockets where
the application should do a select/poll and the condition will clear
when other side has read.

Why not use ENETDOWN instead?

Re: [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

On Wed, May 04, 2011 at 08:04:08AM -0700, Stephen Hemminger wrote:
> On Wed, 4 May 2011 13:11:12 +0200
> Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > On Tue, May 03, 2011 at 06:18:55PM -0700, Mahesh Bandewar wrote:
> > > This patch adds tg3_set_features() to handle loopback mode. Currently the
> > > capability is added for the devices which support internal MAC loopback mode.
> > > So when enabled, it enables internal-MAC loopback.
> > [...]
> > > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > > index 7c7c9a8..46de633 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;
> > > +	}
> > netdev_update_features() is not designed to handle -EAGAIN from
> > ndo_set_features callback. It might be useful to implement this
> > handling, but in this case you should just return 0 and check
> > dev->features in ndo_open callback.
> 
> EAGAIN is a bad choice of error code anyway. It implies that the
> application should retry, which in this case is not true.
> 
> This error code is used for things like flow controlled sockets where
> the application should do a select/poll and the condition will clear
> when other side has read.
> 
> Why not use ENETDOWN instead?

The "application" here is netdev_update_features() here. Whatever the
error code, it would need to be handled there.

Best Regards,
Michał Mirosław

Re: [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.

[Ben Hutchings]

On Wed, 2011-05-04 at 18:05 +0200, Michał Mirosław wrote:
> On Wed, May 04, 2011 at 08:04:08AM -0700, Stephen Hemminger wrote:
> > On Wed, 4 May 2011 13:11:12 +0200
> > Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > > On Tue, May 03, 2011 at 06:18:55PM -0700, Mahesh Bandewar wrote:
> > > > This patch adds tg3_set_features() to handle loopback mode. Currently the
> > > > capability is added for the devices which support internal MAC loopback mode.
> > > > So when enabled, it enables internal-MAC loopback.
> > > [...]
> > > > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > > > index 7c7c9a8..46de633 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;
> > > > +	}
> > > netdev_update_features() is not designed to handle -EAGAIN from
> > > ndo_set_features callback. It might be useful to implement this
> > > handling, but in this case you should just return 0 and check
> > > dev->features in ndo_open callback.
> > 
> > EAGAIN is a bad choice of error code anyway. It implies that the
> > application should retry, which in this case is not true.
> > 
> > This error code is used for things like flow controlled sockets where
> > the application should do a select/poll and the condition will clear
> > when other side has read.
> > 
> > Why not use ENETDOWN instead?
> 
> The "application" here is netdev_update_features() here. Whatever the
> error code, it would need to be handled there.

The important point - which I think you already stated - is that when
the device is down this function is not expected to apply changes to the
hardware and therefore it should return 0.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

This patch adds tg3_set_features() to handle loopback mode. Currently the
capability is added for the devices which support internal MAC loopback mode.
So when enabled, it enables internal-MAC loopback.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v3:
 (a) Corrected the condition (|| => &&) where loopback capability is added.
 (b) set_features() always returns 0.
 (c) Clear the loopback bit in ndo_open callback to avoid discrepancy.

Changes since v2:
 Implemtned Joe Perches's style change.

Changes since v1:
 Implemented Matt Carlson'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 |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 7c7c9a8..7f7a290 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -6309,6 +6309,44 @@ dma_error:
 	return NETDEV_TX_OK;
 }
 
+static int tg3_set_loopback(struct net_device *dev, u32 features)
+{
+	struct tg3 *tp = netdev_priv(dev);
+	u32 cur_mode = 0;
+	int retval = 0;
+
+	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);
+	return retval;
+}
+
 static u32 tg3_fix_features(struct net_device *dev, u32 features)
 {
 	struct tg3 *tp = netdev_priv(dev);
@@ -6319,6 +6357,16 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
 	return features;
 }
 
+static int tg3_set_features(struct net_device *dev, u32 features)
+{
+	u32 changed = dev->features ^ features;
+
+	if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
+		tg3_set_loopback(dev, features);
+
+	return 0;
+}
+
 static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
 			       int new_mtu)
 {
@@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
 
 	netif_tx_start_all_queues(dev);
 
+	/*
+	 * Reset loopback feature if it was turned on while the device was down
+	 * to avoid and any discrepancy in features reporting.
+	 */
+	if (dev->features & NETIF_F_LOOPBACK) {
+		dev->features &= ~NETIF_F_LOOPBACK;
+		dev->wanted_features &= ~NETIF_F_LOOPBACK;
+	}
+
 	return 0;
 
 err_out3:
@@ -15029,6 +15086,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
@@ -15045,6 +15103,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
@@ -15242,6 +15301,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: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

2011/5/5 Mahesh Bandewar <maheshb@google.com>:
> This patch adds tg3_set_features() to handle loopback mode. Currently the
> capability is added for the devices which support internal MAC loopback mode.
> So when enabled, it enables internal-MAC loopback.
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> Changes since v3:
>  (a) Corrected the condition (|| => &&) where loopback capability is added.
>  (b) set_features() always returns 0.
>  (c) Clear the loopback bit in ndo_open callback to avoid discrepancy.
>
> Changes since v2:
>  Implemtned Joe Perches's style change.
>
> Changes since v1:
>  Implemented Matt Carlson'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 |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 69 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 7c7c9a8..7f7a290 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -6309,6 +6309,44 @@ dma_error:
>        return NETDEV_TX_OK;
>  }
>
> +static int tg3_set_loopback(struct net_device *dev, u32 features)
> +{
> +       struct tg3 *tp = netdev_priv(dev);
> +       u32 cur_mode = 0;
> +       int retval = 0;

void? You always return zero and don't check it in callers.

[...]
> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
>
>        netif_tx_start_all_queues(dev);
>
> +       /*
> +        * Reset loopback feature if it was turned on while the device was down
> +        * to avoid and any discrepancy in features reporting.
> +        */
> +       if (dev->features & NETIF_F_LOOPBACK) {
> +               dev->features &= ~NETIF_F_LOOPBACK;
> +               dev->wanted_features &= ~NETIF_F_LOOPBACK;
> +       }
> +

if (dev->features & NETIF_F_LOOPBACK)
  tg3_set_loopback(dev, dev->features);

Whatever you do, don't modify wanted_features in drivers.

BTW, similar problems (also like in previous versions) are in
forcedeth implementation.

Best Regards,
Michał Mirosław

Re: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote:
> 2011/5/5 Mahesh Bandewar <maheshb@google.com>:
>> This patch adds tg3_set_features() to handle loopback mode. Currently the
>> capability is added for the devices which support internal MAC loopback mode.
>> So when enabled, it enables internal-MAC loopback.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>> Changes since v3:
>>  (a) Corrected the condition (|| => &&) where loopback capability is added.
>>  (b) set_features() always returns 0.
>>  (c) Clear the loopback bit in ndo_open callback to avoid discrepancy.
>>
>> Changes since v2:
>>  Implemtned Joe Perches's style change.
>>
>> Changes since v1:
>>  Implemented Matt Carlson'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 |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 69 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> index 7c7c9a8..7f7a290 100644
>> --- a/drivers/net/tg3.c
>> +++ b/drivers/net/tg3.c
>> @@ -6309,6 +6309,44 @@ dma_error:
>>        return NETDEV_TX_OK;
>>  }
>>
>> +static int tg3_set_loopback(struct net_device *dev, u32 features)
>> +{
>> +       struct tg3 *tp = netdev_priv(dev);
>> +       u32 cur_mode = 0;
>> +       int retval = 0;
>
> void? You always return zero and don't check it in callers.
>
> [...]
>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
>>
>>        netif_tx_start_all_queues(dev);
>>
>> +       /*
>> +        * Reset loopback feature if it was turned on while the device was down
>> +        * to avoid and any discrepancy in features reporting.
>> +        */
>> +       if (dev->features & NETIF_F_LOOPBACK) {
>> +               dev->features &= ~NETIF_F_LOOPBACK;
>> +               dev->wanted_features &= ~NETIF_F_LOOPBACK;
>> +       }
>> +
>
> if (dev->features & NETIF_F_LOOPBACK)
>  tg3_set_loopback(dev, dev->features);
>
Unfortunately at this stage device will not be able to set-loopback so
I resorted to clearing the bit(s).

> Whatever you do, don't modify wanted_features in drivers.
>
Since just clearing the 'features' would leave wanted_features in
discrepant state, I thought this will bring it to a sane state. So
what is a preferred way?

> BTW, similar problems (also like in previous versions) are in
> forcedeth implementation.
Yes, whatever we decide about the state of the wanted_features, I'll
implement similarly for forcedeth. Which previous problem you are
referring to? Is it the return value? There is a different kind of
failure (error while writing the register). Since update_features()
cant handle return value, I'm ignoring the return value. As far as the
correct return code is concerned, I wasn't sure what is appropriate
return code here (may be PHY_ERROR would be appropriate there) but
again I could ignore it just the way rest of the code is ignoring.

>
> Best Regards,
> Michał Mirosław
> --
> 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
>

Re: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar
<maheshb@google.com> napisał:
> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>> 2011/5/5 Mahesh Bandewar <maheshb@google.com>:
>>> This patch adds tg3_set_features() to handle loopback mode. Currently the
>>> capability is added for the devices which support internal MAC loopback mode.
>>> So when enabled, it enables internal-MAC loopback.
[...]
>>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
>>>
>>>        netif_tx_start_all_queues(dev);
>>>
>>> +       /*
>>> +        * Reset loopback feature if it was turned on while the device was down
>>> +        * to avoid and any discrepancy in features reporting.
>>> +        */
>>> +       if (dev->features & NETIF_F_LOOPBACK) {
>>> +               dev->features &= ~NETIF_F_LOOPBACK;
>>> +               dev->wanted_features &= ~NETIF_F_LOOPBACK;
>>> +       }
>>> +
>>
>> if (dev->features & NETIF_F_LOOPBACK)
>>  tg3_set_loopback(dev, dev->features);
>>
> Unfortunately at this stage device will not be able to set-loopback so
> I resorted to clearing the bit(s).

ndo_fix_features+ndo_set_features might be caled before ndo_open - the
first might be just from register_netdev() so even before ndo_open
callback.

>> Whatever you do, don't modify wanted_features in drivers.
> Since just clearing the 'features' would leave wanted_features in
> discrepant state, I thought this will bring it to a sane state. So
> what is a preferred way?

If you really can't do other way, driver should keep additional state
that is checked in ndo_fix_features callback.

>> BTW, similar problems (also like in previous versions) are in
>> forcedeth implementation.
> Yes, whatever we decide about the state of the wanted_features, I'll
> implement similarly for forcedeth. Which previous problem you are
> referring to? Is it the return value? There is a different kind of
> failure (error while writing the register). Since update_features()
> cant handle return value, I'm ignoring the return value. As far as the
> correct return code is concerned, I wasn't sure what is appropriate
> return code here (may be PHY_ERROR would be appropriate there) but
> again I could ignore it just the way rest of the code is ignoring.

Let's wait for this threads points to be resolved. You will probably
change the forcedeth's implementation then anyway. :-)

Best Regards,
Michał Mirosław

Re: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Thu, May 5, 2011 at 11:35 AM, Michał Mirosław <mirqus@gmail.com> wrote:
> W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar
> <maheshb@google.com> napisał:
>> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>>> 2011/5/5 Mahesh Bandewar <maheshb@google.com>:
>>>> This patch adds tg3_set_features() to handle loopback mode. Currently the
>>>> capability is added for the devices which support internal MAC loopback mode.
>>>> So when enabled, it enables internal-MAC loopback.
> [...]
>>>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
>>>>
>>>>        netif_tx_start_all_queues(dev);
>>>>
>>>> +       /*
>>>> +        * Reset loopback feature if it was turned on while the device was down
>>>> +        * to avoid and any discrepancy in features reporting.
>>>> +        */
>>>> +       if (dev->features & NETIF_F_LOOPBACK) {
>>>> +               dev->features &= ~NETIF_F_LOOPBACK;
>>>> +               dev->wanted_features &= ~NETIF_F_LOOPBACK;
>>>> +       }
>>>> +
>>>
>>> if (dev->features & NETIF_F_LOOPBACK)
>>>  tg3_set_loopback(dev, dev->features);
>>>
>> Unfortunately at this stage device will not be able to set-loopback so
>> I resorted to clearing the bit(s).
>
> ndo_fix_features+ndo_set_features might be caled before ndo_open - the
> first might be just from register_netdev() so even before ndo_open
> callback.
>
ndo_open is the triggering event, so anything before that point does
not really help.

>>> Whatever you do, don't modify wanted_features in drivers.
>> Since just clearing the 'features' would leave wanted_features in
>> discrepant state, I thought this will bring it to a sane state. So
>> what is a preferred way?
>
> If you really can't do other way, driver should keep additional state
> that is checked in ndo_fix_features callback.
>
ndo_fix_features callback is called just before calling
ndo_set_features callback. Also this is called during update_features,
so having the state maintained is not really going to help in this
scenario since that is not happening when device reaches ready state
(to remove this discrepancy in the feature-set).

There must be a reason (that you did not explain!) why driver code
should not alter wanted_feature set. So in this scenario, just leaving
the wanted_features as it is and clearing the features bit is correct
/ enough?


>>> BTW, similar problems (also like in previous versions) are in
>>> forcedeth implementation.
>> Yes, whatever we decide about the state of the wanted_features, I'll
>> implement similarly for forcedeth. Which previous problem you are
>> referring to? Is it the return value? There is a different kind of
>> failure (error while writing the register). Since update_features()
>> cant handle return value, I'm ignoring the return value. As far as the
>> correct return code is concerned, I wasn't sure what is appropriate
>> return code here (may be PHY_ERROR would be appropriate there) but
>> again I could ignore it just the way rest of the code is ignoring.
>
> Let's wait for this threads points to be resolved. You will probably
> change the forcedeth's implementation then anyway. :-)
>
agreed.

Thanks,
--mahesh..
> Best Regards,
> Michał Mirosław
>

Re: [PATCHv4 2/2] tg3: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

W dniu 6 maja 2011 01:16 użytkownik Mahesh Bandewar
<maheshb@google.com> napisał:
> On Thu, May 5, 2011 at 11:35 AM, Michał Mirosław <mirqus@gmail.com> wrote:
>> W dniu 5 maja 2011 19:47 użytkownik Mahesh Bandewar
>> <maheshb@google.com> napisał:
>>> On Wed, May 4, 2011 at 11:59 PM, Michał Mirosław <mirqus@gmail.com> wrote:
>>>> 2011/5/5 Mahesh Bandewar <maheshb@google.com>:
>>>>> This patch adds tg3_set_features() to handle loopback mode. Currently the
>>>>> capability is added for the devices which support internal MAC loopback mode.
>>>>> So when enabled, it enables internal-MAC loopback.
>> [...]
>>>>> @@ -9485,6 +9533,15 @@ static int tg3_open(struct net_device *dev)
>>>>>
>>>>>        netif_tx_start_all_queues(dev);
>>>>>
>>>>> +       /*
>>>>> +        * Reset loopback feature if it was turned on while the device was down
>>>>> +        * to avoid and any discrepancy in features reporting.
>>>>> +        */
>>>>> +       if (dev->features & NETIF_F_LOOPBACK) {
>>>>> +               dev->features &= ~NETIF_F_LOOPBACK;
>>>>> +               dev->wanted_features &= ~NETIF_F_LOOPBACK;
>>>>> +       }
>>>>> +
>>>>
>>>> if (dev->features & NETIF_F_LOOPBACK)
>>>>  tg3_set_loopback(dev, dev->features);
>>>>
>>> Unfortunately at this stage device will not be able to set-loopback so
>>> I resorted to clearing the bit(s).
>> ndo_fix_features+ndo_set_features might be caled before ndo_open - the
>> first might be just from register_netdev() so even before ndo_open
>> callback.
> ndo_open is the triggering event, so anything before that point does
> not really help.

You're right - I forgot that netif_running() becomes true just before
ndo_open is called. I have a hard time to convince myself that you
can't program the hardware for loopback just at the end of ndo_open
callback.

>>>> Whatever you do, don't modify wanted_features in drivers.
>>> Since just clearing the 'features' would leave wanted_features in
>>> discrepant state, I thought this will bring it to a sane state. So
>>> what is a preferred way?
>> If you really can't do other way, driver should keep additional state
>> that is checked in ndo_fix_features callback.
> ndo_fix_features callback is called just before calling
> ndo_set_features callback. Also this is called during update_features,
> so having the state maintained is not really going to help in this
> scenario since that is not happening when device reaches ready state
> (to remove this discrepancy in the feature-set).

You can call netdev_update_features() at the end of ndo_open. But that
would be the same in the end as just enabling the loopback there.

> There must be a reason (that you did not explain!) why driver code
> should not alter wanted_feature set. So in this scenario, just leaving
> the wanted_features as it is and clearing the features bit is correct
> / enough?

wanted_features reflects a set of features the user wants. I assume
that no code can change the user himself, so changing wanted_features
instead would break this connection. You can view wanted_features as a
cache of user's intention. ;-)

Best Regards,
Michał Mirosław

[PATCHv5 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

This patch adds tg3_set_features() to handle loopback mode. Currently the
capability is added for the devices which support internal MAC loopback mode.
So when enabled, it enables internal-MAC loopback.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v4:
 (a) Added TG3_FLAG_LOOPBACK_ENABLED flag to keep loopback state in driver's 
     private data-structure.
 (b) Corrected the loopback implementation by using tp->mac_mode.
 (c) Forced Link up when in loopback mode. 

Changes since v3:
 (a) Corrected the condition (|| => &&) where loopback capability is added.
 (b) set_features() always returns 0.
 (c) Clear the loopback bit in ndo_open callback to avoid discrepancy.

Changes since v2:
 Implemtned Joe Perches's style change.

Changes since v1:
 Implemented Matt Carlson'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 |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/net/tg3.h |    1 +
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 7c7c9a8..b7270c2 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -3373,8 +3373,8 @@ relink:
 		tg3_phy_copper_begin(tp);
 
 		tg3_readphy(tp, MII_BMSR, &bmsr);
-		if (!tg3_readphy(tp, MII_BMSR, &bmsr) &&
-		    (bmsr & BMSR_LSTATUS))
+		if ((!tg3_readphy(tp, MII_BMSR, &bmsr) && (bmsr & BMSR_LSTATUS)) ||
+		    (tp->mac_mode & MAC_MODE_PORT_INT_LPBACK))
 			current_link_up = 1;
 	}
 
@@ -6309,6 +6309,43 @@ dma_error:
 	return NETDEV_TX_OK;
 }
 
+static void tg3_set_loopback(struct net_device *dev)
+{
+	struct tg3 *tp = netdev_priv(dev);
+
+	if (tg3_flag(tp, LOOPBACK_ENABLED)) {
+		if (tp->mac_mode & MAC_MODE_PORT_INT_LPBACK)
+			return;
+
+		/*
+		 * Clear MAC_MODE_HALF_DUPLEX or you won't get packets back in
+		 * loopback mode if Half-Duplex mode was negotiated earlier.
+		 */
+		tp->mac_mode &= ~MAC_MODE_HALF_DUPLEX;
+
+		/* Enable internal MAC loopback mode */
+		tp->mac_mode |= MAC_MODE_PORT_INT_LPBACK;
+		spin_lock_bh(&tp->lock);
+		tw32(MAC_MODE, tp->mac_mode);
+		netif_carrier_on(tp->dev);
+		spin_unlock_bh(&tp->lock);
+		netdev_info(dev, "Internal MAC loopback mode enabled.\n");
+	} else {
+		if (!(tp->mac_mode & MAC_MODE_PORT_INT_LPBACK))
+			return;
+
+		/* Disable internal MAC loopback mode */
+		tp->mac_mode &= ~MAC_MODE_PORT_INT_LPBACK;
+		spin_lock_bh(&tp->lock);
+		tw32(MAC_MODE, tp->mac_mode);
+		/* Force link status check */
+		tg3_setup_phy(tp, 1);
+		spin_unlock_bh(&tp->lock);
+		netdev_info(dev, "Internal MAC loopback mode disabled.\n");
+	}
+
+}
+
 static u32 tg3_fix_features(struct net_device *dev, u32 features)
 {
 	struct tg3 *tp = netdev_priv(dev);
@@ -6319,6 +6356,24 @@ 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 changed = dev->features ^ features;
+
+	if (changed & NETIF_F_LOOPBACK) {
+		if (tg3_flag(tp, LOOPBACK_ENABLED))
+			tg3_flag_clear(tp, LOOPBACK_ENABLED);
+		else
+			tg3_flag_set(tp, LOOPBACK_ENABLED);
+
+		if (netif_running(dev))
+			tg3_set_loopback(dev);
+	}
+
+	return 0;
+}
+
 static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
 			       int new_mtu)
 {
@@ -9485,6 +9540,13 @@ static int tg3_open(struct net_device *dev)
 
 	netif_tx_start_all_queues(dev);
 
+	/*
+	 * Reset loopback feature if it was turned on while the device was down
+	 * make sure that it's installed properly now.
+	 */
+	if (tg3_flag(tp, LOOPBACK_ENABLED))
+		tg3_set_loopback(dev);
+
 	return 0;
 
 err_out3:
@@ -15029,6 +15091,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
@@ -15045,6 +15108,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
@@ -15242,6 +15306,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)) {
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index ce010cd3..d087ef0 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2891,6 +2891,7 @@ enum TG3_FLAGS {
 	TG3_FLAG_57765_PLUS,
 	TG3_FLAG_APE_HAS_NCSI,
 	TG3_FLAG_5717_PLUS,
+	TG3_FLAG_LOOPBACK_ENABLED,
 
 	/* Add new flags before this comment and TG3_FLAG_NUMBER_OF_FLAGS */
 	TG3_FLAG_NUMBER_OF_FLAGS,	/* Last entry in enum TG3_FLAGS */
-- 
1.7.3.1

Re: [PATCHv5 2/2] tg3: Allow ethtool to enable/disable loopback.

[Michał Mirosław]

On Fri, May 06, 2011 at 11:18:37PM -0700, Mahesh Bandewar wrote:
> This patch adds tg3_set_features() to handle loopback mode. Currently the
> capability is added for the devices which support internal MAC loopback mode.
> So when enabled, it enables internal-MAC loopback.
[...]
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 7c7c9a8..b7270c2 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
[...]
> @@ -6319,6 +6356,24 @@ 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 changed = dev->features ^ features;
> +
> +	if (changed & NETIF_F_LOOPBACK) {
> +		if (tg3_flag(tp, LOOPBACK_ENABLED))
> +			tg3_flag_clear(tp, LOOPBACK_ENABLED);
> +		else
> +			tg3_flag_set(tp, LOOPBACK_ENABLED);
> +
> +		if (netif_running(dev))
> +			tg3_set_loopback(dev);
> +	}
> +
> +	return 0;
> +}
> +
>  static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
>  			       int new_mtu)
>  {
> @@ -9485,6 +9540,13 @@ static int tg3_open(struct net_device *dev)
>  
>  	netif_tx_start_all_queues(dev);
>  
> +	/*
> +	 * Reset loopback feature if it was turned on while the device was down
> +	 * make sure that it's installed properly now.
> +	 */
> +	if (tg3_flag(tp, LOOPBACK_ENABLED))
> +		tg3_set_loopback(dev);
> +
>  	return 0;
>  
>  err_out3:
[...]

So, you've just implemented what I said about enabling loopback at the end
of tg3_open(), but you also added (redundant) flag that mirrors
dev->features & NETIF_F_LOOPBACK. Why?

Best Regards,
Michał Mirosław

Re: [PATCHv5 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

On Sat, May 7, 2011 at 12:43 AM, Michał Mirosław
<mirq-linux@rere.qmqm.pl> wrote:
> On Fri, May 06, 2011 at 11:18:37PM -0700, Mahesh Bandewar wrote:
>> This patch adds tg3_set_features() to handle loopback mode. Currently the
>> capability is added for the devices which support internal MAC loopback mode.
>> So when enabled, it enables internal-MAC loopback.
> [...]
>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> index 7c7c9a8..b7270c2 100644
>> --- a/drivers/net/tg3.c
>> +++ b/drivers/net/tg3.c
> [...]
>> @@ -6319,6 +6356,24 @@ 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 changed = dev->features ^ features;
>> +
>> +     if (changed & NETIF_F_LOOPBACK) {
>> +             if (tg3_flag(tp, LOOPBACK_ENABLED))
>> +                     tg3_flag_clear(tp, LOOPBACK_ENABLED);
>> +             else
>> +                     tg3_flag_set(tp, LOOPBACK_ENABLED);
>> +
>> +             if (netif_running(dev))
>> +                     tg3_set_loopback(dev);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
>>                              int new_mtu)
>>  {
>> @@ -9485,6 +9540,13 @@ static int tg3_open(struct net_device *dev)
>>
>>       netif_tx_start_all_queues(dev);
>>
>> +     /*
>> +      * Reset loopback feature if it was turned on while the device was down
>> +      * make sure that it's installed properly now.
>> +      */
>> +     if (tg3_flag(tp, LOOPBACK_ENABLED))
>> +             tg3_set_loopback(dev);
>> +
>>       return 0;
>>
>>  err_out3:
> [...]
>
> So, you've just implemented what I said about enabling loopback at the end
> of tg3_open(), but you also added (redundant) flag that mirrors
> dev->features & NETIF_F_LOOPBACK. Why?
>
I had tried it before but it failed and I thought the device is not
ready at the end of ndo_open call back, but in fact my change itself
was causing the discrepancy in driver's private data structure. This I
found while debugging the change and corrected by using tp->mac_mode
for the current state of that reg instead of reading it from the reg
into a local variable. I agree, as a side effect of this a redundant
flag got created, which I'll remove. Thanks for pointing it out.

Regards,
--mahesh..

> Best Regards,
> Michał Mirosław
>

[PATCHv6 2/2] tg3: Allow ethtool to enable/disable loopback.

[Mahesh Bandewar]

This patch adds tg3_set_features() to handle loopback mode. Currently the
capability is added for the devices which support internal MAC loopback mode.
So when enabled, it enables internal-MAC loopback.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v5:
 (a) Removed TG3_FLAG_LOOPBACK_ENABLED flag.

Changes since v4:
 (a) Added TG3_FLAG_LOOPBACK_ENABLED flag to keep loopback state in driver's 
     private data-structure.
 (b) Corrected the loopback implementation by using tp->mac_mode.
 (c) Forced Link up when in loopback mode. 

Changes since v3:
 (a) Corrected the condition (|| => &&) where loopback capability is added.
 (b) set_features() always returns 0.
 (c) Clear the loopback bit in ndo_open callback to avoid discrepancy.

Changes since v2:
 Implemtned Joe Perches's style change.

Changes since v1:
 Implemented Matt Carlson'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 |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 7c7c9a8..9f77533 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -3373,8 +3373,8 @@ relink:
 		tg3_phy_copper_begin(tp);
 
 		tg3_readphy(tp, MII_BMSR, &bmsr);
-		if (!tg3_readphy(tp, MII_BMSR, &bmsr) &&
-		    (bmsr & BMSR_LSTATUS))
+		if ((!tg3_readphy(tp, MII_BMSR, &bmsr) && (bmsr & BMSR_LSTATUS)) ||
+		    (tp->mac_mode & MAC_MODE_PORT_INT_LPBACK))
 			current_link_up = 1;
 	}
 
@@ -6309,6 +6309,42 @@ dma_error:
 	return NETDEV_TX_OK;
 }
 
+static void tg3_set_loopback(struct net_device *dev, u32 features)
+{
+	struct tg3 *tp = netdev_priv(dev);
+
+	if (features & NETIF_F_LOOPBACK) {
+		if (tp->mac_mode & MAC_MODE_PORT_INT_LPBACK)
+			return;
+
+		/*
+		 * Clear MAC_MODE_HALF_DUPLEX or you won't get packets back in
+		 * loopback mode if Half-Duplex mode was negotiated earlier.
+		 */
+		tp->mac_mode &= ~MAC_MODE_HALF_DUPLEX;
+
+		/* Enable internal MAC loopback mode */
+		tp->mac_mode |= MAC_MODE_PORT_INT_LPBACK;
+		spin_lock_bh(&tp->lock);
+		tw32(MAC_MODE, tp->mac_mode);
+		netif_carrier_on(tp->dev);
+		spin_unlock_bh(&tp->lock);
+		netdev_info(dev, "Internal MAC loopback mode enabled.\n");
+	} else {
+		if (!(tp->mac_mode & MAC_MODE_PORT_INT_LPBACK))
+			return;
+
+		/* Disable internal MAC loopback mode */
+		tp->mac_mode &= ~MAC_MODE_PORT_INT_LPBACK;
+		spin_lock_bh(&tp->lock);
+		tw32(MAC_MODE, tp->mac_mode);
+		/* Force link status check */
+		tg3_setup_phy(tp, 1);
+		spin_unlock_bh(&tp->lock);
+		netdev_info(dev, "Internal MAC loopback mode disabled.\n");
+	}
+}
+
 static u32 tg3_fix_features(struct net_device *dev, u32 features)
 {
 	struct tg3 *tp = netdev_priv(dev);
@@ -6319,6 +6355,16 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
 	return features;
 }
 
+static int tg3_set_features(struct net_device *dev, u32 features)
+{
+	u32 changed = dev->features ^ features;
+
+	if ((changed & NETIF_F_LOOPBACK) && netif_running(dev))
+		tg3_set_loopback(dev, features);
+
+	return 0;
+}
+
 static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
 			       int new_mtu)
 {
@@ -9485,6 +9531,13 @@ static int tg3_open(struct net_device *dev)
 
 	netif_tx_start_all_queues(dev);
 
+	/*
+	 * Reset loopback feature if it was turned on while the device was down
+	 * make sure that it's installed properly now.
+	 */
+	if (dev->features & NETIF_F_LOOPBACK)
+		tg3_set_loopback(dev, dev->features);
+
 	return 0;
 
 err_out3:
@@ -15029,6 +15082,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
@@ -15045,6 +15099,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
@@ -15242,6 +15297,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: [PATCHv6 2/2] tg3: Allow ethtool to enable/disable loopback.

[David Miller]

From: Mahesh Bandewar <maheshb@google.com>
Date: Sun,  8 May 2011 09:51:48 -0700

> This patch adds tg3_set_features() to handle loopback mode. Currently the
> capability is added for the devices which support internal MAC loopback mode.
> So when enabled, it enables internal-MAC loopback.
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

Applied, thanks.

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

[Michał Mirosław]

On Tue, May 03, 2011 at 06:18:54PM -0700, Mahesh Bandewar wrote:
> 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
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
>  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 d5de66a..e7244ed 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 */
[...]

Just for correctness: you could add this flag to loopback's dev->features.
It's just an aesthetics point, though.

Best Regards,
Michał Mirosław

[PATCHv2 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

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v1
 Added NETIF_F_LOOPBACK in loopback device's feature-set.

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

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index d70fb76..4ce9e5f 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -174,7 +174,8 @@ static void loopback_setup(struct net_device *dev)
 		| NETIF_F_HIGHDMA
 		| NETIF_F_LLTX
 		| NETIF_F_NETNS_LOCAL
-		| NETIF_F_VLAN_CHALLENGED;
+		| NETIF_F_VLAN_CHALLENGED
+		| NETIF_F_LOOPBACK;
 	dev->ethtool_ops	= &loopback_ethtool_ops;
 	dev->header_ops		= &eth_header_ops;
 	dev->netdev_ops		= &loopback_ops;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d5de66a..e7244ed 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

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

[Ben Hutchings]

On Wed, 2011-05-04 at 18:30 -0700, Mahesh Bandewar wrote:
> 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
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 05 May 2011 02:52:48 +0100

> On Wed, 2011-05-04 at 18:30 -0700, Mahesh Bandewar wrote:
>> 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
>> 
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

[PATCH net-next-2.6] sfc: Implement NETIF_F_LOOPBACK

[Ben Hutchings]

We already have comprehensive support for loopback testing, so pick
the nearest mode and run with it.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
David, this depends on Mahesh's patch to define NETIF_F_LOOPBACK
<http://patchwork.ozlabs.org/patch/94190/>.  Although you've marked that
as RFC, I think it is ready to apply.

Ben.

 drivers/net/sfc/efx.c |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 38a55e9..b7bfb49 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1884,6 +1884,25 @@ static int efx_set_features(struct net_device *net_dev, u32 data)
 	if (net_dev->features & ~data & NETIF_F_NTUPLE)
 		efx_filter_clear_rx(efx, EFX_FILTER_PRI_MANUAL);
 
+	/* Toggle loopback if required */
+	if ((net_dev->features ^ data) & NETIF_F_LOOPBACK) {
+		enum efx_loopback_mode old_mode;
+		int rc;
+
+		mutex_lock(&efx->mac_lock);
+		old_mode = efx->loopback_mode;
+		if (data & NETIF_F_LOOPBACK)
+			efx->loopback_mode = __ffs(efx->loopback_modes);
+		else
+			efx->loopback_mode = LOOPBACK_NONE;
+		rc = __efx_reconfigure_port(efx);
+		if (rc)
+			efx->loopback_mode = old_mode;
+		mutex_unlock(&efx->mac_lock);
+		if (rc)
+			return rc;
+	}
+
 	return 0;
 }
 
@@ -2472,8 +2491,9 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
 	net_dev->vlan_features |= (NETIF_F_ALL_CSUM | NETIF_F_SG |
 				   NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |
 				   NETIF_F_RXCSUM);
-	/* All offloads can be toggled */
-	net_dev->hw_features = net_dev->features & ~NETIF_F_HIGHDMA;
+	/* All offloads can be toggled, and so can loopback */
+	net_dev->hw_features = ((net_dev->features & ~NETIF_F_HIGHDMA) |
+				NETIF_F_LOOPBACK);
 	efx = netdev_priv(net_dev);
 	pci_set_drvdata(pci_dev, efx);
 	SET_NETDEV_DEV(net_dev, &pci_dev->dev);
-- 
1.7.4


-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6] sfc: Implement NETIF_F_LOOPBACK

[Michał Mirosław]

On Fri, May 06, 2011 at 01:12:31AM +0100, Ben Hutchings wrote:
> We already have comprehensive support for loopback testing, so pick
> the nearest mode and run with it.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> David, this depends on Mahesh's patch to define NETIF_F_LOOPBACK
> <http://patchwork.ozlabs.org/patch/94190/>.  Although you've marked that
> as RFC, I think it is ready to apply.
> 
> Ben.
> 
>  drivers/net/sfc/efx.c |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
> index 38a55e9..b7bfb49 100644
> --- a/drivers/net/sfc/efx.c
> +++ b/drivers/net/sfc/efx.c
> @@ -1884,6 +1884,25 @@ static int efx_set_features(struct net_device *net_dev, u32 data)
>  	if (net_dev->features & ~data & NETIF_F_NTUPLE)
>  		efx_filter_clear_rx(efx, EFX_FILTER_PRI_MANUAL);
>  
> +	/* Toggle loopback if required */
> +	if ((net_dev->features ^ data) & NETIF_F_LOOPBACK) {
> +		enum efx_loopback_mode old_mode;
> +		int rc;
> +
> +		mutex_lock(&efx->mac_lock);
> +		old_mode = efx->loopback_mode;
> +		if (data & NETIF_F_LOOPBACK)
> +			efx->loopback_mode = __ffs(efx->loopback_modes);
> +		else
> +			efx->loopback_mode = LOOPBACK_NONE;
> +		rc = __efx_reconfigure_port(efx);
> +		if (rc)
> +			efx->loopback_mode = old_mode;
> +		mutex_unlock(&efx->mac_lock);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	return 0;
>  }

If other features than NETIF_F_LOOPBACK were changed correctly, this should
update dev->features of what changed before error return. If not, device's
state will diverge to what is in dev->features.

> @@ -2472,8 +2491,9 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
>  	net_dev->vlan_features |= (NETIF_F_ALL_CSUM | NETIF_F_SG |
>  				   NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |
>  				   NETIF_F_RXCSUM);
> -	/* All offloads can be toggled */
> -	net_dev->hw_features = net_dev->features & ~NETIF_F_HIGHDMA;
> +	/* All offloads can be toggled, and so can loopback */
> +	net_dev->hw_features = ((net_dev->features & ~NETIF_F_HIGHDMA) |
> +				NETIF_F_LOOPBACK);
>  	efx = netdev_priv(net_dev);
>  	pci_set_drvdata(pci_dev, efx);
>  	SET_NETDEV_DEV(net_dev, &pci_dev->dev);

You can remove the '& ~NETIF_F_HIGHDMA' part, as it's now allowed to be
changed (commit fa2bd7ff9247f4218dfc907db14d000cd7edd862).

Best Regards,
Michał Mirosław

Re: [PATCH net-next-2.6] sfc: Implement NETIF_F_LOOPBACK

[Ben Hutchings]

On Mon, 2011-05-09 at 09:36 +0200, Michał Mirosław wrote:
> On Fri, May 06, 2011 at 01:12:31AM +0100, Ben Hutchings wrote:
> > We already have comprehensive support for loopback testing, so pick
> > the nearest mode and run with it.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> > ---
> > David, this depends on Mahesh's patch to define NETIF_F_LOOPBACK
> > <http://patchwork.ozlabs.org/patch/94190/>.  Although you've marked that
> > as RFC, I think it is ready to apply.
> > 
> > Ben.
> > 
> >  drivers/net/sfc/efx.c |   24 ++++++++++++++++++++++--
> >  1 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
> > index 38a55e9..b7bfb49 100644
> > --- a/drivers/net/sfc/efx.c
> > +++ b/drivers/net/sfc/efx.c
> > @@ -1884,6 +1884,25 @@ static int efx_set_features(struct net_device *net_dev, u32 data)
> >  	if (net_dev->features & ~data & NETIF_F_NTUPLE)
> >  		efx_filter_clear_rx(efx, EFX_FILTER_PRI_MANUAL);
> >  
> > +	/* Toggle loopback if required */
> > +	if ((net_dev->features ^ data) & NETIF_F_LOOPBACK) {
> > +		enum efx_loopback_mode old_mode;
> > +		int rc;
> > +
> > +		mutex_lock(&efx->mac_lock);
> > +		old_mode = efx->loopback_mode;
> > +		if (data & NETIF_F_LOOPBACK)
> > +			efx->loopback_mode = __ffs(efx->loopback_modes);
> > +		else
> > +			efx->loopback_mode = LOOPBACK_NONE;
> > +		rc = __efx_reconfigure_port(efx);
> > +		if (rc)
> > +			efx->loopback_mode = old_mode;
> > +		mutex_unlock(&efx->mac_lock);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> >  	return 0;
> >  }
> 
> If other features than NETIF_F_LOOPBACK were changed correctly, this should
> update dev->features of what changed before error return. If not, device's
> state will diverge to what is in dev->features.

In general __efx_reconfigure_port() might fail because PHYs are tricky
beasts, but for internal loopback that isn't an issue.  So this is very
unlikely to fail.  But I suppose I should move it to the top of the
function.

> > @@ -2472,8 +2491,9 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
> >  	net_dev->vlan_features |= (NETIF_F_ALL_CSUM | NETIF_F_SG |
> >  				   NETIF_F_HIGHDMA | NETIF_F_ALL_TSO |
> >  				   NETIF_F_RXCSUM);
> > -	/* All offloads can be toggled */
> > -	net_dev->hw_features = net_dev->features & ~NETIF_F_HIGHDMA;
> > +	/* All offloads can be toggled, and so can loopback */
> > +	net_dev->hw_features = ((net_dev->features & ~NETIF_F_HIGHDMA) |
> > +				NETIF_F_LOOPBACK);
> >  	efx = netdev_priv(net_dev);
> >  	pci_set_drvdata(pci_dev, efx);
> >  	SET_NETDEV_DEV(net_dev, &pci_dev->dev);
> 
> You can remove the '& ~NETIF_F_HIGHDMA' part, as it's now allowed to be
> changed (commit fa2bd7ff9247f4218dfc907db14d000cd7edd862).

OK.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.