diff mbox

[v2] net: bnx2x: convert to hw_features

Message ID 1302531021.21065.79.camel@lb-tlvb-vladz
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Vladislav Zolotarov April 11, 2011, 2:10 p.m. UTC 
On Sun, 2011-04-10 at 08:35 -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.

Unfortunately, NACK again. See below, pls.

> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> [changes from v1: comment in ndo_fix_features callback]
> 
>  drivers/net/bnx2x/bnx2x_cmn.c     |   52 ++++++++++++++++++--
>  drivers/net/bnx2x/bnx2x_cmn.h     |    3 +
>  drivers/net/bnx2x/bnx2x_ethtool.c |   95 -------------------------------------
>  drivers/net/bnx2x/bnx2x_main.c    |   29 +++++++-----
>  4 files changed, 67 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> index e83ac6d..9691b67 100644
> --- a/drivers/net/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> @@ -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;

Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not
NETIF_F_RXCSUM?

> +
> +	return features;
> +}

In addition this function should ensure NETIF_F_IP_CSUM and
NETIF_F_IPV6_CSUM are changed together.

> +
> +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;
>  }

Since there is no set_rx_csum() anymore the above function has to handle
bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM) bits in the 'features'. 

>  
>  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..ffa0611 100644
> --- a/drivers/net/bnx2x/bnx2x_main.c
> +++ b/drivers/net/bnx2x/bnx2x_main.c
> @@ -7661,6 +7661,7 @@ exit_leader_reset:
>  	bp->is_leader = 0;
>  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
>  	smp_wmb();
> +	netdev_update_features(bp->dev);
>  	return rc;
>  }

Before I continue I'd like to clarify one thing: there is no sense to
call for netdev_update_features() if bnx2x_nic_load(), called right
before it, has failed as long as the following bnx2x_nic_load() that
will be called from the netdev_update_features() flow will also fail
(for the same reasons as the previous one). If bnx2x_nic_load() fails
for the certain NIC we actually shut this NIC down. So, the following
remarks will be based on the above statement.

netdev_update_features(bp->dev) should be called after a successful
"leader"'s call for a bnx2x_nic_load() and not as above. See the patch
below:

>  
> @@ -7759,6 +7760,7 @@ static void bnx2x_parity_recover(struct bnx2x *bp)
>  					bp->recovery_state =
>  						BNX2X_RECOVERY_DONE;
>  					smp_wmb();
> +					netdev_update_features(bp->dev);
>  					return;
>  				}
>  			}

Same here: netdev_update_features() should be called only if the
previous bnx2x_nic_load() call has succeeded.

> @@ -8954,6 +8956,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 +8996,9 @@ 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);
> +	netdev_update_features(bp->dev);
> +	return rc;
>  }

U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load()
has failed. It would also be nice if netdev_update_features() would
propagate the exit status of ndo_set_features() when ndo_set_features()
fails in the __netdev_update_features(). See the patch for the bnx2x
below:

@@ -8993,7 +8995,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 0;
+       else
+               return -EBUSY;
 }
 
 /* called with rtnl_lock */
>  
>  /* called with rtnl_lock */
> @@ -9304,6 +9309,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 +9437,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_HW_VLAN_TX;

hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are
currently configured in bnx2x_init_bp(). 

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

I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I
think it's better to correlate it with the USING_DAC_FLAG which is set
according to what is returned by 
dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)).

thanks,
vlad

>  
>  #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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Michał Mirosław April 11, 2011, 8:12 p.m. UTC | #1 
The v3 patch fixes missing LRO flag and ensures that netdev_update_features()
won't be called after failed bnx2x_nic_load(). More comments below.

On Mon, Apr 11, 2011 at 05:10:21PM +0300, Vladislav Zolotarov wrote:
> On Sun, 2011-04-10 at 08:35 -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.
> Unfortunately, NACK again. See below, pls.
[...]
> > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> > index e83ac6d..9691b67 100644
> > --- a/drivers/net/bnx2x/bnx2x_cmn.c
> > +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> > @@ -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)
> >  {
[...]
> > +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;
> Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not
> NETIF_F_RXCSUM?
[...]
> In addition this function should ensure NETIF_F_IP_CSUM and
> NETIF_F_IPV6_CSUM are changed together.
[...]
> > +int bnx2x_set_features(struct net_device *dev, u32 features)
[...]
> Since there is no set_rx_csum() anymore the above function has to handle
> bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM) bits in the 'features'. 

