Linux Virtualization - Aug 2011 - [PATCH 0/8] Staging: hv: vmbus: Driver cleanup

Further cleanup of the vmbus driver:

	1) Cleanup the interrupt handler by inlining some code. 

	2) Ensure message handling is performed on the same CPU that
	   takes the vmbus interrupt. 

	3) Check for events before messages (from the host).

	4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
	   interrupt anyway. 

	5) Some general cleanup.
	  

Regards,

K. Y

Get rid of vmbus_on_isr() by inlining this code in the
real interrupt handler.

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   41 +++++++++++----------------------------
 1 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index b382829..bb25c5b 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -411,53 +411,36 @@ static void vmbus_on_msg_dpc(unsigned long data)
 	}
 }
 
-/*
- * vmbus_on_isr - ISR routine
- */
-static int vmbus_on_isr(void)
+static irqreturn_t vmbus_isr(int irq, void *dev_id)
 {
-	int ret = 0;
 	int cpu = smp_processor_id();
 	void *page_addr;
 	struct hv_message *msg;
 	union hv_synic_event_flags *event;
+	bool handled = false;
 
 	page_addr = hv_context.synic_message_page[cpu];
 	msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
 
 	/* Check if there are actual msgs to be process */
-	if (msg->header.message_type != HVMSG_NONE)
-		ret |= 0x1;
+	if (msg->header.message_type != HVMSG_NONE) {
+		handled = true;
+		tasklet_schedule(&msg_dpc);
+	}
 
 	page_addr = hv_context.synic_event_page[cpu];
 	event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
 
 	/* Since we are a child, we only need to check bit 0 */
-	if (sync_test_and_clear_bit(0, (unsigned long *) &event->flags32[0]))
-		ret |= 0x2;
-
-	return ret;
-}
-
-
-static irqreturn_t vmbus_isr(int irq, void *dev_id)
-{
-	int ret;
-
-	ret = vmbus_on_isr();
-
-	/* Schedules a dpc if necessary */
-	if (ret > 0) {
-		if (test_bit(0, (unsigned long *)&ret))
-			tasklet_schedule(&msg_dpc);
-
-		if (test_bit(1, (unsigned long *)&ret))
-			tasklet_schedule(&event_dpc);
+	if (sync_test_and_clear_bit(0, (unsigned long *) &event->flags32[0])) {
+		handled = true;
+		tasklet_schedule(&event_dpc);
+	}
 
+	if (handled)
 		return IRQ_HANDLED;
-	} else {
+	else
 		return IRQ_NONE;
-	}
 }
 
 /*
-- 
1.7.4.1

The message processing function needs to execute on the same CPU where
the interrupt was taken. tasklets cannot gurantee this; so, invoke the 
function directly.

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index bb25c5b..1d4e878 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -39,7 +39,6 @@
 
 static struct acpi_device  *hv_acpi_dev;
 
-static struct tasklet_struct msg_dpc;
 static struct tasklet_struct event_dpc;
 
 unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
@@ -425,7 +424,7 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 	/* Check if there are actual msgs to be process */
 	if (msg->header.message_type != HVMSG_NONE) {
 		handled = true;
-		tasklet_schedule(&msg_dpc);
+		vmbus_on_msg_dpc(0);
 	}
 
 	page_addr = hv_context.synic_event_page[cpu];
@@ -465,7 +464,6 @@ static int vmbus_bus_init(int irq)
 	}
 
 	/* Initialize the bus context */
-	tasklet_init(&msg_dpc, vmbus_on_msg_dpc, 0);
 	tasklet_init(&event_dpc, vmbus_on_event, 0);
 
 	/* Now, register the bus  with LDM */
-- 
1.7.4.1

Hyper-V requires that events be processed before processing messages. Make the
necessary code adjustments.

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 1d4e878..536466b 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -418,15 +418,6 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 	union hv_synic_event_flags *event;
 	bool handled = false;
 
-	page_addr = hv_context.synic_message_page[cpu];
-	msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
-
-	/* Check if there are actual msgs to be process */
-	if (msg->header.message_type != HVMSG_NONE) {
-		handled = true;
-		vmbus_on_msg_dpc(0);
-	}
-
 	page_addr = hv_context.synic_event_page[cpu];
 	event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
 
@@ -436,6 +427,15 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 		tasklet_schedule(&event_dpc);
 	}
 
+	page_addr = hv_context.synic_message_page[cpu];
+	msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
+
+	/* Check if there are actual msgs to be process */
+	if (msg->header.message_type != HVMSG_NONE) {
+		handled = true;
+		vmbus_on_msg_dpc(0);
+	}
+
 	if (handled)
 		return IRQ_HANDLED;
 	else
-- 
1.7.4.1

The standard interrupt handling in Linux issues an eoi when the
interrupt handling is done. So, setup the vmbus interrupt without
enabling auto eoi.

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/hv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
index 14e6315..3b0f9f4 100644
--- a/drivers/staging/hv/hv.c
+++ b/drivers/staging/hv/hv.c
@@ -370,7 +370,7 @@ void hv_synic_init(void *irqarg)
 	shared_sint.as_uint64 = 0;
 	shared_sint.vector = irq_vector; /* HV_SHARED_SINT_IDT_VECTOR + 0x20; */
 	shared_sint.masked = false;
