netdev - Re: [PATCH v5 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: <1321482632.2709.24.camel@bwh-desktop>
Date:	Wed, 16 Nov 2011 22:30:32 +0000
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: [PATCH v5 4/9] net: introduce and use netdev_features_t for
 device features sets

On Wed, 2011-11-16 at 02:29 +0100, Michał Mirosław wrote:
[...]
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
[...]
> @@ -3538,7 +3538,7 @@ static int __devinit init_one(struct pci_dev *pdev,
>  {
>  	int func, i, err;
>  	struct port_info *pi;
> -	unsigned int highdma = 0;
> +	bool highdma = false;
>  	struct adapter *adapter = NULL;
>  
>  	printk_once(KERN_INFO "%s - version %s\n", DRV_DESC, DRV_VERSION);
> @@ -3564,7 +3564,7 @@ static int __devinit init_one(struct pci_dev *pdev,
>  	}
>  
>  	if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(64))) {
> -		highdma = NETIF_F_HIGHDMA;
> +		highdma = true;
>  		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
>  		if (err) {
>  			dev_err(&pdev->dev, "unable to obtain 64-bit DMA for "
> @@ -3638,7 +3638,9 @@ static int __devinit init_one(struct pci_dev *pdev,
>  			NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  			NETIF_F_RXCSUM | NETIF_F_RXHASH |
>  			NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
> -		netdev->features |= netdev->hw_features | highdma;
> +		if (highdma)
> +			netdev->hw_features |= NETIF_F_HIGHDMA;

This wasn't previously included in hw_features.  Since hidma was being
used as a bitmask in this function, not a boolean, why don't you just
change its type to netdev_features_t for now?

> +		netdev->features |= netdev->hw_features;
>  		netdev->vlan_features = netdev->features & VLAN_FEAT;
>  
>  		netdev->priv_flags |= IFF_UNICAST_FLT;
[....]
> --- a/drivers/net/ethernet/jme.c
> +++ b/drivers/net/ethernet/jme.c
> @@ -1917,7 +1917,7 @@ jme_map_tx_skb(struct jme_adapter *jme, struct sk_buff *skb, int idx)
>  	struct jme_ring *txring = &(jme->txring[0]);
>  	struct txdesc *txdesc = txring->desc, *ctxdesc;
>  	struct jme_buffer_info *txbi = txring->bufinf, *ctxbi;
> -	u8 hidma = jme->dev->features & NETIF_F_HIGHDMA;
> +	u8 hidma = !!(jme->dev->features & NETIF_F_HIGHDMA);

The original here is nasty!  But you can change the type to bool and
then there is no need for the '!!'.

[...]
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -1579,10 +1579,10 @@ mv643xx_eth_set_ringparam(struct net_device *dev, struct ethtool_ringparam *er)
>  
> 
>  static int
> -mv643xx_eth_set_features(struct net_device *dev, u32 features)
> +mv643xx_eth_set_features(struct net_device *dev, netdev_features_t features)
>  {
>  	struct mv643xx_eth_private *mp = netdev_priv(dev);
> -	u32 rx_csum = features & NETIF_F_RXCSUM;
> +	int rx_csum = !!(features & NETIF_F_RXCSUM);

Again, change the type to bool.

[...]
> --- a/drivers/net/ethernet/marvell/sky2.c
> +++ b/drivers/net/ethernet/marvell/sky2.c
[...] 
> -static int sky2_set_features(struct net_device *dev, u32 features)
> +static int sky2_set_features(struct net_device *dev, netdev_features_t features)
>  {
>  	struct sky2_port *sky2 = netdev_priv(dev);
> -	u32 changed = dev->features ^ features;
> +	netdev_features_t changed = dev->features ^ features;
>  
>  	if (changed & NETIF_F_RXCSUM) {
> -		u32 on = features & NETIF_F_RXCSUM;
> +		int on = !!(features & NETIF_F_RXCSUM);

Same here.

[...]
> --- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> +++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
> @@ -1491,7 +1491,7 @@ myri10ge_clean_rx_done(struct myri10ge_slice_state *ss, int budget)
>  	 * access to avoid theoretical race condition with functions that
>  	 * change NETIF_F_LRO flag at runtime.
>  	 */
> -	bool lro_enabled = ACCESS_ONCE(mgp->dev->features) & NETIF_F_LRO;
> +	bool lro_enabled = !!(ACCESS_ONCE(mgp->dev->features) & NETIF_F_LRO);

No change required here.

[...]
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -203,7 +203,7 @@ static void xennet_sysfs_delif(struct net_device *netdev);
>  
>  static int xennet_can_sg(struct net_device *dev)
>  {
> -	return dev->features & NETIF_F_SG;
> +	return !!(dev->features & NETIF_F_SG);
>  }
[...]

You could change the return type to bool.

Ben.

-- 
Ben Hutchings, Staff 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