You seem to confuse TX checksum offloads (IP_CSUM,IPV6_CSUM) with
RX checksum offload (RXCSUM).

The driver doesn't touch hardware state on changes to checksum offloads
so they are independent - there's no point in adding artificial
dependencies here.

[...]
> > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> > index f3cf889..ffa0611 100644
> > --- a/drivers/net/bnx2x/bnx2x_main.c
> > +++ b/drivers/net/bnx2x/bnx2x_main.c
> > @@ -7661,6 +7661,7 @@ exit_leader_reset:
> >  	bp->is_leader = 0;
> >  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
> >  	smp_wmb();
> > +	netdev_update_features(bp->dev);
> >  	return rc;
> >  }
> 
> Before I continue I'd like to clarify one thing: there is no sense to
> call for netdev_update_features() if bnx2x_nic_load(), called right
> before it, has failed as long as the following bnx2x_nic_load() that
> will be called from the netdev_update_features() flow will also fail
> (for the same reasons as the previous one). If bnx2x_nic_load() fails
> for the certain NIC we actually shut this NIC down. So, the following
> remarks will be based on the above statement.

In all those cases, bnx2x_reload_if_running() will be called only when
LRO state is changed while there's a recovery in progress.

[...]
> U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load()
> has failed. It would also be nice if netdev_update_features() would
> propagate the exit status of ndo_set_features() when ndo_set_features()
> fails in the __netdev_update_features().

That's fixed in v3.

> See the patch for the bnx2x below:
> 
> @@ -8993,7 +8995,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 0;
> +       else
> +               return -EBUSY;
>  }

Hmm. I missed this part in the v3 patch. This clobbers bnx2x_nic_load()'s
error return, though.

> >  /* called with rtnl_lock */
> > @@ -9304,6 +9309,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 +9437,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_HW_VLAN_TX;
> hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are
> currently configured in bnx2x_init_bp(). 

GRO is enabled by core now. LRO is fixed in v3.

> > +	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;
> I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I
> think it's better to correlate it with the USING_DAC_FLAG which is set
> according to what is returned by 
> dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)).

dev->vlan_features get masked with dev->features and only then applied
to VLAN device.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladislav Zolotarov April 12, 2011, 7:26 a.m. UTC | #2 
On Mon, 2011-04-11 at 13:12 -0700, Michał Mirosław wrote:
> The v3 patch fixes missing LRO flag and ensures that netdev_update_features()
> won't be called after failed bnx2x_nic_load(). More comments below.

As long as there is v4 already I'll comment it and skip v3. See a few
comments on your comments below. ;)

> 
> On Mon, Apr 11, 2011 at 05:10:21PM +0300, Vladislav Zolotarov wrote:
> > On Sun, 2011-04-10 at 08:35 -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.
> > Unfortunately, NACK again. See below, pls.
> [...]
> > > diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
> > > index e83ac6d..9691b67 100644
> > > --- a/drivers/net/bnx2x/bnx2x_cmn.c
> > > +++ b/drivers/net/bnx2x/bnx2x_cmn.c
> > > @@ -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)
> > >  {
> [...]
> > > +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;
> > Shouldn't it be (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) and not
> > NETIF_F_RXCSUM?
> [...]
> > In addition this function should ensure NETIF_F_IP_CSUM and
> > NETIF_F_IPV6_CSUM are changed together.
> [...]
> > > +int bnx2x_set_features(struct net_device *dev, u32 features)
> [...]
> > Since there is no set_rx_csum() anymore the above function has to handle
> > bp->rx_csum namely correlate it with (NETIF_F_IP_CSUM |
> > NETIF_F_IPV6_CSUM) bits in the 'features'. 
> 
> You seem to confuse TX checksum offloads (IP_CSUM,IPV6_CSUM) with
> RX checksum offload (RXCSUM).

U r right. My bad. However u forgot to add RXCSUM to hw_features in v2
but I see it fixed in v4.

> 
> The driver doesn't touch hardware state on changes to checksum offloads
> so they are independent - there's no point in adding artificial
> dependencies here.

Considering Tx csum offloads u are right but this is not true regarding
the Rx csum offload and this is what I meant above. I see that v4
properly handles it now. Sorry for a confusion.

> 
> [...]
> > > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> > > index f3cf889..ffa0611 100644
> > > --- a/drivers/net/bnx2x/bnx2x_main.c
> > > +++ b/drivers/net/bnx2x/bnx2x_main.c
> > > @@ -7661,6 +7661,7 @@ exit_leader_reset:
> > >  	bp->is_leader = 0;
> > >  	bnx2x_release_hw_lock(bp, HW_LOCK_RESOURCE_RESERVED_08);
> > >  	smp_wmb();
> > > +	netdev_update_features(bp->dev);
> > >  	return rc;
> > >  }
> > 
> > Before I continue I'd like to clarify one thing: there is no sense to
> > call for netdev_update_features() if bnx2x_nic_load(), called right
> > before it, has failed as long as the following bnx2x_nic_load() that
> > will be called from the netdev_update_features() flow will also fail
> > (for the same reasons as the previous one). If bnx2x_nic_load() fails
> > for the certain NIC we actually shut this NIC down. So, the following
> > remarks will be based on the above statement.
> 
> In all those cases, bnx2x_reload_if_running() will be called only when
> LRO state is changed while there's a recovery in progress.

