netdev - Re: [RFT PATCH 4/9] net: introduce and use netdev_features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1308603696.2701.185.camel@bwh-desktop>
Date:	Mon, 20 Jun 2011 22:01:36 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [RFT PATCH 4/9] net: introduce and use netdev_features_t for
 device features sets

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html