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

[Ben Hutchings <bhutchings@solarflare.com>]

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.