-	shared_sint.auto_eoi = true;
+	shared_sint.auto_eoi = false;
 
 	wrmsrl(HV_X64_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
 
-- 
1.7.4.1

Fixup some  indentation issues in vmbus_acpi_add().

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 536466b..275c4ef 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -654,9 +654,8 @@ static int vmbus_acpi_add(struct acpi_device *device)
 
 	hv_acpi_dev = device;
 
-	result -	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
-			vmbus_walk_resources, &irq);
+	result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+					vmbus_walk_resources, &irq);
 
 	if (ACPI_FAILURE(result)) {
 		complete(&probe_event);
-- 
1.7.4.1

Get rid of some dated/redundant comments.

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 275c4ef..f20ab9a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -364,9 +364,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
 	kfree(ctx);
 }
 
-/*
- * vmbus_on_msg_dpc - DPC routine to handle messages from the hypervisior
- */
 static void vmbus_on_msg_dpc(unsigned long data)
 {
 	int cpu = smp_processor_id();
@@ -463,15 +460,12 @@ static int vmbus_bus_init(int irq)
 		return ret;
 	}
 
-	/* Initialize the bus context */
 	tasklet_init(&event_dpc, vmbus_on_event, 0);
 
-	/* Now, register the bus  with LDM */
 	ret = bus_register(&hv_bus);
 	if (ret)
 		return ret;
 
-	/* Get the interrupt resource */
 	ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
 			driver_name, hv_acpi_dev);
 
@@ -522,7 +516,6 @@ int vmbus_child_driver_register(struct hv_driver *hv_drv)
 
 	pr_info("child driver registering - name %s\n", drv->name);
 
-	/* The child driver on this vmbus */
 	drv->bus = &hv_bus;
 
 	ret = driver_register(drv);
@@ -563,7 +556,6 @@ struct hv_device *vmbus_child_device_create(uuid_le *type,
 {
 	struct hv_device *child_device_obj;
 
-	/* Allocate the new child device */
 	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
 	if (!child_device_obj) {
 		pr_err("Unable to allocate device object for child device\n");
@@ -589,12 +581,10 @@ int vmbus_child_device_register(struct hv_device
*child_device_obj)
 
 	static atomic_t device_num = ATOMIC_INIT(0);
 
-	/* Set the device name. Otherwise, device_register() will fail. */
 	dev_set_name(&child_device_obj->device, "vmbus_0_%d",
 		     atomic_inc_return(&device_num));
 
-	/* The new device belongs to this bus */
-	child_device_obj->device.bus = &hv_bus; /* device->dev.bus; */
+	child_device_obj->device.bus = &hv_bus;
 	child_device_obj->device.parent = &hv_acpi_dev->dev;
 	child_device_obj->device.release = vmbus_device_release;
 
-- 
1.7.4.1

Fix a bug in error handling in vmbus_bus_init().

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index f20ab9a..0b0bff3 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -463,8 +463,10 @@ static int vmbus_bus_init(int irq)
 	tasklet_init(&event_dpc, vmbus_on_event, 0);
 
 	ret = bus_register(&hv_bus);
-	if (ret)
+	if (ret) {
+		hv_cleanup();
 		return ret;
+	}
 
 	ret = request_irq(irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
 			driver_name, hv_acpi_dev);
@@ -473,6 +475,7 @@ static int vmbus_bus_init(int irq)
 		pr_err("Unable to request IRQ %d\n",
 			   irq);
 
+		hv_cleanup();
 		bus_unregister(&hv_bus);
 
 		return ret;
@@ -487,6 +490,7 @@ static int vmbus_bus_init(int irq)
 	on_each_cpu(hv_synic_init, (void *)&vector, 1);
 	ret = vmbus_connect();
 	if (ret) {
+		hv_cleanup();
 		free_irq(irq, hv_acpi_dev);
 		bus_unregister(&hv_bus);
 		return ret;
-- 
1.7.4.1

Get rid of an unnecessary check in vmbus_connect(). Since vmbus_connect is
only called once, the check being removed is redundant.

Signed-off-by: K. Y. Srinivasan <kys at microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz at microsoft.com>
---
 drivers/staging/hv/connection.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/hv/connection.c b/drivers/staging/hv/connection.c
index 66b7c4b..9e99c04 100644
--- a/drivers/staging/hv/connection.c
+++ b/drivers/staging/hv/connection.c
@@ -50,10 +50,6 @@ int vmbus_connect(void)
 	struct vmbus_channel_initiate_contact *msg;
 	unsigned long flags;
 
-	/* Make sure we are not connecting or connected */
-	if (vmbus_connection.conn_state != DISCONNECTED)
-		return -EISCONN;
-
 	/* Initialize the vmbus connection */
 	vmbus_connection.conn_state = CONNECTING;
 	vmbus_connection.work_queue = create_workqueue("hv_vmbus_con");
-- 
1.7.4.1

On Mon, Aug 15, 2011 at 03:10:10PM -0700, K. Y. Srinivasan
wrote:
> Further cleanup of the vmbus driver:
> 
> 	1) Cleanup the interrupt handler by inlining some code. 
> 
> 	2) Ensure message handling is performed on the same CPU that
> 	   takes the vmbus interrupt. 
> 
> 	3) Check for events before messages (from the host).
> 
> 	4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
> 	   interrupt anyway. 
> 
> 	5) Some general cleanup.
As I didn't apply your other changes, care to respin all of your
outstanding hv patches against the next linux-next release and resend
them?  I tried picking some of the other patches, but they started not
applying easily so it would be easier for everyone to just resend them
after you fix them up.

thanks,

greg k-h