RE: [PATCH 2/2] hv_balloon: Reorganize the probe function

[Michael Kelley <mikelley@microsoft.com>]

From: Dexuan Cui <decui@microsoft.com>  Sent: Friday, June 14, 2019 11:43 AM
> 
> Move the code that negotiates with the host to a new function
> balloon_connect_vsp() and improve the error handling.
> 
> This makes the code more readable and paves the way for the
> support of hibernation in future.
> 
> Makes no real logic change here.
> 
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/hv_balloon.c | 124 +++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 13381ea3e3e7..111ea3599659 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1574,50 +1574,18 @@ static void balloon_onchannelcallback(void *context)
> 
>  }
> 
> -static int balloon_probe(struct hv_device *dev,
> -			const struct hv_vmbus_device_id *dev_id)
> +static int balloon_connect_vsp(struct hv_device *dev)
>  {
> -	int ret;
> -	unsigned long t;
>  	struct dm_version_request version_req;
>  	struct dm_capabilities cap_msg;
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -	do_hot_add = hot_add;
> -#else
> -	do_hot_add = false;
> -#endif
> +	unsigned long t;
> +	int ret;
> 
>  	ret = vmbus_open(dev->channel, dm_ring_size, dm_ring_size, NULL, 0,
> -			balloon_onchannelcallback, dev);
> -
> +			 balloon_onchannelcallback, dev);
>  	if (ret)
>  		return ret;
> 
> -	dm_device.dev = dev;
> -	dm_device.state = DM_INITIALIZING;
> -	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> -	init_completion(&dm_device.host_event);
> -	init_completion(&dm_device.config_event);
> -	INIT_LIST_HEAD(&dm_device.ha_region_list);
> -	spin_lock_init(&dm_device.ha_lock);
> -	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> -	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> -	dm_device.host_specified_ha_region = false;
> -
> -	dm_device.thread =
> -		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> -	if (IS_ERR(dm_device.thread)) {
> -		ret = PTR_ERR(dm_device.thread);
> -		goto probe_error1;
> -	}
> -
> -#ifdef CONFIG_MEMORY_HOTPLUG
> -	set_online_page_callback(&hv_online_page);
> -	register_memory_notifier(&hv_memory_nb);
> -#endif
> -
> -	hv_set_drvdata(dev, &dm_device);
>  	/*
>  	 * Initiate the hand shake with the host and negotiate
>  	 * a version that the host can support. We start with the
> @@ -1633,16 +1601,15 @@ static int balloon_probe(struct hv_device *dev,
>  	dm_device.version = version_req.version.version;
> 
>  	ret = vmbus_sendpacket(dev->channel, &version_req,
> -				sizeof(struct dm_version_request),
> -				(unsigned long)NULL,
> -				VM_PKT_DATA_INBAND, 0);
> +			       sizeof(struct dm_version_request),
> +			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
>  	if (ret)
> -		goto probe_error2;
> +		goto out;
> 
>  	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		goto out;
>  	}
> 
>  	/*
> @@ -1650,8 +1617,8 @@ static int balloon_probe(struct hv_device *dev,
>  	 * fail the probe function.
>  	 */
>  	if (dm_device.state == DM_INIT_ERROR) {
> -		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		ret = -EPROTO;
> +		goto out;
>  	}
> 
>  	pr_info("Using Dynamic Memory protocol version %u.%u\n",
> @@ -1684,16 +1651,15 @@ static int balloon_probe(struct hv_device *dev,
>  	cap_msg.max_page_number = -1;
> 
>  	ret = vmbus_sendpacket(dev->channel, &cap_msg,
> -				sizeof(struct dm_capabilities),
> -				(unsigned long)NULL,
> -				VM_PKT_DATA_INBAND, 0);
> +			       sizeof(struct dm_capabilities),
> +			       (unsigned long)NULL, VM_PKT_DATA_INBAND, 0);
>  	if (ret)
> -		goto probe_error2;
> +		goto out;
> 
>  	t = wait_for_completion_timeout(&dm_device.host_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		goto out;
>  	}
> 
>  	/*
> @@ -1701,23 +1667,65 @@ static int balloon_probe(struct hv_device *dev,
>  	 * fail the probe function.
>  	 */
>  	if (dm_device.state == DM_INIT_ERROR) {
> -		ret = -ETIMEDOUT;
> -		goto probe_error2;
> +		ret = -EPROTO;
> +		goto out;
>  	}
> 
> +	return 0;
> +out:
> +	vmbus_close(dev->channel);
> +	return ret;
> +}
> +
> +static int balloon_probe(struct hv_device *dev,
> +			 const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	do_hot_add = hot_add;
> +#else
> +	do_hot_add = false;
> +#endif
> +	dm_device.dev = dev;
> +	dm_device.state = DM_INITIALIZING;
> +	dm_device.next_version = DYNMEM_PROTOCOL_VERSION_WIN8;
> +	init_completion(&dm_device.host_event);
> +	init_completion(&dm_device.config_event);
> +	INIT_LIST_HEAD(&dm_device.ha_region_list);
> +	spin_lock_init(&dm_device.ha_lock);
> +	INIT_WORK(&dm_device.balloon_wrk.wrk, balloon_up);
> +	INIT_WORK(&dm_device.ha_wrk.wrk, hot_add_req);
> +	dm_device.host_specified_ha_region = false;
> +
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	set_online_page_callback(&hv_online_page);
> +	register_memory_notifier(&hv_memory_nb);
> +#endif
> +
> +	hv_set_drvdata(dev, &dm_device);
> +
> +	ret = balloon_connect_vsp(dev);
> +	if (ret != 0)
> +		return ret;
> +
>  	dm_device.state = DM_INITIALIZED;
> -	last_post_time = jiffies;

I was curious about the above deletion.  But I guess the line
is not needed as the time_after() check in post_status() should
handle an initial value of 0 for last_post_time just fine.

> +
> +	dm_device.thread =
> +		 kthread_run(dm_thread_func, &dm_device, "hv_balloon");
> +	if (IS_ERR(dm_device.thread)) {
> +		ret = PTR_ERR(dm_device.thread);
> +		goto probe_error;
> +	}

Just an observation:  this thread creation now happens at the end of the
probing process.  But that's good, because in the old code, the thread
was started and could run before the protocol version had been
negotiated.  So I'll assume your change here is intentional.

> 
>  	return 0;
> 
> -probe_error2:
> +probe_error:
> +	vmbus_close(dev->channel);
>  #ifdef CONFIG_MEMORY_HOTPLUG
> +	unregister_memory_notifier(&hv_memory_nb);

Hmmm. Evidently the above cleanup was missing in the
old code.

>  	restore_online_page_callback(&hv_online_page);
>  #endif
> -	kthread_stop(dm_device.thread);
> -
> -probe_error1:
> -	vmbus_close(dev->channel);
>  	return ret;
>  }
> 
> @@ -1734,11 +1742,11 @@ static int balloon_remove(struct hv_device *dev)
>  	cancel_work_sync(&dm->balloon_wrk.wrk);
>  	cancel_work_sync(&dm->ha_wrk.wrk);
> 
> -	vmbus_close(dev->channel);
>  	kthread_stop(dm->thread);
> +	vmbus_close(dev->channel);

Presumably this is an intentional ordering change as well.
The kthread should be stopped before closing the channel.

>  #ifdef CONFIG_MEMORY_HOTPLUG
> -	restore_online_page_callback(&hv_online_page);
>  	unregister_memory_notifier(&hv_memory_nb);
> +	restore_online_page_callback(&hv_online_page);

And you've changed the ordering of these steps so they are
the inverse of when they are set up.  Also a good cleanup ....

>  #endif
>  	spin_lock_irqsave(&dm_device.ha_lock, flags);
>  	list_for_each_entry_safe(has, tmp, &dm->ha_region_list, list) {
> --
> 2.19.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>