Re: [PATCHv3 net-next 1/3] sunvnet: upgrade to VIO protocol version 1.6

[David L Stevens <david.stevens@oracle.com>]

On 09/13/2014 04:18 PM, David Miller wrote:
> From: David L Stevens <david.stevens@oracle.com>

> Is this really applicable to devices other than sunvnet?  Just keep
> this attribute in the sunvnet driver "struct vnet_port" private state.

Yes, I'll move it.

> 
>> +	/* v1.6 and higher, ACK with desired, supported mode, or NACK */
>> +	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
>  ...
>> +	if (vio->ver.major <= 1 && vio->ver.minor < 2)
>  ...
>> +	/* for version < 1.2, VIO_DRING_MODE = 0x3 and no bitmask */
>> +	if ((vio->ver.major <= 1 &&  vio->ver.minor < 2) &&
> ...
>> +	if (vio->ver.major == 1 && vio->ver.minor < 3) {
> 	...
>> +	} else if (vio->ver.major == 1 && vio->ver.minor == 3) {
> 	...
>> +	} else {
> 	...
>> +	}
>  ...
>> +	/* for version >= 1.6, ACK packet mode we support */
>> +	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
> 
> These version tests give me a headache, and some of them accept
> impossible things like major version zero.  If we only have major
> version 1 and later in our version lists, testing things like "major
> <= 1" makes no sense at all.
> 
> That last test quoted above definitely looks wrong, it should
> be testing "major >= 1" if anything if it is trying to test
> "version >= 1.6"

Yes, that'd break at > v2.0 -- good catch.

> Sadly, I expect that we'll have more and more of these version tests
> over time.  So why not make some generic helpers in the VIO or LDC
> layer?
> 
> static inline bool vio_version_before(struct vio_driver_state *vio,
> 				      u16 major, u16 minor)
> {
> 	u32 have = (u32)vio->major << 16 | vio->minor;
> 	u32 want = (u32)major << 16 | minor;
> 
> 	return have < want;
> }
> static inline bool vio_version_after_eq(struct vio_driver_state *vio,
> 					u16 major, u16 minor)
> {
> 	u32 have = (u32)vio->major << 16 | vio->minor;
> 	u32 want = (u32)major << 16 | minor;
> 
> 	return have >= want;
> }
> 
> Something like that.

Sure. I was thinking about something like:

#define VIO_VER(major, minor)	(((major)<<16)|(minor))

change the version struct to a 32-bit int, and do things like:

	if (vio->ver > VIO_VER(1,6)) {

unless you have a preference. (?)

						+-DLS