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: <C9C3641D.23F23%roprabhu@cisco.com>
Date:	Thu, 07 Apr 2011 12:52:45 -0700
From:	roprabhu <roprabhu@...co.com>
To:	Michał Mirosław 
	<mirq-linux@...e.qmqm.pl>, <netdev@...r.kernel.org>
CC:	Christian Benvenuti <benve@...co.com>,
	Vasanthy Kolluri <vkolluri@...co.com>,
	David Wang <dwang2@...co.com>
Subject: Re: [PATCH] net: enic: convert to hw_features

Thanks michal.

One small comment below,

On 4/7/11 5:43 AM, "Michał Mirosław" <mirq-linux@...e.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@...e.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;

--
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