Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Alexander Duyck <alexander.h.duyck@linux.intel.com>]

On Wed, 2020-03-04 at 10:00 -0800, Jakub Kicinski wrote:
> On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:
> > Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> > is de facto an array so that this block could be replaced by a loop like
> > 
> > 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> > 	const u32 *values = &coalesce->rx_coalesce_usecs;
> > 
> > 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> > 		if (values[i] && !(supported_types & BIT(i)))
> > 			return false;
> > 
> > and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> > of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.
> 
> I kind of prefer the greppability over the saved 40 lines :(
> But I'm happy to change if we get more votes for the more concise
> version. Or perhaps the Intel version with the warnings printed.

I agree that it would make more sense to replace the types with an enum
definition, and then use the enum to define bits to be used by the
drivers.

> > > +	return !dev->ethtool_ops->coalesce_types ||
> > > +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> > > +}  
> > 
> > I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
> > beginning of the function so that we avoid calculating the bitmap if we
> > are not going to check it anyway.
> 
> Good point!

So one thing I just wanted to point out. The used_types won't necessarily
be correct because it is only actually checking for non-zero types. There
are some of these values where a zero is a valid input and the driver will
accept it, such as rx_coalesce_usecs for ixgbe. As such we might want to
rename the value to nonzero_types instead of used_types.