Date:	Tue, 12 Apr 2011 10:26:48 +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 v2] net: bnx2x: convert to hw_features

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

Powered by blists - more mailing lists