Re: [RFT PATCH 4/9] net: introduce and use netdev_features_t for device features sets

[Ben Hutchings <bhutchings@solarflare.com>]

On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote:
> The type and bits definitions are moved to separate header so that
> linux/skbuff.h won't need to include linux/netdevice.h.
[...]
> diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
> index c914729..ca869c0 100644
> --- a/drivers/net/sfc/efx.c
> +++ b/drivers/net/sfc/efx.c
> @@ -1883,7 +1883,7 @@ static void efx_set_multicast_list(struct net_device *net_dev)
>  	/* Otherwise efx_start_port() will do this */
>  }
>  
> -static int efx_set_features(struct net_device *net_dev, u32 data)
> +static int efx_set_features(struct net_device *net_dev, netdev_features_t data)
>  {
>  	struct efx_nic *efx = netdev_priv(net_dev);
>  

The type of efx_nic_type::offload_features, defined in
drivers/net/sfc/net_driver.h, will also need to be changed.

[...]
> --- /dev/null
> +++ b/include/linux/netdev_features.h
[...]
> +	/* Segmentation offload features */
[...]
> +	/* Features valid for ethtool to change */
> +	/* = all defined minus driver/device-class-related */
[...]
> +	/* List of features with software fallbacks. */
[...]
> +	/*
> +	 * If one device supports one of these features, then enable them
> +	 * for all in netdev_increment_features.
> +	 */
[...]
> +	/*
> +	 * If one device doesn't support one of these features, then disable it
> +	 * for all in netdev_increment_features.
> +	 */
[...]
> +	/* changeable features with no special hardware requirements */
[...]

A cosmetic detail: these comments should no longer be indented.

[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3041b6ae..a5a0e76 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> @@ -1872,8 +1872,8 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, u32 features)
>  		if (dev && dev->ethtool_ops && dev->ethtool_ops->get_drvinfo)
>  			dev->ethtool_ops->get_drvinfo(dev, &info);
>  
> -		WARN(1, "%s: caps=(0x%lx, 0x%lx) len=%d data_len=%d ip_summed=%d\n",
> -		     info.driver, dev ? dev->features : 0L,
> +		WARN(1, "%s: caps=(0x%llx, 0x%lx) len=%d data_len=%d ip_summed=%d\n",
> +		     info.driver, dev ? (long long)dev->features : 0LL,
> 		     skb->sk ? skb->sk->sk_route_caps : 0L,
> 		     skb->len, skb->data_len, skb->ip_summed);

The types of sock::sk_route_caps and sock::sk_route_nocaps also need to
be fixed (they are still int and not even unsigned!).

[...]
> @@ -5250,11 +5252,10 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
>  
>  	return features;
>  }
> -EXPORT_SYMBOL(netdev_fix_features);

I don't think that change belongs in this patch!

[...]
> @@ -5270,8 +5271,10 @@ int __netdev_update_features(struct net_device *dev)
>  	if (dev->features == features)
>  		return 0;
>  
> +	BUILD_BUG_ON(sizeof(features) != sizeof(u32));	/* XXX: need: %Fx */
> +
>  	netdev_dbg(dev, "Features changed: 0x%08x -> 0x%08x\n",
> -		dev->features, features);
> +		(u32)dev->features, (u32)features);
[...]

You could either define macros for this:

#define NETDEV_FEATURES_FMT			"0x%08x"
#define NETDEV_FEATURES_FMT_ARG(features)	features

or follow the current trend of extending %p and passing a pointer to the
value.

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.