RE: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiationcode for util services

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@xxxxxxxxx]
> Sent: Wednesday, September 04, 2013 8:07 AM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/1] Drivers: hv: util: Fix a bug in version negotiation code for
> util services
> 
> On Wed, Sep 04, Olaf Hering wrote:
> 
> > I suggest to let callers deal with error handling. Also as a cleanup,
> > vmbus_prep_negotiate_resp does not make use of the negop passed to it.
> > So that arg can be removed.
> 
> Simplify vmbus_prep_negotiate_resp. If we take the optimistic approach
> that negotiation will not fail for any of the callers of
> vmbus_prep_negotiate_resp this patch on top of yours should fix 2008 and
> 2012r2.
Thanks Olaf. I would prefer that we explicitly fail the negotiations than assume that it will succeed.
Essentially, I want the same behavior as what we had before but only for the final version being
negotiated. If it is ok with you, I can spin up the patch and send it out.

Regards,

K. Y

> 
> Olaf
> 
> ---
>  drivers/hv/channel_mgmt.c | 22 +++++++++-------------
>  drivers/hv/hv_kvp.c       |  8 ++++----
>  drivers/hv/hv_snapshot.c  |  3 +--
>  drivers/hv/hv_util.c      |  7 +++----
>  include/linux/hyperv.h    |  4 +---
>  5 files changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 12ec8c8..dcaad3e 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -41,7 +41,6 @@ struct vmbus_channel_message_table_entry {
>  /**
>   * vmbus_prep_negotiate_resp() - Create default response for Hyper-V
> Negotiate message
>   * @icmsghdrp: Pointer to msg header structure
> - * @icmsg_negotiate: Pointer to negotiate message structure
>   * @buf: Raw buffer channel data
>   *
>   * @icmsghdrp is of type &struct icmsg_hdr.
> @@ -54,10 +53,10 @@ struct vmbus_channel_message_table_entry {
>   *
>   * Mainly used by Hyper-V drivers.
>   */
> -bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> -				struct icmsg_negotiate *negop, u8 *buf,
> +bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf,
>  				int fw_version, int srv_version)
>  {
> +	struct icmsg_negotiate *negop;
>  	int icframe_major, icframe_minor;
>  	int icmsg_major, icmsg_minor;
>  	int fw_major, fw_minor;
> @@ -65,7 +64,6 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
>  	int i;
>  	bool found_match = false;
> 
> -	icmsghdrp->icmsgsize = 0x10;
>  	fw_major = (fw_version >> 16);
>  	fw_minor = (fw_version & 0xFFFF);
> 
> @@ -116,19 +114,17 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
>  	 * version numbers we can support.
>  	 */
> 
> -fw_error:
> -	if (!found_match) {
> -		negop->icframe_vercnt = 0;
> -		negop->icmsg_vercnt = 0;
> -	} else {
> +	if (found_match) {
> +		icmsghdrp->icmsgsize = 0x10;
>  		negop->icframe_vercnt = 1;
>  		negop->icmsg_vercnt = 1;
> +		negop->icversion_data[0].major = icframe_major;
> +		negop->icversion_data[0].minor = icframe_minor;
> +		negop->icversion_data[1].major = icmsg_major;
> +		negop->icversion_data[1].minor = icmsg_minor;
>  	}
> 
> -	negop->icversion_data[0].major = icframe_major;
> -	negop->icversion_data[0].minor = icframe_minor;
> -	negop->icversion_data[1].major = icmsg_major;
> -	negop->icversion_data[1].minor = icmsg_minor;
> +fw_error:
>  	return found_match;
>  }
> 
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 5312720..31e338a 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -584,7 +584,6 @@ void hv_kvp_onchannelcallback(void *context)
>  	struct hv_kvp_msg *kvp_msg;
> 
>  	struct icmsg_hdr *icmsghdrp;
> -	struct icmsg_negotiate *negop = NULL;
> 
>  	if (kvp_transaction.active) {
>  		/*
> @@ -607,14 +606,15 @@ void hv_kvp_onchannelcallback(void *context)
>  			 * We start with win8 version and if the host cannot
>  			 * support that we use the previous version.
>  			 */
> -			if (vmbus_prep_negotiate_resp(icmsghdrp, negop,
> +			if (vmbus_prep_negotiate_resp(icmsghdrp,
>  				 recv_buffer, UTIL_FW_MAJOR_MINOR,
>  				 WIN8_SRV_MAJOR_MINOR))
>  				goto done;
> 
> -			vmbus_prep_negotiate_resp(icmsghdrp, negop,
> +			if (vmbus_prep_negotiate_resp(icmsghdrp,
>  				 recv_buffer, UTIL_FW_MAJOR_MINOR,
> -				 WIN7_SRV_MAJOR_MINOR);
> +				 WIN7_SRV_MAJOR_MINOR))
> +				goto done;
> 
>  		} else {
>  			kvp_msg = (struct hv_kvp_msg *)&recv_buffer[
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index e4572f3..51e8203 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -170,7 +170,6 @@ void hv_vss_onchannelcallback(void *context)
> 
> 
>  	struct icmsg_hdr *icmsghdrp;
> -	struct icmsg_negotiate *negop = NULL;
> 
>  	if (vss_transaction.active) {
>  		/*
> @@ -189,7 +188,7 @@ void hv_vss_onchannelcallback(void *context)
>  			sizeof(struct vmbuspipe_hdr)];
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			vmbus_prep_negotiate_resp(icmsghdrp, negop,
> +			vmbus_prep_negotiate_resp(icmsghdrp,
>  				 recv_buffer, UTIL_FW_MAJOR_MINOR,
>  				 VSS_MAJOR_MINOR);
>  		} else {
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index c16164d..01d7b17 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -88,7 +88,6 @@ static void shutdown_onchannelcallback(void *context)
>  	struct shutdown_msg_data *shutdown_msg;
> 
>  	struct icmsg_hdr *icmsghdrp;
> -	struct icmsg_negotiate *negop = NULL;
> 
>  	vmbus_recvpacket(channel, shut_txf_buf,
>  			 PAGE_SIZE, &recvlen, &requestid);
> @@ -98,7 +97,7 @@ static void shutdown_onchannelcallback(void *context)
>  			sizeof(struct vmbuspipe_hdr)];
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			vmbus_prep_negotiate_resp(icmsghdrp, negop,
> +			vmbus_prep_negotiate_resp(icmsghdrp,
>  					shut_txf_buf, UTIL_FW_MAJOR_MINOR,
>  					SHUTDOWN_MAJOR_MINOR);
>  		} else {
> @@ -225,7 +224,7 @@ static void timesync_onchannelcallback(void *context)
>  				sizeof(struct vmbuspipe_hdr)];
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> time_txf_buf,
> +			vmbus_prep_negotiate_resp(icmsghdrp, time_txf_buf,
>  						UTIL_FW_MAJOR_MINOR,
>  						TIMESYNCH_MAJOR_MINOR);
>  		} else {
> @@ -266,7 +265,7 @@ static void heartbeat_onchannelcallback(void *context)
>  				sizeof(struct vmbuspipe_hdr)];
> 
>  		if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
> -			vmbus_prep_negotiate_resp(icmsghdrp, NULL,
> +			vmbus_prep_negotiate_resp(icmsghdrp,
>  				hbeat_txf_buf, UTIL_FW_MAJOR_MINOR,
>  				HEARTBEAT_MAJOR_MINOR);
>  		} else {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 4994907..084a858 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1502,9 +1502,7 @@ struct hyperv_service_callback {
>  };
> 
>  #define MAX_SRV_VER	0x7ffffff
> -extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *,
> -					struct icmsg_negotiate *, u8 *, int,
> -					int);
> +extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *, u8 *, int, int);
> 
>  int hv_kvp_init(struct hv_util_service *);
>  void hv_kvp_deinit(void);
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_