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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5414FFDD.9050509@oracle.com>
Date:	Sat, 13 Sep 2014 22:39:25 -0400
From:	David L Stevens <david.stevens@...cle.com>
To:	David Miller <davem@...emloft.net>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCHv3 net-next 1/3] sunvnet: upgrade to VIO protocol version
 1.6



On 09/13/2014 04:18 PM, David Miller wrote:
> From: David L Stevens <david.stevens@...cle.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
--
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