Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions

[Vitaly Kuznetsov]

Andrea Parri <parri.andrea@xxxxxxxxx> writes:

> The technique used to get the next VMBus version seems increasisly
> clumsy as the number of VMBus versions increases.  Performance is
> not a concern since this is only done once during system boot; it's
> just that we'll end up with more lines of code than is really needed.
>
> As an alternative, introduce a table with the version numbers listed
> in order (from the most recent to the oldest).  vmbus_connect() loops
> through the versions listed in the table until it gets an accepted
> connection or gets to the end of the table (invalid version).
>
> Suggested-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> Signed-off-by: Andrea Parri <parri.andrea@xxxxxxxxx>
> ---
>  drivers/hv/connection.c | 51 +++++++++++++++--------------------------
>  include/linux/hyperv.h  |  2 --
>  2 files changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 6e4c015783ffc..90a32c9d79403 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -40,29 +40,19 @@ EXPORT_SYMBOL_GPL(vmbus_connection);
>  __u32 vmbus_proto_version;
>  EXPORT_SYMBOL_GPL(vmbus_proto_version);
>  
> -static __u32 vmbus_get_next_version(__u32 current_version)
> -{
> -	switch (current_version) {
> -	case (VERSION_WIN7):
> -		return VERSION_WS2008;
> -
> -	case (VERSION_WIN8):
> -		return VERSION_WIN7;
> -
> -	case (VERSION_WIN8_1):
> -		return VERSION_WIN8;
> -
> -	case (VERSION_WIN10):
> -		return VERSION_WIN8_1;
> -
> -	case (VERSION_WIN10_V5):
> -		return VERSION_WIN10;
> -
> -	case (VERSION_WS2008):
> -	default:
> -		return VERSION_INVAL;
> -	}
> -}
> +/*
> + * Table of VMBus versions listed from newest to oldest; the table
> + * must terminate with VERSION_INVAL.
> + */
> +__u32 vmbus_versions[] = {
> +	VERSION_WIN10_V5,
> +	VERSION_WIN10,
> +	VERSION_WIN8_1,
> +	VERSION_WIN8,
> +	VERSION_WIN7,
> +	VERSION_WS2008,
> +	VERSION_INVAL
> +};
>  
>  int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
>  {
> @@ -169,8 +159,8 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
>   */
>  int vmbus_connect(void)
>  {
> -	int ret = 0;
>  	struct vmbus_channel_msginfo *msginfo = NULL;
> +	int i, ret = 0;
>  	__u32 version;
>  
>  	/* Initialize the vmbus connection */
> @@ -244,21 +234,18 @@ int vmbus_connect(void)
>  	 * version.
>  	 */
>  
> -	version = VERSION_CURRENT;
> +	for (i = 0; ; i++) {
> +		version = vmbus_versions[i];
> +		if (version == VERSION_INVAL)
> +			goto cleanup;

If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
this code look more natural.
>  
> -	do {
>  		ret = vmbus_negotiate_version(msginfo, version);
>  		if (ret == -ETIMEDOUT)
>  			goto cleanup;
>  
>  		if (vmbus_connection.conn_state == CONNECTED)
>  			break;
> -
> -		version = vmbus_get_next_version(version);
> -	} while (version != VERSION_INVAL);
> -
> -	if (version == VERSION_INVAL)
> -		goto cleanup;
> +	}
>  
>  	vmbus_proto_version = version;
>  	pr_info("Vmbus version:%d.%d\n",
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index b4a017093b697..7073f1eb3618c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -194,8 +194,6 @@ static inline u32 hv_get_avail_to_write_percent(
>  
>  #define VERSION_INVAL -1
>  
> -#define VERSION_CURRENT VERSION_WIN10_V5
> -
>  /* Make maximum size of pipe payload of 16K */
>  #define MAX_PIPE_DATA_PAYLOAD		(sizeof(u8) * 16384)

-- 
Vitaly