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
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 <[email protected]>
---
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
Explicitly kill tasklets we create on module unload.
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
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
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 <[email protected]>
---
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
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:[email protected]]
> Sent: Wednesday, April 22, 2015 0:22
> To: KY Srinivasan
> Cc: Haiyang Zhang; [email protected]; linux-
> [email protected]; 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 <[email protected]>
Thanks,
-- Dexuan
Dexuan Cui <[email protected]> writes:
>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:[email protected]]
>> Sent: Wednesday, April 22, 2015 0:22
>> To: KY Srinivasan
>> Cc: Haiyang Zhang; [email protected]; linux-
>> [email protected]; 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 <[email protected]>
>
Thanks!
--
Vitaly
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:[email protected]]
> Sent: Tuesday, April 21, 2015 9:22 AM
> To: KY Srinivasan
> Cc: Haiyang Zhang; [email protected]; linux-
> [email protected]; 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 <[email protected]>
> ---
> 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
KY Srinivasan <[email protected]> writes:
>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:[email protected]]
>> Sent: Tuesday, April 21, 2015 9:22 AM
>> To: KY Srinivasan
>> Cc: Haiyang Zhang; [email protected]; linux-
>> [email protected]; 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 <[email protected]>
>> ---
>> 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