Hmmm... And what about all other features from hw_features? What if they
have changed (in wanted_features) while recovery was in progress?
According to the __netdev_update_features() code it will invoke
ndo_set_features() in these cases either. Do I miss something here?

> 
> [...]
> > U shouldn't call for netdev_update_features(bp->dev) if bnx2x_nic_load()
> > has failed. It would also be nice if netdev_update_features() would
> > propagate the exit status of ndo_set_features() when ndo_set_features()
> > fails in the __netdev_update_features().
> 
> That's fixed in v3.

Not everything. See below.

> 
> > See the patch for the bnx2x below:
> > 
> > @@ -8993,7 +8995,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 0;
> > +       else
> > +               return -EBUSY;
> >  }
> 
> Hmm. I missed this part in the v3 patch. This clobbers bnx2x_nic_load()'s
> error return, though.

Exactly! Quoting my remark above: "It would also be nice if
netdev_update_features() would propagate the exit status of
ndo_set_features() when ndo_set_features() fails in the
__netdev_update_features()." Could u comment on this, pls.

> 
> > >  /* called with rtnl_lock */
> > > @@ -9304,6 +9309,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 +9437,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_HW_VLAN_TX;
> > hw_features are missing NETIF_F_GRO and NETIF_F_LRO flags that are
> > currently configured in bnx2x_init_bp(). 
> 
> GRO is enabled by core now. LRO is fixed in v3.

Got it. Thanks.

> 
> > > +	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;
> > I'm not sure if it's safe to set NETIF_F_HIGHDMA unconditionally. I
> > think it's better to correlate it with the USING_DAC_FLAG which is set
> > according to what is returned by 
> > dma_set_mask(&pdev->dev, DMA_BIT_MASK(64)).
> 
> dev->vlan_features get masked with dev->features and only then applied
> to VLAN device.

Ok. However, could, pls., quote the above sentence of yours as a comment
for this code line? ;)

See my further comments for v4.

thanks,
vlad

> 
> Best Regards,
> Michał Mirosław
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladislav Zolotarov April 12, 2011, 7:46 a.m. UTC | #3 
> > In all those cases, bnx2x_reload_if_running() will be called only when
> > LRO state is changed while there's a recovery in progress.
> 
> Hmmm... And what about all other features from hw_features? What if they
> have changed (in wanted_features) while recovery was in progress?
> According to the __netdev_update_features() code it will invoke
> ndo_set_features() in these cases either. Do I miss something here?

I think I understood what u meant. So, yes, if the bnx2x_nic_load()
called only if TPA_ENABLED_FLAG in bp->flags has changed. And this can
happen if either NETIF_F_LRO has changed while NETIF_F_RXCSUM was set or
if NETIF_F_LRO was set and NETIF_F_RXCSUM is being cleared.

thanks,
vlad


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/bnx2x/bnx2x_main.c
b/drivers/net/bnx2x/bnx2x_main.c
index f3cf889..71e7818 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -7728,6 +7728,8 @@  static void bnx2x_parity_recover(struct bnx2x *bp)
 						return;
 					}
 
+					netdev_update_features(bp->dev);
+
					return;
 				}