Re: [v2 PATCH for-4.4 07/16] Drivers: hv: get rid of redundant messagecount in create_gpadl_header()

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Apr 12, 2017 at 11:13:41PM +0530, Sumit Semwal wrote:
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> [ Upstream commit 4d63763296ab7865a98bc29cc7d77145815ef89f ]
> 
> We use messagecount only once in vmbus_establish_gpadl() to check if
> it is safe to iterate through the submsglist. We can just initialize
> the list header in all cases in create_gpadl_header() instead.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> ---
>  drivers/hv/channel.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 1ef37c7..fb1e3df 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -223,8 +223,7 @@ EXPORT_SYMBOL_GPL(vmbus_open);
>   * create_gpadl_header - Creates a gpadl for the specified buffer
>   */
>  static int create_gpadl_header(void *kbuffer, u32 size,
> -					 struct vmbus_channel_msginfo **msginfo,
> -					 u32 *messagecount)
> +			       struct vmbus_channel_msginfo **msginfo)
>  {
>  	int i;
>  	int pagecount;
> @@ -268,7 +267,6 @@ static int create_gpadl_header(void *kbuffer, u32 size,
>  			gpadl_header->range[0].pfn_array[i] = slow_virt_to_phys(
>  				kbuffer + PAGE_SIZE * i) >> PAGE_SHIFT;
>  		*msginfo = msgheader;
> -		*messagecount = 1;
>  
>  		pfnsum = pfncount;
>  		pfnleft = pagecount - pfncount;
> @@ -308,7 +306,6 @@ static int create_gpadl_header(void *kbuffer, u32 size,
>  			}
>  
>  			msgbody->msgsize = msgsize;
> -			(*messagecount)++;
>  			gpadl_body =
>  				(struct vmbus_channel_gpadl_body *)msgbody->msg;
>  
> @@ -337,6 +334,8 @@ static int create_gpadl_header(void *kbuffer, u32 size,
>  		msgheader = kzalloc(msgsize, GFP_KERNEL);
>  		if (msgheader == NULL)
>  			goto nomem;
> +
> +		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
>  
>  		gpadl_header = (struct vmbus_channel_gpadl_header *)
> @@ -351,7 +350,6 @@ static int create_gpadl_header(void *kbuffer, u32 size,
>  				kbuffer + PAGE_SIZE * i) >> PAGE_SHIFT;
>  
>  		*msginfo = msgheader;
> -		*messagecount = 1;
>  	}
>  
>  	return 0;
> @@ -376,7 +374,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
>  	struct vmbus_channel_gpadl_body *gpadl_body;
>  	struct vmbus_channel_msginfo *msginfo = NULL;
>  	struct vmbus_channel_msginfo *submsginfo;
> -	u32 msgcount;
>  	struct list_head *curr;
>  	u32 next_gpadl_handle;
>  	unsigned long flags;
> @@ -385,7 +382,7 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
>  	next_gpadl_handle =
>  		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
>  
> -	ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount);
> +	ret = create_gpadl_header(kbuffer, size, &msginfo);
>  	if (ret)
>  		return ret;
>  
> @@ -408,24 +405,21 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
>  	if (ret != 0)
>  		goto cleanup;
>  
> -	if (msgcount > 1) {
> -		list_for_each(curr, &msginfo->submsglist) {
> +	list_for_each(curr, &msginfo->submsglist) {
> +		submsginfo = (struct vmbus_channel_msginfo *)curr;
> +		gpadl_body =
> +			(struct vmbus_channel_gpadl_body *)submsginfo->msg;
>  
> -			submsginfo = (struct vmbus_channel_msginfo *)curr;
> -			gpadl_body =
> -			     (struct vmbus_channel_gpadl_body *)submsginfo->msg;
> +		gpadl_body->header.msgtype =
> +			CHANNELMSG_GPADL_BODY;
> +		gpadl_body->gpadl = next_gpadl_handle;
>  
> -			gpadl_body->header.msgtype =
> -				CHANNELMSG_GPADL_BODY;
> -			gpadl_body->gpadl = next_gpadl_handle;
> +		ret = vmbus_post_msg(gpadl_body,
> +				     submsginfo->msgsize -
> +				     sizeof(*submsginfo));
> +		if (ret != 0)
> +			goto cleanup;
>  
> -			ret = vmbus_post_msg(gpadl_body,
> -					       submsginfo->msgsize -
> -					       sizeof(*submsginfo));
> -			if (ret != 0)
> -				goto cleanup;
> -
> -		}
>  	}
>  	wait_for_completion(&msginfo->waitevent);
>  

Why should this go into a stable kernel? What does it fix?

thanks,

greg k-h