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

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

[Vitaly Kuznetsov]

Changes since v1 (PATCH 1/3):
- Return -EAGAIN instead of open_info->response.open_result.status [Dexuan Cui]
- Avoid 'if (ret != 0)' statement [Dan Carpenter] 

Original description:
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   | 13 ++++++-------
 drivers/hv/vmbus_drv.c | 10 +++++++---
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
1.9.3

[PATCH v2 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. We also need
to avoid returning open_info->response.open_result.status as the return value as
all other errors we return from vmbus_open() are -EXXX and vmbus_open() callers
are not supposed to analyze host error codes.

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

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 54da66d..7a1c2db 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -178,19 +178,18 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 		goto error1;
 	}
 
-
-	if (open_info->response.open_result.status)
-		err = open_info->response.open_result.status;
-
 	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
 	list_del(&open_info->msglistentry);
 	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
 
-	if (err == 0)
-		newchannel->state = CHANNEL_OPENED_STATE;
+	if (open_info->response.open_result.status) {
+		err = -EAGAIN;
+		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 v2 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 v2 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 v2 0/3] Drivers: hv: vmbus: additional fixes for the setup/teardown path

[Dexuan Cui]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, April 22, 2015 0:22
> To: KY Srinivasan
> Cc: Haiyang Zhang; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org; Dexuan Cui; Dan Carpenter
> Subject: [PATCH v2 0/3] Drivers: hv: vmbus: additional fixes for the
> setup/teardown path
> 
> Changes since v1 (PATCH 1/3):
> - Return -EAGAIN instead of open_info->response.open_result.status
> [Dexuan Cui]
> - Avoid 'if (ret != 0)' statement [Dan Carpenter]
> 
> Original description:
> 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   | 13 ++++++-------
>  drivers/hv/vmbus_drv.c | 10 +++++++---
>  2 files changed, 13 insertions(+), 10 deletions(-)

The patchset seems good to me.

Reviewed-by: Dexuan Cui <decui@microsoft.com>

Thanks,
-- Dexuan

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Wednesday, April 22, 2015 0:22
>> To: KY Srinivasan
>> Cc: Haiyang Zhang; devel@linuxdriverproject.org; linux-
>> kernel@vger.kernel.org; Dexuan Cui; Dan Carpenter
>> Subject: [PATCH v2 0/3] Drivers: hv: vmbus: additional fixes for the
>> setup/teardown path
>> 
>> Changes since v1 (PATCH 1/3):
>> - Return -EAGAIN instead of open_info->response.open_result.status
>> [Dexuan Cui]
>> - Avoid 'if (ret != 0)' statement [Dan Carpenter]
>> 
>> Original description:
>> 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   | 13 ++++++-------
>>  drivers/hv/vmbus_drv.c | 10 +++++++---
>>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> The patchset seems good to me.
>
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
>

Thanks!

-- 
  Vitaly

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, April 21, 2015 9:22 AM
> To: KY Srinivasan
> Cc: Haiyang Zhang; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org; Dexuan Cui; Dan Carpenter
> Subject: [PATCH v2 3/3] Drivers: hv: vmbus: setup irq after synic is initialized
> 
> 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.

Why? Until we tell the hypervisor to route the vmbus interrupts on the specified vector,
no vmbus interrupts will be delivered and this is done in the function hv_synic_init().
So, the only requirement here is that all of the irq plumbing and allocations be done 
before we enable the synthetic interrupt controller and register the vector with the
hypervisor. What am I missing.

Regards,

K. Y
> 
> 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 v2 3/3] Drivers: hv: vmbus: setup irq after synic is initialized

[Vitaly Kuznetsov]

KY Srinivasan <kys@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Tuesday, April 21, 2015 9:22 AM
>> To: KY Srinivasan
>> Cc: Haiyang Zhang; devel@linuxdriverproject.org; linux-
>> kernel@vger.kernel.org; Dexuan Cui; Dan Carpenter
>> Subject: [PATCH v2 3/3] Drivers: hv: vmbus: setup irq after synic is initialized
>> 
>> 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.
>
> Why? Until we tell the hypervisor to route the vmbus interrupts on the specified vector,
> no vmbus interrupts will be delivered and this is done in the function hv_synic_init().
> So, the only requirement here is that all of the irq plumbing and allocations be done 
> before we enable the synthetic interrupt controller and register the vector with the
> hypervisor. What am I missing.

I should have made it clearer that I'm not fixing a bug here, it just
looks logical to me to put synic pages allocation (hv_synic_alloc())
before we register vmbus_isr() handler which uses these pages. You're
absolutely right and this handler won't be executed with unallocated
pages now, this patch was rather future-proof.


>
> Regards,
>
> K. Y
>> 
>> 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

-- 
  Vitaly