Re: [PATCH v4] net: bnx2x: convert to hw_features

["Vladislav Zolotarov" <vladz@broadcom.com>]

On Tue, 2011-04-12 at 07:07 -0700, Michał Mirosław wrote:
> On Tue, Apr 12, 2011 at 03:10:28PM +0300, Vladislav Zolotarov wrote:
> > 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 one about vlan_features? 

Yeah, this one.

> I'll just fix the comment in netdevice.h
> instead, since it might be not clear enough.

Could u still add this comment to bnx2x in your patch as well. It'll
just make these code section more clear regardless the netdevice.h
contents... ;)



> 
> > 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.
> 
> It might be possible to get rid of ndo_set_features, since it looks like
> the reset/recovery handler is doing unload/load itself. This needs more
> digging into the driver than this simple conversion.

Hmm... I don't get u here. Although the recovery handler does
unload/load itself if there has been an attempt to change features
during the recovery it won't be able to get applied until the whole
recovery process ends. So, this patch added the extra call for
netdev_update_features() right after we know that the recovery process
has successfully ended. So, could u, pls., explain exactly what do u
mean here by saying "It might be possible to get rid of
ndo_set_features"?

thanks,
vlad

> 
> Best Regards,
> Michał Mirosław
>