[PATCH] net: enic: convert to hw_features

[PATCH] net: enic: convert to hw_features

[Michał Mirosław]

As the driver uses GRO and not LRO, LRO settings are ignored anyway
and are removed here to avoid confusion.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/enic/enic.h      |    1 -
 drivers/net/enic/enic_main.c |   74 ++++-------------------------------------
 drivers/net/enic/enic_res.c  |    4 +-
 3 files changed, 10 insertions(+), 69 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 178b94d..38b351c 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -84,7 +84,6 @@ struct enic {
 	unsigned int flags;
 	unsigned int mc_count;
 	unsigned int uc_count;
-	int csum_rx_enabled;
 	u32 port_mtu;
 	u32 rx_coalesce_usecs;
 	u32 tx_coalesce_usecs;
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9a3a027..b224551 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -251,56 +251,6 @@ static void enic_get_ethtool_stats(struct net_device *netdev,
 		*(data++) = ((u64 *)&vstats->rx)[enic_rx_stats[i].offset];
 }
 
-static u32 enic_get_rx_csum(struct net_device *netdev)
-{
-	struct enic *enic = netdev_priv(netdev);
-	return enic->csum_rx_enabled;
-}
-
-static int enic_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct enic *enic = netdev_priv(netdev);
-
-	if (data && !ENIC_SETTING(enic, RXCSUM))
-		return -EINVAL;
-
-	enic->csum_rx_enabled = !!data;
-
-	return 0;
-}
-
-static int enic_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	struct enic *enic = netdev_priv(netdev);
-
-	if (data && !ENIC_SETTING(enic, TXCSUM))
-		return -EINVAL;
-
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-
-	return 0;
-}
-
-static int enic_set_tso(struct net_device *netdev, u32 data)
-{
-	struct enic *enic = netdev_priv(netdev);
-
-	if (data && !ENIC_SETTING(enic, TSO))
-		return -EINVAL;
-
-	if (data)
-		netdev->features |=
-			NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN;
-	else
-		netdev->features &=
-			~(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN);
-
-	return 0;
-}
-
 static u32 enic_get_msglevel(struct net_device *netdev)
 {
 	struct enic *enic = netdev_priv(netdev);
@@ -388,17 +338,8 @@ static const struct ethtool_ops enic_ethtool_ops = {
 	.get_strings = enic_get_strings,
 	.get_sset_count = enic_get_sset_count,
 	.get_ethtool_stats = enic_get_ethtool_stats,
-	.get_rx_csum = enic_get_rx_csum,
-	.set_rx_csum = enic_set_rx_csum,
-	.get_tx_csum = ethtool_op_get_tx_csum,
-	.set_tx_csum = enic_set_tx_csum,
-	.get_sg = ethtool_op_get_sg,
-	.set_sg = ethtool_op_set_sg,
-	.get_tso = ethtool_op_get_tso,
-	.set_tso = enic_set_tso,
 	.get_coalesce = enic_get_coalesce,
 	.set_coalesce = enic_set_coalesce,
-	.get_flags = ethtool_op_get_flags,
 };
 
 static void enic_free_wq_buf(struct vnic_wq *wq, struct vnic_wq_buf *buf)
@@ -1309,7 +1250,7 @@ static void enic_rq_indicate_buf(struct vnic_rq *rq,
 		skb_put(skb, bytes_written);
 		skb->protocol = eth_type_trans(skb, netdev);
 
-		if (enic->csum_rx_enabled && !csum_not_calc) {
+		if ((netdev->features & NETIF_F_RXCSUM) && !csum_not_calc) {
 			skb->csum = htons(checksum);
 			skb->ip_summed = CHECKSUM_COMPLETE;
 		}
@@ -2438,17 +2379,18 @@ static int __devinit enic_probe(struct pci_dev *pdev,
 		dev_info(dev, "loopback tag=0x%04x\n", enic->loop_tag);
 	}
 	if (ENIC_SETTING(enic, TXCSUM))
-		netdev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
+		netdev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM;
 	if (ENIC_SETTING(enic, TSO))
-		netdev->features |= NETIF_F_TSO |
+		netdev->hw_features |= NETIF_F_TSO |
 			NETIF_F_TSO6 | NETIF_F_TSO_ECN;
-	if (ENIC_SETTING(enic, LRO))
-		netdev->features |= NETIF_F_GRO;
+	if (ENIC_SETTING(enic, RXCSUM))
+		netdev->hw_features |= NETIF_F_RXCSUM;
+
+	netdev->features |= netdev->hw_features;
+
 	if (using_dac)
 		netdev->features |= NETIF_F_HIGHDMA;
 
-	enic->csum_rx_enabled = ENIC_SETTING(enic, RXCSUM);
-
 	err = register_netdev(netdev);
 	if (err) {
 		dev_err(dev, "Cannot register net device, aborting\n");
diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
index f111a37..6e5c635 100644
--- a/drivers/net/enic/enic_res.c
+++ b/drivers/net/enic/enic_res.c
@@ -98,9 +98,9 @@ int enic_get_vnic_config(struct enic *enic)
 		"vNIC MAC addr %pM wq/rq %d/%d mtu %d\n",
 		enic->mac_addr, c->wq_desc_count, c->rq_desc_count, c->mtu);
 	dev_info(enic_get_dev(enic), "vNIC csum tx/rx %d/%d "
-		"tso/lro %d/%d intr timer %d usec rss %d\n",
+		"tso %d intr timer %d usec rss %d\n",
 		ENIC_SETTING(enic, TXCSUM), ENIC_SETTING(enic, RXCSUM),
-		ENIC_SETTING(enic, TSO), ENIC_SETTING(enic, LRO),
+		ENIC_SETTING(enic, TSO),
 		c->intr_timer_usec, ENIC_SETTING(enic, RSS));
 
 	return 0;
-- 
1.7.2.5

Re: [PATCH] net: enic: convert to hw_features

[roprabhu]

Thanks michal.

One small comment below,

On 4/7/11 5:43 AM, "Michał Mirosław" <mirq-linux@rere.qmqm.pl> wrote:

> As the driver uses GRO and not LRO, LRO settings are ignored anyway
> and are removed here to avoid confusion.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/net/enic/enic.h      |    1 -
>  drivers/net/enic/enic_main.c |   74 ++++-------------------------------------
>  drivers/net/enic/enic_res.c  |    4 +-
>  3 files changed, 10 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> index 178b94d..38b351c 100644
> --- a/drivers/net/enic/enic.h
> +++ b/drivers/net/enic/enic.h
> @@ -84,7 +84,6 @@ struct enic {
> unsigned int flags;
> unsigned int mc_count;
> unsigned int uc_count;
> - int csum_rx_enabled;
> u32 port_mtu;
> u32 rx_coalesce_usecs;
> u32 tx_coalesce_usecs;
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 9a3a027..b224551 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -251,56 +251,6 @@ static void enic_get_ethtool_stats(struct net_device
> *netdev,
> *(data++) = ((u64 *)&vstats->rx)[enic_rx_stats[i].offset];
>  }
>  
> -static u32 enic_get_rx_csum(struct net_device *netdev)
> -{
> - struct enic *enic = netdev_priv(netdev);
> - return enic->csum_rx_enabled;
> -}
> -
> -static int enic_set_rx_csum(struct net_device *netdev, u32 data)
> -{
> - struct enic *enic = netdev_priv(netdev);
> -
> - if (data && !ENIC_SETTING(enic, RXCSUM))
> -  return -EINVAL;
> -
> - enic->csum_rx_enabled = !!data;
> -
> - return 0;
> -}
> -
> -static int enic_set_tx_csum(struct net_device *netdev, u32 data)
> -{
> - struct enic *enic = netdev_priv(netdev);
> -
> - if (data && !ENIC_SETTING(enic, TXCSUM))
> -  return -EINVAL;
> -
> - if (data)
> -  netdev->features |= NETIF_F_HW_CSUM;
> - else
> -  netdev->features &= ~NETIF_F_HW_CSUM;
> -
> - return 0;
> -}
> -
> -static int enic_set_tso(struct net_device *netdev, u32 data)
> -{
> - struct enic *enic = netdev_priv(netdev);
> -
> - if (data && !ENIC_SETTING(enic, TSO))
> -  return -EINVAL;
> -
> - if (data)
> -  netdev->features |=
> -   NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN;
> - else
> -  netdev->features &=
> -   ~(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN);
> -
> - return 0;
> -}
> -
>  static u32 enic_get_msglevel(struct net_device *netdev)
>  {
> struct enic *enic = netdev_priv(netdev);
> @@ -388,17 +338,8 @@ static const struct ethtool_ops enic_ethtool_ops = {
> .get_strings = enic_get_strings,
> .get_sset_count = enic_get_sset_count,
> .get_ethtool_stats = enic_get_ethtool_stats,
> - .get_rx_csum = enic_get_rx_csum,
> - .set_rx_csum = enic_set_rx_csum,
> - .get_tx_csum = ethtool_op_get_tx_csum,
> - .set_tx_csum = enic_set_tx_csum,
> - .get_sg = ethtool_op_get_sg,
> - .set_sg = ethtool_op_set_sg,
> - .get_tso = ethtool_op_get_tso,
> - .set_tso = enic_set_tso,
> .get_coalesce = enic_get_coalesce,
> .set_coalesce = enic_set_coalesce,
> - .get_flags = ethtool_op_get_flags,
>  };
>  
>  static void enic_free_wq_buf(struct vnic_wq *wq, struct vnic_wq_buf *buf)
> @@ -1309,7 +1250,7 @@ static void enic_rq_indicate_buf(struct vnic_rq *rq,
> skb_put(skb, bytes_written);
> skb->protocol = eth_type_trans(skb, netdev);
>  
> -  if (enic->csum_rx_enabled && !csum_not_calc) {
> +  if ((netdev->features & NETIF_F_RXCSUM) && !csum_not_calc) {
> skb->csum = htons(checksum);
> skb->ip_summed = CHECKSUM_COMPLETE;
> }
> @@ -2438,17 +2379,18 @@ static int __devinit enic_probe(struct pci_dev *pdev,
> dev_info(dev, "loopback tag=0x%04x\n", enic->loop_tag);
> }
> if (ENIC_SETTING(enic, TXCSUM))
> -  netdev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
> +  netdev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM;
> if (ENIC_SETTING(enic, TSO))
> -  netdev->features |= NETIF_F_TSO |
> +  netdev->hw_features |= NETIF_F_TSO |
> NETIF_F_TSO6 | NETIF_F_TSO_ECN;
> - if (ENIC_SETTING(enic, LRO))
> -  netdev->features |= NETIF_F_GRO;
> + if (ENIC_SETTING(enic, RXCSUM))
> +  netdev->hw_features |= NETIF_F_RXCSUM;
> +
> + netdev->features |= netdev->hw_features;
> +
> if (using_dac)
> netdev->features |= NETIF_F_HIGHDMA;
>  
> - enic->csum_rx_enabled = ENIC_SETTING(enic, RXCSUM);
> -
> err = register_netdev(netdev);
> if (err) {
> dev_err(dev, "Cannot register net device, aborting\n");
> diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
> index f111a37..6e5c635 100644
> --- a/drivers/net/enic/enic_res.c
> +++ b/drivers/net/enic/enic_res.c
> @@ -98,9 +98,9 @@ int enic_get_vnic_config(struct enic *enic)
> "vNIC MAC addr %pM wq/rq %d/%d mtu %d\n",
> enic->mac_addr, c->wq_desc_count, c->rq_desc_count, c->mtu);
> dev_info(enic_get_dev(enic), "vNIC csum tx/rx %d/%d "
> -  "tso/lro %d/%d intr timer %d usec rss %d\n",
> +  "tso %d intr timer %d usec rss %d\n",
> ENIC_SETTING(enic, TXCSUM), ENIC_SETTING(enic, RXCSUM),
> -  ENIC_SETTING(enic, TSO), ENIC_SETTING(enic, LRO),
> +  ENIC_SETTING(enic, TSO),
> c->intr_timer_usec, ENIC_SETTING(enic, RSS));
>  
You are right about the driver using GRO and not LRO by default. But the
config entry from where enic gets this setting is still called LRO for
reasons out of the scope of the driver. Yes, I see that it can lead to
confusion. So, we will need to retain the ENIC_SETTING(enic, LRO) but fix
the name of that setting.  Thanks for pointing this out. We will fix this
and any other changes if required and resubmit.


> return 0;

Re: [PATCH] net: enic: convert to hw_features

[Michał Mirosław]

On Thu, Apr 07, 2011 at 12:52:45PM -0700, roprabhu wrote:
> Thanks michal.
> 
> One small comment below,
> 
> On 4/7/11 5:43 AM, "Michał Mirosław" <mirq-linux@rere.qmqm.pl> wrote:
> > As the driver uses GRO and not LRO, LRO settings are ignored anyway
> > and are removed here to avoid confusion.
[...]
> > diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
> > index f111a37..6e5c635 100644
> > --- a/drivers/net/enic/enic_res.c
> > +++ b/drivers/net/enic/enic_res.c
> > @@ -98,9 +98,9 @@ int enic_get_vnic_config(struct enic *enic)
> > "vNIC MAC addr %pM wq/rq %d/%d mtu %d\n",
> > enic->mac_addr, c->wq_desc_count, c->rq_desc_count, c->mtu);
> > dev_info(enic_get_dev(enic), "vNIC csum tx/rx %d/%d "
> > -  "tso/lro %d/%d intr timer %d usec rss %d\n",
> > +  "tso %d intr timer %d usec rss %d\n",
> > ENIC_SETTING(enic, TXCSUM), ENIC_SETTING(enic, RXCSUM),
> > -  ENIC_SETTING(enic, TSO), ENIC_SETTING(enic, LRO),
> > +  ENIC_SETTING(enic, TSO),
> > c->intr_timer_usec, ENIC_SETTING(enic, RSS));
> >  
> You are right about the driver using GRO and not LRO by default. But the
> config entry from where enic gets this setting is still called LRO for
> reasons out of the scope of the driver. Yes, I see that it can lead to
> confusion. So, we will need to retain the ENIC_SETTING(enic, LRO) but fix
> the name of that setting.  Thanks for pointing this out. We will fix this
> and any other changes if required and resubmit.

I removed this part because now network core is always turning on GRO
by default regardless of what driver sets in features at init time.
That's on purpose, since GRO is purely a software feature.

OTOH, if you want to forcibly disable GRO when LRO is not set in enic's
config, then you can do so in ndo_fix_features callback.

Best Regards,
Michał Mirosław

Re: [PATCH] net: enic: convert to hw_features

[roprabhu]

On 4/7/11 1:45 PM, "Michał Mirosław" <mirq-linux@rere.qmqm.pl> wrote:

> On Thu, Apr 07, 2011 at 12:52:45PM -0700, roprabhu wrote:
>> Thanks michal.
>> 
>> One small comment below,
>> 
>> On 4/7/11 5:43 AM, "Michał Mirosław" <mirq-linux@rere.qmqm.pl> wrote:
>>> As the driver uses GRO and not LRO, LRO settings are ignored anyway
>>> and are removed here to avoid confusion.
> [...]
>>> diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
>>> index f111a37..6e5c635 100644
>>> --- a/drivers/net/enic/enic_res.c
>>> +++ b/drivers/net/enic/enic_res.c
>>> @@ -98,9 +98,9 @@ int enic_get_vnic_config(struct enic *enic)
>>> "vNIC MAC addr %pM wq/rq %d/%d mtu %d\n",
>>> enic->mac_addr, c->wq_desc_count, c->rq_desc_count, c->mtu);
>>> dev_info(enic_get_dev(enic), "vNIC csum tx/rx %d/%d "
>>> -  "tso/lro %d/%d intr timer %d usec rss %d\n",
>>> +  "tso %d intr timer %d usec rss %d\n",
>>> ENIC_SETTING(enic, TXCSUM), ENIC_SETTING(enic, RXCSUM),
>>> -  ENIC_SETTING(enic, TSO), ENIC_SETTING(enic, LRO),
>>> +  ENIC_SETTING(enic, TSO),
>>> c->intr_timer_usec, ENIC_SETTING(enic, RSS));
>>>  
>> You are right about the driver using GRO and not LRO by default. But the
>> config entry from where enic gets this setting is still called LRO for
>> reasons out of the scope of the driver. Yes, I see that it can lead to
>> confusion. So, we will need to retain the ENIC_SETTING(enic, LRO) but fix
>> the name of that setting.  Thanks for pointing this out. We will fix this
>> and any other changes if required and resubmit.
> 
> I removed this part because now network core is always turning on GRO
> by default regardless of what driver sets in features at init time.
> That's on purpose, since GRO is purely a software feature.
> 
> OTOH, if you want to forcibly disable GRO when LRO is not set in enic's
> config, then you can do so in ndo_fix_features callback.
> 
Ok thanks michal. 

Re: [PATCH] net: enic: convert to hw_features

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Thu,  7 Apr 2011 14:43:48 +0200 (CEST)

> As the driver uses GRO and not LRO, LRO settings are ignored anyway
> and are removed here to avoid confusion.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied.