Re: [PATCH v5 4/9] net: introduce and use netdev_features_t for device features sets
From: Ben Hutchings <hidden>
Date: 2011-11-16 22:30:37
On Wed, 2011-11-16 at 02:29 +0100, Michał Mirosław wrote: [...]
quoted hunk ↗ jump to hunk
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
[...]
quoted hunk ↗ jump to hunk
@@ -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;
[....]
quoted hunk ↗ jump to hunk
--- 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 '!!'. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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.