[PATCH 0/3] Drivers: hv: vmbus: additional fixes for the setup/teardown path

[PATCH 0/3] Drivers: hv: vmbus: additional fixes for the setup/teardown path

[Vitaly Kuznetsov]

K. Y.,

here are 3 additional patches for your "[PATCH 0/5] Drivers: hv: vmbus: Cleanup
the vmbus unload path" series (with the fix I suggested). I tested the whole
set on Gen2 Win2012R2 guest, load/unload path seems being stable. Can you
please take a look?

Thanks,

Vitaly Kuznetsov (3):
  Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths
  Drivers: hv: vmbus: kill tasklets on module unload
  Drivers: hv: vmbus: setup irq after synic is initialized

 drivers/hv/channel.c   |  7 ++++---
 drivers/hv/vmbus_drv.c | 10 +++++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

-- 
1.9.3

[PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths

[Vitaly Kuznetsov]

In case there was an error reported in the response to the CHANNELMSG_OPENCHANNEL
call we need to do the cleanup as a vmbus_open() user won't be doing it after
receiving an error. The cleanup should be done on all failure paths.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 54da66d..836386f 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -186,11 +186,12 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 	list_del(&open_info->msglistentry);
 	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
 
-	if (err == 0)
-		newchannel->state = CHANNEL_OPENED_STATE;
+	if (err != 0)
+		goto error_gpadl;
 
+	newchannel->state = CHANNEL_OPENED_STATE;
 	kfree(open_info);
-	return err;
+	return 0;
 
 error1:
 	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
-- 
1.9.3

[PATCH 2/3] Drivers: hv: vmbus: kill tasklets on module unload

[Vitaly Kuznetsov]

Explicitly kill tasklets we create on module unload.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 2b56260..cf20400 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1108,6 +1108,7 @@ static void __exit vmbus_exit(void)
 	hv_synic_clockevents_cleanup();
 	vmbus_disconnect();
 	hv_remove_vmbus_irq();
+	tasklet_kill(&msg_dpc);
 	vmbus_free_channels();
 	if (ms_hyperv.features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
 		atomic_notifier_chain_unregister(&panic_notifier_list,
@@ -1115,8 +1116,10 @@ static void __exit vmbus_exit(void)
 	}
 	bus_unregister(&hv_bus);
 	hv_cleanup();
-	for_each_online_cpu(cpu)
+	for_each_online_cpu(cpu) {
+		tasklet_kill(hv_context.event_dpc[cpu]);
 		smp_call_function_single(cpu, hv_synic_cleanup, NULL, 1);
+	}
 	acpi_bus_unregister_driver(&vmbus_acpi_driver);
 	hv_cpu_hotplug_quirk(false);
 }
-- 
1.9.3

[PATCH 3/3] Drivers: hv: vmbus: setup irq after synic is initialized

[Vitaly Kuznetsov]

vmbus_isr() uses synic pages which are being setup in hv_synic_alloc(), we
need to register this irq handler only after we allocate all the required
pages.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index cf20400..e922804 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -822,11 +822,12 @@ static int vmbus_bus_init(int irq)
 	if (ret)
 		goto err_cleanup;
 
-	hv_setup_vmbus_irq(vmbus_isr);
-
 	ret = hv_synic_alloc();
 	if (ret)
 		goto err_alloc;
+
+	hv_setup_vmbus_irq(vmbus_isr);
+
 	/*
 	 * Initialize the per-cpu interrupt state and
 	 * connect to the host.
-- 
1.9.3

RE: [PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths

[Dexuan Cui]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, April 21, 2015 16:18
> To: KY Srinivasan
> Cc: Haiyang Zhang; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org; Dexuan Cui
> Subject: [PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open()
> failure paths
> 
> In case there was an error reported in the response to the
> CHANNELMSG_OPENCHANNEL
> call we need to do the cleanup as a vmbus_open() user won't be doing it
> after
> receiving an error. The cleanup should be done on all failure paths.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/channel.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 54da66d..836386f 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -186,11 +186,12 @@ int vmbus_open(struct vmbus_channel
> *newchannel, u32 send_ringbuffer_size,
>  	list_del(&open_info->msglistentry);
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock,
> flags);
> 
> -	if (err == 0)
> -		newchannel->state = CHANNEL_OPENED_STATE;
> +	if (err != 0)
> +		goto error_gpadl;
> 
> +	newchannel->state = CHANNEL_OPENED_STATE;
>  	kfree(open_info);
> -	return err;
> +	return 0;
> 
>  error1:
>  	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
> --

Thanks for the patch, Vitaly!

The patch looks good to me except for 1 small issue:
I suppose open_info->response.open_result.status is different from
Linux-style -EXXX error codes.

Maybe here we can return -EAGAIN if err != 0 ?

-- Dexuan

Re: [PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths

[Dan Carpenter]

On Tue, Apr 21, 2015 at 10:17:52AM +0200, Vitaly Kuznetsov wrote:
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 54da66d..836386f 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -186,11 +186,12 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
>  	list_del(&open_info->msglistentry);
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>  
> -	if (err == 0)
> -		newchannel->state = CHANNEL_OPENED_STATE;
> +	if (err != 0)

Doesn't the double negative not make it slightly more confusing?

regards,
dan carpenter

Re: [PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Tuesday, April 21, 2015 16:18
>> To: KY Srinivasan
>> Cc: Haiyang Zhang; devel@linuxdriverproject.org; linux-
>> kernel@vger.kernel.org; Dexuan Cui
>> Subject: [PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open()
>> failure paths
>> 
>> In case there was an error reported in the response to the
>> CHANNELMSG_OPENCHANNEL
>> call we need to do the cleanup as a vmbus_open() user won't be doing it
>> after
>> receiving an error. The cleanup should be done on all failure paths.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/hv/channel.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> index 54da66d..836386f 100644
>> --- a/drivers/hv/channel.c
>> +++ b/drivers/hv/channel.c
>> @@ -186,11 +186,12 @@ int vmbus_open(struct vmbus_channel
>> *newchannel, u32 send_ringbuffer_size,
>>  	list_del(&open_info->msglistentry);
>>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock,
>> flags);
>> 
>> -	if (err == 0)
>> -		newchannel->state = CHANNEL_OPENED_STATE;
>> +	if (err != 0)
>> +		goto error_gpadl;
>> 
>> +	newchannel->state = CHANNEL_OPENED_STATE;
>>  	kfree(open_info);
>> -	return err;
>> +	return 0;
>> 
>>  error1:
>>  	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
>> --
>
> Thanks for the patch, Vitaly!
>
> The patch looks good to me except for 1 small issue:
> I suppose open_info->response.open_result.status is different from
> Linux-style -EXXX error codes.
>
> Maybe here we can return -EAGAIN if err != 0 ?

I think I inspected all vmbus_open() callers and they all just check ret
!= 0 but I agree we'd better return some -EXXXX code here. I'm not
exactly sure if open_result.status != 0 usually means a permanent or a
temporary failure but if it's temporary -EAGAIN here seems reasonable.

I'll send v2, thanks for the review!

>
> -- Dexuan

-- 
  Vitaly

Re: [PATCH 1/3] Drivers: hv: vmbus: do cleanup on all vmbus_open() failure paths

[Vitaly Kuznetsov]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Tue, Apr 21, 2015 at 10:17:52AM +0200, Vitaly Kuznetsov wrote:
>> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> index 54da66d..836386f 100644
>> --- a/drivers/hv/channel.c
>> +++ b/drivers/hv/channel.c
>> @@ -186,11 +186,12 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
>>  	list_del(&open_info->msglistentry);
>>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>>  
>> -	if (err == 0)
>> -		newchannel->state = CHANNEL_OPENED_STATE;
>> +	if (err != 0)
>
> Doesn't the double negative not make it slightly more confusing?
>

Point taken, will fix in v2 :-)

> regards,
> dan carpenter

-- 
  Vitaly