| 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
| ||
|
Date: Tue, 12 Apr 2011 15:10:28 +0300
From: "Vladislav Zolotarov" <vladz@...adcom.com>
To: Michał Mirosław <mirq-linux@...e.qmqm.pl>
cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Eilon Greenstein" <eilong@...adcom.com>
Subject: Re: [PATCH v4] net: bnx2x: convert to hw_features
On Mon, 2011-04-11 at 13:26 -0700, Michał Mirosław wrote:
> Since ndo_fix_features callback is postponing features change when
> bp->recovery_state != BNX2X_RECOVERY_DONE, netdev_update_features()
> has to be called again when this condition changes. Previously,
> ethtool_ops->set_flags callback returned -EBUSY in that case
> (it's not possible in the new model).
ACK (with reservations). ;)
Could u, pls., just add this comment I've asked for in the previous
e-mail?
The things I first thought to comment on are:
- Removing TPA_ENABLED_FLAG the similar way u've removed the
bp->rx_csum.
- Merging the code handling 'features' in bnx2x_init_bp() with the
similar code in bnx2x_init_dev().
However I think it would be right if we clear our mess by ourselves and
that u have already done much enough... ;)
I've run our standard test suite (which in particular heavily tests the
RX_CSUM and LRO flags toggling) on this patch and it passed it.
Thanks a lot, Michal.
vlad
>
> Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> ---
> v4: - complete bp->rx_csum -> NETIF_F_RXCSUM conversion
> - add check for failed ndo_set_features in ndo_open callback
> v3: - include NETIF_F_LRO in hw_features
> - don't call netdev_update_features() if bnx2x_nic_load() failed
> v2: - comment in ndo_fix_features callback
> ---
> drivers/net/bnx2x/bnx2x.h | 1 -
> drivers/net/bnx2x/bnx2x_cmn.c | 54 +++++++++++++++++++--
> drivers/net/bnx2x/bnx2x_cmn.h | 3 +
> drivers/net/bnx2x/bnx2x_ethtool.c | 95 -------------------------------------
> drivers/net/bnx2x/bnx2x_main.c | 41 +++++++++-------
> 5 files changed, 75 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
> index b7ff87b..fefd1d5 100644
> --- a/drivers/net/bnx2x/bnx2x.h
> +++ b/drivers/net/bnx2x/bnx2x.h
> @@ -918,7 +918,6 @@ struct bnx2x {
>
> int tx_ring_size;
>
> - u32 rx_csum;
> /* L2 header size + 2*VLANs (8 bytes) + LLC SNAP (8 bytes) */
> #define ETH_OVREHEAD (ETH_HLEN + 8 + 8)
> #define ETH_MIN_PACKET_SIZE 60
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index e83ac6d..7f49cf4 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -640,7 +640,7 @@ reuse_rx:
>
> skb_checksum_none_assert(skb);
>
> - if (bp->rx_csum) {
> + if (bp->dev->features & NETIF_F_RXCSUM) {
> if (likely(BNX2X_RX_CSUM_OK(cqe)))
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> else
> @@ -2443,11 +2443,21 @@ alloc_err:
>
> }
>
> +static int bnx2x_reload_if_running(struct net_device *dev)
> +{
> + struct bnx2x *bp = netdev_priv(dev);
> +
> + if (unlikely(!netif_running(dev)))
> + return 0;
> +
> + bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> + return bnx2x_nic_load(bp, LOAD_NORMAL);
> +}
> +
> /* called with rtnl_lock */
> int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
> {
> struct bnx2x *bp = netdev_priv(dev);
> - int rc = 0;
>
> if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> @@ -2464,12 +2474,44 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
> */
> dev->mtu = new_mtu;
>
> - if (netif_running(dev)) {
> - bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> - rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> + return bnx2x_reload_if_running(dev);
> +}
> +
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features)
> +{
> + struct bnx2x *bp = netdev_priv(dev);
> +
> + if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> + netdev_err(dev, "Handling parity error recovery. Try again later\n");
> +
> + /* Don't allow bnx2x_set_features() to be called now. */
> + return dev->features;
> + }
> +
> + /* TPA requires Rx CSUM offloading */
> + if (!(features & NETIF_F_RXCSUM) || bp->disable_tpa)
> + features &= ~NETIF_F_LRO;
> +
> + return features;
> +}
> +
> +int bnx2x_set_features(struct net_device *dev, u32 features)
> +{
> + struct bnx2x *bp = netdev_priv(dev);
> + u32 flags = bp->flags;
> +
> + if (features & NETIF_F_LRO)
> + flags |= TPA_ENABLE_FLAG;
> + else
> + flags &= ~TPA_ENABLE_FLAG;
> +
> + if (flags ^ bp->flags) {
> + bp->flags = flags;
> +
> + return bnx2x_reload_if_running(dev);
> }
>
> - return rc;
> + return 0;
> }
>
> void bnx2x_tx_timeout(struct net_device *dev)
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.h b/drivers/net/bnx2x/bnx2x_cmn.h
> index 775fef0..1cdab69 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.h
> +++ b/drivers/net/bnx2x/bnx2x_cmn.h
> @@ -431,6 +431,9 @@ void bnx2x_free_mem_bp(struct bnx2x *bp);
> */
> int bnx2x_change_mtu(struct net_device *dev, int new_mtu);
>
> +u32 bnx2x_fix_features(struct net_device *dev, u32 features);
> +int bnx2x_set_features(struct net_device *dev, u32 features);
> +
> /**
> * tx timeout netdev callback
> *
> diff --git a/drivers/net/bnx2x/bnx2x_ethtool.c b/drivers/net/bnx2x/bnx2x_ethtool.c
> index 1479994..ad7d91e 100644
> --- a/drivers/net/bnx2x/bnx2x_ethtool.c
> +++ b/drivers/net/bnx2x/bnx2x_ethtool.c
> @@ -1299,91 +1299,6 @@ static int bnx2x_set_pauseparam(struct net_device *dev,
> return 0;
> }
>
> -static int bnx2x_set_flags(struct net_device *dev, u32 data)
> -{
> - struct bnx2x *bp = netdev_priv(dev);
> - int changed = 0;
> - int rc = 0;
> -
> - if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> - printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> - return -EAGAIN;
> - }
> -
> - if (!(data & ETH_FLAG_RXVLAN))
> - return -EINVAL;
> -
> - if ((data & ETH_FLAG_LRO) && bp->rx_csum && bp->disable_tpa)
> - return -EINVAL;
> -
> - rc = ethtool_op_set_flags(dev, data, ETH_FLAG_LRO | ETH_FLAG_RXVLAN |
> - ETH_FLAG_TXVLAN | ETH_FLAG_RXHASH);
> - if (rc)
> - return rc;
> -
> - /* TPA requires Rx CSUM offloading */
> - if ((data & ETH_FLAG_LRO) && bp->rx_csum) {
> - if (!(bp->flags & TPA_ENABLE_FLAG)) {
> - bp->flags |= TPA_ENABLE_FLAG;
> - changed = 1;
> - }
> - } else if (bp->flags & TPA_ENABLE_FLAG) {
> - dev->features &= ~NETIF_F_LRO;
> - bp->flags &= ~TPA_ENABLE_FLAG;
> - changed = 1;
> - }
> -
> - if (changed && netif_running(dev)) {
> - bnx2x_nic_unload(bp, UNLOAD_NORMAL);
> - rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> - }
> -
> - return rc;
> -}
> -
> -static u32 bnx2x_get_rx_csum(struct net_device *dev)
> -{
> - struct bnx2x *bp = netdev_priv(dev);
> -
> - return bp->rx_csum;
> -}
> -
> -static int bnx2x_set_rx_csum(struct net_device *dev, u32 data)
> -{
> - struct bnx2x *bp = netdev_priv(dev);
> - int rc = 0;
> -
> - if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> - printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> - return -EAGAIN;
> - }
> -
> - bp->rx_csum = data;
> -
> - /* Disable TPA, when Rx CSUM is disabled. Otherwise all
> - TPA'ed packets will be discarded due to wrong TCP CSUM */
> - if (!data) {
> - u32 flags = ethtool_op_get_flags(dev);
> -
> - rc = bnx2x_set_flags(dev, (flags & ~ETH_FLAG_LRO));
> - }
> -
> - return rc;
> -}
> -
> -static int bnx2x_set_tso(struct net_device *dev, u32 data)
> -{
> - if (data) {
> - dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> - dev->features |= NETIF_F_TSO6;
> - } else {
> - dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
> - dev->features &= ~NETIF_F_TSO6;
> - }
> -
> - return 0;
> -}
> -
> static const struct {
> char string[ETH_GSTRING_LEN];
> } bnx2x_tests_str_arr[BNX2X_NUM_TESTS] = {
> @@ -2207,16 +2122,6 @@ static const struct ethtool_ops bnx2x_ethtool_ops = {
> .set_ringparam = bnx2x_set_ringparam,
> .get_pauseparam = bnx2x_get_pauseparam,
> .set_pauseparam = bnx2x_set_pauseparam,
> - .get_rx_csum = bnx2x_get_rx_csum,
> - .set_rx_csum = bnx2x_set_rx_csum,
> - .get_tx_csum = ethtool_op_get_tx_csum,
> - .set_tx_csum = ethtool_op_set_tx_hw_csum,
> - .set_flags = bnx2x_set_flags,
> - .get_flags = ethtool_op_get_flags,
> - .get_sg = ethtool_op_get_sg,
> - .set_sg = ethtool_op_set_sg,
> - .get_tso = ethtool_op_get_tso,
> - .set_tso = bnx2x_set_tso,
> .self_test = bnx2x_self_test,
> .get_sset_count = bnx2x_get_sset_count,
> .get_strings = bnx2x_get_strings,
> diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> index f3cf889..5fd7cbb 100644
> --- a/drivers/net/bnx2x/bnx2x_main.c
> +++ b/drivers/net/bnx2x/bnx2x_main.c
> @@ -7728,6 +7728,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
> return;
> }
>
> + netdev_update_features(bp->dev);
> return;
> }
> } else { /* non-leader */
> @@ -7755,10 +7756,12 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
> * the "process kill". It's an exit
> * point for a non-leader.
> */
> - bnx2x_nic_load(bp, LOAD_NORMAL);
> + int rc = bnx2x_nic_load(bp, LOAD_NORMAL);
> bp->recovery_state =
> BNX2X_RECOVERY_DONE;
> smp_wmb();
> + if (!rc)
> + netdev_update_features(bp->dev);
> return;
> }
> }
> @@ -8904,8 +8907,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
> bp->multi_mode = multi_mode;
> bp->int_mode = int_mode;
>
> - bp->dev->features |= NETIF_F_GRO;
> -
> /* Set TPA flags */
> if (disable_tpa) {
> bp->flags &= ~TPA_ENABLE_FLAG;
> @@ -8925,8 +8926,6 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
>
> bp->tx_ring_size = MAX_TX_AVAIL;
>
> - bp->rx_csum = 1;
> -
> /* make sure that the numbers are in the right granularity */
> bp->tx_ticks = (50 / BNX2X_BTR) * BNX2X_BTR;
> bp->rx_ticks = (25 / BNX2X_BTR) * BNX2X_BTR;
> @@ -8954,6 +8953,7 @@ static int __devinit bnx2x_init_bp(struct bnx2x *bp)
> static int bnx2x_open(struct net_device *dev)
> {
> struct bnx2x *bp = netdev_priv(dev);
> + int rc;
>
> netif_carrier_off(dev);
>
> @@ -8993,7 +8993,14 @@ static int bnx2x_open(struct net_device *dev)
>
> bp->recovery_state = BNX2X_RECOVERY_DONE;
>
> - return bnx2x_nic_load(bp, LOAD_OPEN);
> + rc = bnx2x_nic_load(bp, LOAD_OPEN);
> + if (!rc) {
> + netdev_update_features(bp->dev);
> + if (bp->state != BNX2X_STATE_OPEN)
> + return -EBUSY;
> + }
> +
> + return rc;
> }
>
> /* called with rtnl_lock */
> @@ -9304,6 +9311,8 @@ static const struct net_device_ops bnx2x_netdev_ops = {
> .ndo_validate_addr = eth_validate_addr,
> .ndo_do_ioctl = bnx2x_ioctl,
> .ndo_change_mtu = bnx2x_change_mtu,
> + .ndo_fix_features = bnx2x_fix_features,
> + .ndo_set_features = bnx2x_set_features,
> .ndo_tx_timeout = bnx2x_tx_timeout,
> #ifdef CONFIG_NET_POLL_CONTROLLER
> .ndo_poll_controller = poll_bnx2x,
> @@ -9430,20 +9439,18 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
>
> dev->netdev_ops = &bnx2x_netdev_ops;
> bnx2x_set_ethtool_ops(dev);
> - dev->features |= NETIF_F_SG;
> - dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> +
> if (bp->flags & USING_DAC_FLAG)
> dev->features |= NETIF_F_HIGHDMA;
> - dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> - dev->features |= NETIF_F_TSO6;
> - dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
>
> - dev->vlan_features |= NETIF_F_SG;
> - dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> - if (bp->flags & USING_DAC_FLAG)
> - dev->vlan_features |= NETIF_F_HIGHDMA;
> - dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> - dev->vlan_features |= NETIF_F_TSO6;
> + dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> + NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_HW_VLAN_TX;
> +
> + dev->features |= dev->hw_features | NETIF_F_HW_VLAN_RX;
> +
> + dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> + NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_HIGHDMA;
>
> #ifdef BCM_DCBNL
> dev->dcbnl_ops = &bnx2x_dcbnl_ops;
--
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