Date:	Mon, 11 Apr 2011 22:12:25 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Vladislav Zolotarov <vladz@...adcom.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Eilon Greenstein <eilong@...adcom.com>
Subject: Re: [PATCH v2] net: bnx2x: convert to hw_features

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

Powered by blists - more mailing lists