[PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[K. Y. Srinivasan]

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/vmbus_drv.c |   49 +++++++++++++++++++++++++++------------
 1 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..f279e6a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -36,9 +36,7 @@
 #include "vmbus_private.h"
 
 
-/* FIXME! We need to do this dynamically for PIC and APIC system */
-#define VMBUS_IRQ		0x5
-#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
+static int vmbus_irq;
 
 /* Main vmbus driver data structure */
 struct vmbus_driver_context {
@@ -57,6 +55,25 @@ struct vmbus_driver_context {
 	struct vm_device device_ctx;
 };
 
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available; return -1.
+ */
+
+static int vmbus_get_irq(void)
+{
+	unsigned int avail_irq_mask;
+	int irq = -1;
+	/*
+	 * Pick the first unused interrupt. HyperV can
+	 * interrupt us on any interrupt line we specify.
+	 */
+	avail_irq_mask = probe_irq_on();
+	if (avail_irq_mask != 0)
+		irq = ffs(avail_irq_mask);
+	probe_irq_off(avail_irq_mask);
+	return irq;
+}
+
 static int vmbus_match(struct device *device, struct device_driver *driver);
 static int vmbus_probe(struct device *device);
 static int vmbus_remove(struct device *device);
@@ -80,7 +97,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
 	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
 	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
 
-static int vmbus_irq = VMBUS_IRQ;
 
 /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
 static struct device_attribute vmbus_device_attrs[] = {
@@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
 	struct hv_driver *driver = &vmbus_drv.drv_obj;
 	struct vm_device *dev_ctx = &vmbus_drv.device_ctx;
 	int ret;
-	unsigned int vector;
+	unsigned int vector, prev_irq = ~0;
 
 	DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
 		    HV_DRV_VERSION);
@@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
 	}
 
 	/* Get the interrupt resource */
-	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
-			  driver->name, NULL);
-
-	if (ret != 0) {
-		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
-			   vmbus_irq);
-
+get_irq_again:
+	vmbus_irq = vmbus_get_irq();
+	if ((vmbus_irq < 0) || (prev_irq == vmbus_irq)) {
+		printk(KERN_WARNING "VMBUS_DRV: Failed to allocate IRQ\n");
 		bus_unregister(&vmbus_drv_ctx->bus);
-
 		ret = -1;
 		goto cleanup;
 	}
-	vector = VMBUS_IRQ_VECTOR;
+	prev_irq = vmbus_irq;
+
+	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+			  driver->name, NULL);
+
+	if (ret != 0)
+		goto get_irq_again;
+
+	vector = IRQ0_VECTOR + vmbus_irq;
 
 	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
 
@@ -1117,7 +1137,6 @@ MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
 
 MODULE_LICENSE("GPL");
 MODULE_VERSION(HV_DRV_VERSION);
-module_param(vmbus_irq, int, S_IRUGO);
 module_param(vmbus_loglevel, int, S_IRUGO);
 
 module_init(vmbus_init);
-- 
1.5.5.6

Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Tue, Feb 15, 2011 at 07:15:37AM -0800, K. Y. Srinivasan wrote:
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> ---
>  drivers/staging/hv/vmbus_drv.c |   49 +++++++++++++++++++++++++++------------
>  1 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index 459c707..f279e6a 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -36,9 +36,7 @@
>  #include "vmbus_private.h"
>  
>  
> -/* FIXME! We need to do this dynamically for PIC and APIC system */
> -#define VMBUS_IRQ		0x5
> -#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
> +static int vmbus_irq;
>  
>  /* Main vmbus driver data structure */
>  struct vmbus_driver_context {
> @@ -57,6 +55,25 @@ struct vmbus_driver_context {
>  	struct vm_device device_ctx;
>  };
>  
> +/*
> + * Find an un-used IRQ that the VMBUS can use. If none is available; return -1.
> + */
> +
> +static int vmbus_get_irq(void)

No extra blank line between function comment and function.

> +{
> +	unsigned int avail_irq_mask;
> +	int irq = -1;
> +	/*

Put an extra blank line after your variables please.

> +	 * Pick the first unused interrupt. HyperV can
> +	 * interrupt us on any interrupt line we specify.
> +	 */
> +	avail_irq_mask = probe_irq_on();
> +	if (avail_irq_mask != 0)
> +		irq = ffs(avail_irq_mask);
> +	probe_irq_off(avail_irq_mask);
> +	return irq;
> +}
> +
>  static int vmbus_match(struct device *device, struct device_driver *driver);
>  static int vmbus_probe(struct device *device);
>  static int vmbus_remove(struct device *device);
> @@ -80,7 +97,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
>  	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
>  	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
>  
> -static int vmbus_irq = VMBUS_IRQ;
>  
>  /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
>  static struct device_attribute vmbus_device_attrs[] = {
> @@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
>  	struct hv_driver *driver = &vmbus_drv.drv_obj;
>  	struct vm_device *dev_ctx = &vmbus_drv.device_ctx;
>  	int ret;
> -	unsigned int vector;
> +	unsigned int vector, prev_irq = ~0;
>  
>  	DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
>  		    HV_DRV_VERSION);
> @@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
>  	}
>  
>  	/* Get the interrupt resource */
> -	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> -			  driver->name, NULL);
> -
> -	if (ret != 0) {
> -		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
> -			   vmbus_irq);
> -
> +get_irq_again:
> +	vmbus_irq = vmbus_get_irq();
> +	if ((vmbus_irq < 0) || (prev_irq == vmbus_irq)) {
> +		printk(KERN_WARNING "VMBUS_DRV: Failed to allocate IRQ\n");
>  		bus_unregister(&vmbus_drv_ctx->bus);
> -
>  		ret = -1;

Please provide a "real" error code.

>  		goto cleanup;
>  	}
> -	vector = VMBUS_IRQ_VECTOR;
> +	prev_irq = vmbus_irq;
> +
> +	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> +			  driver->name, NULL);
> +
> +	if (ret != 0)
> +		goto get_irq_again;

You just set up the potential for an endless loop.  You almost _never_
want to goto backwards in a function.  Please don't do this.

> +
> +	vector = IRQ0_VECTOR + vmbus_irq;

What's this for?

>  	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);

And why are you still printing this out for the whole world to see?

thanks,

greg k-h

RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Tuesday, February 15, 2011 11:00 AM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; Haiyang Zhang; Hank Janssen
> Subject: Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
> 
> On Tue, Feb 15, 2011 at 07:15:37AM -0800, K. Y. Srinivasan wrote:
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> >
> > ---
> >  drivers/staging/hv/vmbus_drv.c |   49 +++++++++++++++++++++++++++------
> ------
> >  1 files changed, 34 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > index 459c707..f279e6a 100644
> > --- a/drivers/staging/hv/vmbus_drv.c
> > +++ b/drivers/staging/hv/vmbus_drv.c
> > @@ -36,9 +36,7 @@
> >  #include "vmbus_private.h"
> >
> >
> > -/* FIXME! We need to do this dynamically for PIC and APIC system */
> > -#define VMBUS_IRQ		0x5
> > -#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
> > +static int vmbus_irq;
> >
> >  /* Main vmbus driver data structure */
> >  struct vmbus_driver_context {
> > @@ -57,6 +55,25 @@ struct vmbus_driver_context {
> >  	struct vm_device device_ctx;
> >  };
> >
> > +/*
> > + * Find an un-used IRQ that the VMBUS can use. If none is available; return -1.
> > + */
> > +
> > +static int vmbus_get_irq(void)
> 
> No extra blank line between function comment and function.

I will take care of this.
> 
> > +{
> > +	unsigned int avail_irq_mask;
> > +	int irq = -1;
> > +	/*
> 
> Put an extra blank line after your variables please.

Will do.
> 
> > +	 * Pick the first unused interrupt. HyperV can
> > +	 * interrupt us on any interrupt line we specify.
> > +	 */
> > +	avail_irq_mask = probe_irq_on();
> > +	if (avail_irq_mask != 0)
> > +		irq = ffs(avail_irq_mask);
> > +	probe_irq_off(avail_irq_mask);
> > +	return irq;
> > +}
> > +
> >  static int vmbus_match(struct device *device, struct device_driver *driver);
> >  static int vmbus_probe(struct device *device);
> >  static int vmbus_remove(struct device *device);
> > @@ -80,7 +97,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
> >  	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
> >  	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
> >
> > -static int vmbus_irq = VMBUS_IRQ;
> >
> >  /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
> >  static struct device_attribute vmbus_device_attrs[] = {
> > @@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
> >  	struct hv_driver *driver = &vmbus_drv.drv_obj;
> >  	struct vm_device *dev_ctx = &vmbus_drv.device_ctx;
> >  	int ret;
> > -	unsigned int vector;
> > +	unsigned int vector, prev_irq = ~0;
> >
> >  	DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
> >  		    HV_DRV_VERSION);
> > @@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
> >  	}
> >
> >  	/* Get the interrupt resource */
> > -	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > -			  driver->name, NULL);
> > -
> > -	if (ret != 0) {
> > -		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
> > -			   vmbus_irq);
> > -
> > +get_irq_again:
> > +	vmbus_irq = vmbus_get_irq();
> > +	if ((vmbus_irq < 0) || (prev_irq == vmbus_irq)) {
> > +		printk(KERN_WARNING "VMBUS_DRV: Failed to allocate IRQ\n");
> >  		bus_unregister(&vmbus_drv_ctx->bus);
> > -
> >  		ret = -1;
> 
> Please provide a "real" error code.

Will do.
> 
> >  		goto cleanup;
> >  	}
> > -	vector = VMBUS_IRQ_VECTOR;
> > +	prev_irq = vmbus_irq;
> > +
> > +	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > +			  driver->name, NULL);
> > +
> > +	if (ret != 0)
> > +		goto get_irq_again;
> 
> You just set up the potential for an endless loop.  You almost _never_
> want to goto backwards in a function.  Please don't do this.

The loop is not endless. We need to take care of two conditions here:
1) From the point we selected an irq line to the point where we call
request_irq(), it is possible that someone else could have requested
the same line and so we need to close this window.
2) request_irq() may fail for reasons other than the fact that we
may have lost the line that we thought we got.

So, while we loopback, we check to see if the irq line we got is the same 
that we got before (refer to the check soon after the call
to vmbus_get_irq()). This check would ensure that we would not loop
endlessly.

> 
> > +
> > +	vector = IRQ0_VECTOR + vmbus_irq;
> 
> What's this for?
This is what gets passed to the hypervisor - refer to vmbus_dev_add(). 
This is existing code.
> 
> >  	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
> 
> And why are you still printing this out for the whole world to see?
Greg, this patch addressed a specific comment with regards dynamically allocate the irq. 
Hank is working on a patch to address the various DPRINTS in the code.

Regards,

K. Y
> 
> thanks,
> 
> greg k-h

RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Hank Janssen]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Tuesday, February 15, 2011 8:54 AM
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Tuesday, February 15, 2011 11:00 AM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org; Haiyang Zhang; Hank Janssen
> > Subject: Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
> >

> > And why are you still printing this out for the whole world to see?
> Greg, this patch addressed a specific comment with regards dynamically
> allocate the irq.
> Hank is working on a patch to address the various DPRINTS in the code.

Before the end of the week I will submit two patches for this;

	Remove DPRINT and change it to printk
	Remove excessive printouts on startup of vmbus

Hank.

Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Tue, Feb 15, 2011 at 04:59:28PM +0000, Hank Janssen wrote:
> 
> 
> > -----Original Message-----
> > From: KY Srinivasan
> > Sent: Tuesday, February 15, 2011 8:54 AM
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@suse.de]
> > > Sent: Tuesday, February 15, 2011 11:00 AM
> > > To: KY Srinivasan
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org; Haiyang Zhang; Hank Janssen
> > > Subject: Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
> > >
> 
> > > And why are you still printing this out for the whole world to see?
> > Greg, this patch addressed a specific comment with regards dynamically
> > allocate the irq.
> > Hank is working on a patch to address the various DPRINTS in the code.
> 
> Before the end of the week I will submit two patches for this;
> 
> 	Remove DPRINT and change it to printk

No, use dev_dbg() and friends instead of "raw" printk() calls.

> 	Remove excessive printouts on startup of vmbus

That would be nice to see.  There should be almost nothing outputed by
these drivers on "normal" operation.

thanks,

greg k-h

RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Hank Janssen]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Tuesday, February 15, 2011 9:23 AM
> > Before the end of the week I will submit two patches for this;
> >
> > 	Remove DPRINT and change it to printk
> 
> No, use dev_dbg() and friends instead of "raw" printk() calls.
> 

Will do, you caught me just as I was starting the conversion :)

> > 	Remove excessive printouts on startup of vmbus
> 
> That would be nice to see.  There should be almost nothing outputed by
> these drivers on "normal" operation.

Agreed.

Hank.

RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Hank Janssen]

> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Tuesday, February 15, 2011 9:23 AM
> > > Before the end of the week I will submit two patches for this;
> > >
> > > 	Remove DPRINT and change it to printk
> >
> > No, use dev_dbg() and friends instead of "raw" printk() calls.
> >
> 
> Will do, you caught me just as I was starting the conversion :)

While cleaning this up there are a few places in vmbus and channel behavior
where it is not in a device context. Are printk's okay in that context?

The three drivers network/SCSI and Block of course will use dev_XX family.

Hank.

Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Tue, Feb 15, 2011 at 07:09:34PM +0000, Hank Janssen wrote:
> 
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@suse.de]
> > > Sent: Tuesday, February 15, 2011 9:23 AM
> > > > Before the end of the week I will submit two patches for this;
> > > >
> > > > 	Remove DPRINT and change it to printk
> > >
> > > No, use dev_dbg() and friends instead of "raw" printk() calls.
> > >
> > 
> > Will do, you caught me just as I was starting the conversion :)
> 
> While cleaning this up there are a few places in vmbus and channel behavior
> where it is not in a device context. Are printk's okay in that context?

No, use pr_* instead for those.  But those should be quite rare, as you
should almost always have a device you are operating on, right?

The reason you don't use "raw" printk() is the dev_dbg() and pr_debug()
calls tie into the dynamic debugging core, which you want to use, as you
don't want to roll your own special way of doing debugging.

> The three drivers network/SCSI and Block of course will use dev_XX family.

Good, but note, I think the network has their own version of this macro
as well :)

thanks,

greg k-h

Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Tue, Feb 15, 2011 at 04:53:41PM +0000, KY Srinivasan wrote:
> > > @@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
> > >  	}
> > >
> > >  	/* Get the interrupt resource */
> > > -	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > > -			  driver->name, NULL);
> > > -
> > > -	if (ret != 0) {
> > > -		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
> > > -			   vmbus_irq);
> > > -
> > > +get_irq_again:
> > > +	vmbus_irq = vmbus_get_irq();
> > > +	if ((vmbus_irq < 0) || (prev_irq == vmbus_irq)) {
> > > +		printk(KERN_WARNING "VMBUS_DRV: Failed to allocate IRQ\n");
> > >  		bus_unregister(&vmbus_drv_ctx->bus);
> > > -
> > >  		ret = -1;
> > 
> > Please provide a "real" error code.
> 
> Will do.
> > 
> > >  		goto cleanup;
> > >  	}
> > > -	vector = VMBUS_IRQ_VECTOR;
> > > +	prev_irq = vmbus_irq;
> > > +
> > > +	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > > +			  driver->name, NULL);
> > > +
> > > +	if (ret != 0)
> > > +		goto get_irq_again;
> > 
> > You just set up the potential for an endless loop.  You almost _never_
> > want to goto backwards in a function.  Please don't do this.
> 
> The loop is not endless. We need to take care of two conditions here:
> 1) From the point we selected an irq line to the point where we call
> request_irq(), it is possible that someone else could have requested
> the same line and so we need to close this window.
> 2) request_irq() may fail for reasons other than the fact that we
> may have lost the line that we thought we got.
> 
> So, while we loopback, we check to see if the irq line we got is the same 
> that we got before (refer to the check soon after the call
> to vmbus_get_irq()). This check would ensure that we would not loop
> endlessly.

That's very subtle, and easy to overlook.  So please don't do it.  Make
it obvious that you will not constantly loop, and again, don't call goto
backwards.  goto is for error handling only, unless you are writing
scheduler code, and you aren't doing that here.

Please rework your logic before resending this.

thanks,

greg k-h

[PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[K. Y. Srinivasan]

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/vmbus_drv.c |   72 +++++++++++++++++++++++++++++++---------
 1 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..441ce85 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -36,9 +36,7 @@
 #include "vmbus_private.h"
 
 
-/* FIXME! We need to do this dynamically for PIC and APIC system */
-#define VMBUS_IRQ		0x5
-#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
+static int vmbus_irq;
 
 /* Main vmbus driver data structure */
 struct vmbus_driver_context {
@@ -57,6 +55,27 @@ struct vmbus_driver_context {
 	struct vm_device device_ctx;
 };
 
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available;
+ * return -EBUSY.
+ */
+static int vmbus_get_irq(void)
+{
+	unsigned int avail_irq_mask;
+	int irq = -EBUSY;
+
+	/*
+	 * Pick the first unused interrupt. HyperV can
+	 * interrupt us on any interrupt line we specify.
+	 */
+
+	avail_irq_mask = probe_irq_on();
+	if (avail_irq_mask != 0)
+		irq = ffs(avail_irq_mask);
+	probe_irq_off(avail_irq_mask);
+	return irq;
+}
+
 static int vmbus_match(struct device *device, struct device_driver *driver);
 static int vmbus_probe(struct device *device);
 static int vmbus_remove(struct device *device);
@@ -80,7 +99,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
 	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
 	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
 
-static int vmbus_irq = VMBUS_IRQ;
 
 /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
 static struct device_attribute vmbus_device_attrs[] = {
@@ -467,6 +485,7 @@ static int vmbus_bus_init(void)
 	struct vm_device *dev_ctx = &vmbus_drv.device_ctx;
 	int ret;
 	unsigned int vector;
+	bool irq_assigned = false;
 
 	DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
 		    HV_DRV_VERSION);
@@ -517,20 +536,42 @@ static int vmbus_bus_init(void)
 		goto cleanup;
 	}
 
-	/* Get the interrupt resource */
-	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
-			  driver->name, NULL);
-
-	if (ret != 0) {
-		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
-			   vmbus_irq);
+	/*
+	 * Get an unused interrupt line and register our handler.
+	 * Since there is a window between getting an unused
+	 * interrupt line and registering our handler, we close
+	 * this window by repeating the sequence if we raced.
+	 * This loop will terminate under the following conditions:
+	 *
+	 * 1) If we succeed in registering our handler OR
+	 * 2) If there are no free interrupt lines for our use OR
+	 * 3) request_irq fails for reasons other than an irq collision
+	 */
+	while (!irq_assigned) {
+		vmbus_irq = vmbus_get_irq();
 
-		bus_unregister(&vmbus_drv_ctx->bus);
+		if (vmbus_irq < 0) {
+			bus_unregister(&vmbus_drv_ctx->bus);
+			ret = -EBUSY;
+			goto cleanup;
+		}
 
-		ret = -1;
-		goto cleanup;
+		ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+			  driver->name, NULL);
+		switch (ret) {
+		case -EBUSY:
+			 /* We raced and lost; try again.*/
+			continue;
+		case 0:
+			irq_assigned = true;
+			break;
+		default:
+			/* Failed to request_irq; cleanup */
+			goto cleanup;
+		}
 	}
-	vector = VMBUS_IRQ_VECTOR;
+
+	vector = IRQ0_VECTOR + vmbus_irq;
 
 	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
 
@@ -1117,7 +1158,6 @@ MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
 
 MODULE_LICENSE("GPL");
 MODULE_VERSION(HV_DRV_VERSION);
-module_param(vmbus_irq, int, S_IRUGO);
 module_param(vmbus_loglevel, int, S_IRUGO);
 
 module_init(vmbus_init);
-- 
1.5.5.6

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

You didn't run this through checkpatch.pl.

Please do so and fix the warning it gives you.

thanks,

greg k-h

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, February 18, 2011 4:14 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> You didn't run this through checkpatch.pl.
> 
> Please do so and fix the warning it gives you.
Greg, I did run the checkpatch script against this patch and the only complaint I got was with 
regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only
external event that the VM is going to see and so I chose to keep this flag.  Is there something that would 
replace this flag; looking at the Xen drivers they do pass this flag.

Regards,

K. Y 

> 
> thanks,
> 
> greg k-h

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Fri, Feb 18, 2011 at 10:00:04PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Friday, February 18, 2011 4:14 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > 
> > On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > 
> > You didn't run this through checkpatch.pl.
> > 
> > Please do so and fix the warning it gives you.
> Greg, I did run the checkpatch script against this patch and the only
> complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I
> pass. As a virtual machine, this is the only external event that the
> VM is going to see and so I chose to keep this flag.  Is there
> something that would replace this flag; looking at the Xen drivers
> they do pass this flag.

But that flag is going away, right?  And this really can't be a valid
source of entropy as the HV channel is pretty predictable.

If you are only using this because Xen does/did it, that's not a valid
excuse :)

thanks,

greg k-h

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Friday, February 18, 2011 5:07 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Fri, Feb 18, 2011 at 10:00:04PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Friday, February 18, 2011 4:14 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > >
> > > On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > >
> > > You didn't run this through checkpatch.pl.
> > >
> > > Please do so and fix the warning it gives you.
> > Greg, I did run the checkpatch script against this patch and the only
> > complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I
> > pass. As a virtual machine, this is the only external event that the
> > VM is going to see and so I chose to keep this flag.  Is there
> > something that would replace this flag; looking at the Xen drivers
> > they do pass this flag.
> 
> But that flag is going away, right?  And this really can't be a valid
> source of entropy as the HV channel is pretty predictable.

Is it going away? What would replace this. Is all interrupt sources considered
predictable? This is the only unpredictable thing happening in the VM and that is the reason
I chose to keep the flag.
> 
> If you are only using this because Xen does/did it, that's not a valid
> excuse :)
Surely, you are joking. In any event I am sending you a new patch with that flag removed.

Regards,

K. Y


> 
> thanks,
> 
> greg k-h

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Fri, Feb 18, 2011 at 10:16:05PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Friday, February 18, 2011 5:07 PM
> > To: KY Srinivasan
> > Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org
> > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > 
> > On Fri, Feb 18, 2011 at 10:00:04PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:greg@kroah.com]
> > > > Sent: Friday, February 18, 2011 4:14 PM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > > >
> > > > On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> > > > >
> > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > >
> > > > You didn't run this through checkpatch.pl.
> > > >
> > > > Please do so and fix the warning it gives you.
> > > Greg, I did run the checkpatch script against this patch and the only
> > > complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I
> > > pass. As a virtual machine, this is the only external event that the
> > > VM is going to see and so I chose to keep this flag.  Is there
> > > something that would replace this flag; looking at the Xen drivers
> > > they do pass this flag.
> > 
> > But that flag is going away, right?  And this really can't be a valid
> > source of entropy as the HV channel is pretty predictable.
> 
> Is it going away? What would replace this. Is all interrupt sources considered
> predictable?

Did you read the file that the checkpatch script told you to about this
entry?

> This is the only unpredictable thing happening in the VM and that is the reason
> I chose to keep the flag.

If you remove it, do we loose all entropy for the VM?

> > If you are only using this because Xen does/did it, that's not a valid
> > excuse :)
> Surely, you are joking.

Not at all.

> In any event I am sending you a new patch with that flag removed.

Have you tested to see if you now loose all entropy, and it causes
problems or not?

thanks,

greg k-h

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Friday, February 18, 2011 5:29 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Fri, Feb 18, 2011 at 10:16:05PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@suse.de]
> > > Sent: Friday, February 18, 2011 5:07 PM
> > > To: KY Srinivasan
> > > Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org
> > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > >
> > > On Fri, Feb 18, 2011 at 10:00:04PM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:greg@kroah.com]
> > > > > Sent: Friday, February 18, 2011 4:14 PM
> > > > > To: KY Srinivasan
> > > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > > > >
> > > > > On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> > > > > >
> > > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > > >
> > > > > You didn't run this through checkpatch.pl.
> > > > >
> > > > > Please do so and fix the warning it gives you.
> > > > Greg, I did run the checkpatch script against this patch and the only
> > > > complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I
> > > > pass. As a virtual machine, this is the only external event that the
> > > > VM is going to see and so I chose to keep this flag.  Is there
> > > > something that would replace this flag; looking at the Xen drivers
> > > > they do pass this flag.
> > >
> > > But that flag is going away, right?  And this really can't be a valid
> > > source of entropy as the HV channel is pretty predictable.
> >
> > Is it going away? What would replace this. Is all interrupt sources considered
> > predictable?
> 
> Did you read the file that the checkpatch script told you to about this
> entry?

It is only after reading the document, I decided to keep that flag. Please
note this is not a question of some interrupt sources not being 
a good source of entropy; for this VM this is the only source of interrupts.
The document on this flag talked about how people were incorrectly
marking their interrupt as an entropy source; in this case there is not much of a
choice. 

> 
> > This is the only unpredictable thing happening in the VM and that is the reason
> > I chose to keep the flag.
> 
> If you remove it, do we loose all entropy for the VM?
> 
> > > If you are only using this because Xen does/did it, that's not a valid
> > > excuse :)
> > Surely, you are joking.
> 
> Not at all.

To set the record straight here, this flag is in the existing code.
After I ran checkpatch, I toyed with the idea of getting rid of this. Then I 
decided to keep it for all the reasons I mentioned earlier. 

> 
> > In any event I am sending you a new patch with that flag removed.
> 
> Have you tested to see if you now loose all entropy, and it causes
> problems or not?

I am glad you asked me to test it. When I remove this flag, the entropy goes down 
significantly and this is not surprising. Looking at 
/proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple
of compiles the number would be in thousands. With that flag removed,
I have the VM up for about an hour, even after a couple of compiles,
the entropy number is yet to crack 200.

Let me know how you want to proceed here.

Regards,

K. Y

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Sat, Feb 19, 2011 at 12:56:16AM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Friday, February 18, 2011 5:29 PM
> > To: KY Srinivasan
> > Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org
> > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > 
> > On Fri, Feb 18, 2011 at 10:16:05PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@suse.de]
> > > > Sent: Friday, February 18, 2011 5:07 PM
> > > > To: KY Srinivasan
> > > > Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > > virtualization@lists.osdl.org
> > > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > > >
> > > > On Fri, Feb 18, 2011 at 10:00:04PM +0000, KY Srinivasan wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Greg KH [mailto:greg@kroah.com]
> > > > > > Sent: Friday, February 18, 2011 4:14 PM
> > > > > > To: KY Srinivasan
> > > > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > > > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > > > > >
> > > > > > On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> > > > > > >
> > > > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > > > >
> > > > > > You didn't run this through checkpatch.pl.
> > > > > >
> > > > > > Please do so and fix the warning it gives you.
> > > > > Greg, I did run the checkpatch script against this patch and the only
> > > > > complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I
> > > > > pass. As a virtual machine, this is the only external event that the
> > > > > VM is going to see and so I chose to keep this flag.  Is there
> > > > > something that would replace this flag; looking at the Xen drivers
> > > > > they do pass this flag.
> > > >
> > > > But that flag is going away, right?  And this really can't be a valid
> > > > source of entropy as the HV channel is pretty predictable.
> > >
> > > Is it going away? What would replace this. Is all interrupt sources considered
> > > predictable?
> > 
> > Did you read the file that the checkpatch script told you to about this
> > entry?
> 
> It is only after reading the document, I decided to keep that flag. Please
> note this is not a question of some interrupt sources not being 
> a good source of entropy; for this VM this is the only source of interrupts.
> The document on this flag talked about how people were incorrectly
> marking their interrupt as an entropy source; in this case there is not much of a
> choice. 
> 
> > 
> > > This is the only unpredictable thing happening in the VM and that is the reason
> > > I chose to keep the flag.
> > 
> > If you remove it, do we loose all entropy for the VM?
> > 
> > > > If you are only using this because Xen does/did it, that's not a valid
> > > > excuse :)
> > > Surely, you are joking.
> > 
> > Not at all.
> 
> To set the record straight here, this flag is in the existing code.
> After I ran checkpatch, I toyed with the idea of getting rid of this. Then I 
> decided to keep it for all the reasons I mentioned earlier. 
> 
> > 
> > > In any event I am sending you a new patch with that flag removed.
> > 
> > Have you tested to see if you now loose all entropy, and it causes
> > problems or not?
> 
> I am glad you asked me to test it. When I remove this flag, the entropy goes down 
> significantly and this is not surprising. Looking at 
> /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple
> of compiles the number would be in thousands. With that flag removed,
> I have the VM up for about an hour, even after a couple of compiles,
> the entropy number is yet to crack 200.
> 
> Let me know how you want to proceed here.

Ok, my fault, let's keep the original flag.  Care to resend the patch
you had originally sent?

thanks,

greg k-h

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Friday, February 18, 2011 8:03 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Sat, Feb 19, 2011 at 12:56:16AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@suse.de]
> > > Sent: Friday, February 18, 2011 5:29 PM
> > > To: KY Srinivasan
> > > Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org
> > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > >
> > > On Fri, Feb 18, 2011 at 10:16:05PM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:gregkh@suse.de]
> > > > > Sent: Friday, February 18, 2011 5:07 PM
> > > > > To: KY Srinivasan
> > > > > Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > > > virtualization@lists.osdl.org
> > > > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > > > >
> > > > > On Fri, Feb 18, 2011 at 10:00:04PM +0000, KY Srinivasan wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Greg KH [mailto:greg@kroah.com]
> > > > > > > Sent: Friday, February 18, 2011 4:14 PM
> > > > > > > To: KY Srinivasan
> > > > > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > > > > > Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> > > > > > >
> > > > > > > On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote:
> > > > > > > >
> > > > > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > > > > >
> > > > > > > You didn't run this through checkpatch.pl.
> > > > > > >
> > > > > > > Please do so and fix the warning it gives you.
> > > > > > Greg, I did run the checkpatch script against this patch and the only
> > > > > > complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag
> that I
> > > > > > pass. As a virtual machine, this is the only external event that the
> > > > > > VM is going to see and so I chose to keep this flag.  Is there
> > > > > > something that would replace this flag; looking at the Xen drivers
> > > > > > they do pass this flag.
> > > > >
> > > > > But that flag is going away, right?  And this really can't be a valid
> > > > > source of entropy as the HV channel is pretty predictable.
> > > >
> > > > Is it going away? What would replace this. Is all interrupt sources considered
> > > > predictable?
> > >
> > > Did you read the file that the checkpatch script told you to about this
> > > entry?
> >
> > It is only after reading the document, I decided to keep that flag. Please
> > note this is not a question of some interrupt sources not being
> > a good source of entropy; for this VM this is the only source of interrupts.
> > The document on this flag talked about how people were incorrectly
> > marking their interrupt as an entropy source; in this case there is not much of a
> > choice.
> >
> > >
> > > > This is the only unpredictable thing happening in the VM and that is the
> reason
> > > > I chose to keep the flag.
> > >
> > > If you remove it, do we loose all entropy for the VM?
> > >
> > > > > If you are only using this because Xen does/did it, that's not a valid
> > > > > excuse :)
> > > > Surely, you are joking.
> > >
> > > Not at all.
> >
> > To set the record straight here, this flag is in the existing code.
> > After I ran checkpatch, I toyed with the idea of getting rid of this. Then I
> > decided to keep it for all the reasons I mentioned earlier.
> >
> > >
> > > > In any event I am sending you a new patch with that flag removed.
> > >
> > > Have you tested to see if you now loose all entropy, and it causes
> > > problems or not?
> >
> > I am glad you asked me to test it. When I remove this flag, the entropy goes
> down
> > significantly and this is not surprising. Looking at
> > /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple
> > of compiles the number would be in thousands. With that flag removed,
> > I have the VM up for about an hour, even after a couple of compiles,
> > the entropy number is yet to crack 200.
> >
> > Let me know how you want to proceed here.
> 
> Ok, my fault, let's keep the original flag.  Care to resend the patch
> you had originally sent?
Thanks Greg; I just sent it. I will deal with irq balancing issue in a separate patch.

Regards,

K. Y

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Thomas Gleixner]

On Tue, 15 Feb 2011, K. Y. Srinivasan wrote:
> -/* FIXME! We need to do this dynamically for PIC and APIC system */
> -#define VMBUS_IRQ		0x5
> -#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
> +static int vmbus_irq;
>  
>  /* Main vmbus driver data structure */
>  struct vmbus_driver_context {
> @@ -57,6 +55,27 @@ struct vmbus_driver_context {
>  	struct vm_device device_ctx;
>  };
>  
> +/*
> + * Find an un-used IRQ that the VMBUS can use. If none is available;
> + * return -EBUSY.
> + */
> +static int vmbus_get_irq(void)
> +{
> +	unsigned int avail_irq_mask;
> +	int irq = -EBUSY;
> +
> +	/*
> +	 * Pick the first unused interrupt. HyperV can
> +	 * interrupt us on any interrupt line we specify.
> +	 */
> +
> +	avail_irq_mask = probe_irq_on();
> +	if (avail_irq_mask != 0)
> +		irq = ffs(avail_irq_mask);
> +	probe_irq_off(avail_irq_mask);
> +	return irq;


Please do not use probe_irq_on for dynamic irq allocation. Highjacking
the lower PIC irqs is really not a good idea. Depending on when this
runs, you might grab an irq required by a driver which gets loaded
later. 

Could you please explain what you're trying to do here ?

Thanks,

	tglx

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Saturday, February 19, 2011 5:23 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Tue, 15 Feb 2011, K. Y. Srinivasan wrote:
> > -/* FIXME! We need to do this dynamically for PIC and APIC system */
> > -#define VMBUS_IRQ		0x5
> > -#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
> > +static int vmbus_irq;
> >
> >  /* Main vmbus driver data structure */
> >  struct vmbus_driver_context {
> > @@ -57,6 +55,27 @@ struct vmbus_driver_context {
> >  	struct vm_device device_ctx;
> >  };
> >
> > +/*
> > + * Find an un-used IRQ that the VMBUS can use. If none is available;
> > + * return -EBUSY.
> > + */
> > +static int vmbus_get_irq(void)
> > +{
> > +	unsigned int avail_irq_mask;
> > +	int irq = -EBUSY;
> > +
> > +	/*
> > +	 * Pick the first unused interrupt. HyperV can
> > +	 * interrupt us on any interrupt line we specify.
> > +	 */
> > +
> > +	avail_irq_mask = probe_irq_on();
> > +	if (avail_irq_mask != 0)
> > +		irq = ffs(avail_irq_mask);
> > +	probe_irq_off(avail_irq_mask);
> > +	return irq;
> 
> 
> Please do not use probe_irq_on for dynamic irq allocation. Highjacking
> the lower PIC irqs is really not a good idea. Depending on when this
> runs, you might grab an irq required by a driver which gets loaded
> later.
> 
> Could you please explain what you're trying to do here ?

The IRQ being allocated is for the VMBUS driver for Linux guests running on
a Windows virtualization platform (Hyper-V hypervisor).
The hypervisor is capable of notifying events on the VMBUS via
a guest specified interrupt line. Prior to this patch,
the code was statically selecting an interrupt line for
use by VMBUS. One of the long standing review comments
on  that code was to make this irq allocation dynamic and that
is what this patch does. For the Linux guest running as a VM
on Hyper-V, the concern you raise is not an issue.

Regards,

K. Y

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Thomas Gleixner]

On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > Please do not use probe_irq_on for dynamic irq allocation. Highjacking
> > the lower PIC irqs is really not a good idea. Depending on when this
> > runs, you might grab an irq required by a driver which gets loaded
> > later.
> > 
> > Could you please explain what you're trying to do here ?
> 
> The IRQ being allocated is for the VMBUS driver for Linux guests running on
> a Windows virtualization platform (Hyper-V hypervisor).
> The hypervisor is capable of notifying events on the VMBUS via
> a guest specified interrupt line. Prior to this patch,
> the code was statically selecting an interrupt line for
> use by VMBUS. One of the long standing review comments
> on  that code was to make this irq allocation dynamic and that
> is what this patch does. For the Linux guest running as a VM
> on Hyper-V, the concern you raise is not an issue.

That patch does a whole lot of useless crap. 

When grabbing some random irq from the PIC is not an issue, then
what's the point of this probing, retry loop and the comments about
racing ? What races here? That does not make sense at all.

I don't know why the previous reviewer wanted to have that
dynamic. That just does not make sense to me.

Btw, that whole interrupt handler with two tasklets, one of them
scheduling work is just screaming threaded interrupt handler.

Thanks,

	tglx

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Saturday, February 19, 2011 10:12 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > Please do not use probe_irq_on for dynamic irq allocation. Highjacking
> > > the lower PIC irqs is really not a good idea. Depending on when this
> > > runs, you might grab an irq required by a driver which gets loaded
> > > later.
> > >
> > > Could you please explain what you're trying to do here ?
> >
> > The IRQ being allocated is for the VMBUS driver for Linux guests running on
> > a Windows virtualization platform (Hyper-V hypervisor).
> > The hypervisor is capable of notifying events on the VMBUS via
> > a guest specified interrupt line. Prior to this patch,
> > the code was statically selecting an interrupt line for
> > use by VMBUS. One of the long standing review comments
> > on  that code was to make this irq allocation dynamic and that
> > is what this patch does. For the Linux guest running as a VM
> > on Hyper-V, the concern you raise is not an issue.
> 
> That patch does a whole lot of useless crap.
> 
> When grabbing some random irq from the PIC is not an issue, then
> what's the point of this probing, retry loop and the comments about
> racing ? What races here? That does not make sense at all.

Like most virtualization platforms, Hyper-V also emulates the full PC 
platform. So, it is possible that the driver of some other emulated
devices might register for the IRQ line we might have selected. That 
is the race this code addresses. For performance reasons, we want
both storage and network traffic to go over the PV drivers. 

> 
> I don't know why the previous reviewer wanted to have that
> dynamic. That just does not make sense to me.

Prior to this patch, we had a hard coded interrupt line for use by
this driver. If that line was already in use, the load of this driver 
would fail. This would be a fatal issue especially for distributions
that have embedded these PV drivers as part of their installation
media. This patch deals with such collisions in a more graceful way -
we would not bail until we have scanned all low interrupt lines.
 
> 
> Btw, that whole interrupt handler with two tasklets, one of them
> scheduling work is just screaming threaded interrupt handler.

We are in the process of cleaning up these drivers; I am looking at
some of these and other issues.

Regards,

K. Y  

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Thomas Gleixner]

On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > When grabbing some random irq from the PIC is not an issue, then
> > what's the point of this probing, retry loop and the comments about
> > racing ? What races here? That does not make sense at all.
> 
> Like most virtualization platforms, Hyper-V also emulates the full PC 
> platform. So, it is possible that the driver of some other emulated
> devices might register for the IRQ line we might have selected. That 
> is the race this code addresses. For performance reasons, we want
> both storage and network traffic to go over the PV drivers. 

So in case your driver gets the interrupt line first, which the other
driver wants to acquire as well, then what? Do you want to do that
probe magic in the other driver as well? What if this is a regular
device driver which gets its irq number from ACPI/PCI or
whatever. Then that driver simply wont work as it's interrupt line is
busy.

> > 
> > I don't know why the previous reviewer wanted to have that
> > dynamic. That just does not make sense to me.
> 
> Prior to this patch, we had a hard coded interrupt line for use by
> this driver. If that line was already in use, the load of this driver 
> would fail. This would be a fatal issue especially for distributions
> that have embedded these PV drivers as part of their installation
> media. This patch deals with such collisions in a more graceful way -
> we would not bail until we have scanned all low interrupt lines.

So you trade breaking the PV stuff against breaking random other
drivers? That doesn't sound like a brilliant idea.

There are various ways to solve that proper.

 - You can provide the interrupt number from ACPI/PCI or whatever your HV
   provides as enumeration.

 - Use a fixed vector like XEN does for the event channel

 - Use dynamic allocation in the IOAPIC space like the kernel does for
   MSI(-X)

Thanks,

	tglx

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Sunday, February 20, 2011 11:16 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > > When grabbing some random irq from the PIC is not an issue, then
> > > what's the point of this probing, retry loop and the comments about
> > > racing ? What races here? That does not make sense at all.
> >
> > Like most virtualization platforms, Hyper-V also emulates the full PC
> > platform. So, it is possible that the driver of some other emulated
> > devices might register for the IRQ line we might have selected. That
> > is the race this code addresses. For performance reasons, we want
> > both storage and network traffic to go over the PV drivers.
> 
> So in case your driver gets the interrupt line first, which the other
> driver wants to acquire as well, then what? Do you want to do that
> probe magic in the other driver as well? What if this is a regular
> device driver which gets its irq number from ACPI/PCI or
> whatever. Then that driver simply wont work as it's interrupt line is
> busy.
> 
> > >
> > > I don't know why the previous reviewer wanted to have that
> > > dynamic. That just does not make sense to me.
> >
> > Prior to this patch, we had a hard coded interrupt line for use by
> > this driver. If that line was already in use, the load of this driver
> > would fail. This would be a fatal issue especially for distributions
> > that have embedded these PV drivers as part of their installation
> > media. This patch deals with such collisions in a more graceful way -
> > we would not bail until we have scanned all low interrupt lines.
> 
> So you trade breaking the PV stuff against breaking random other
> drivers? That doesn't sound like a brilliant idea.
> 
> There are various ways to solve that proper.
> 
>  - You can provide the interrupt number from ACPI/PCI or whatever your HV
>    provides as enumeration.
> 
>  - Use a fixed vector like XEN does for the event channel
> 
>  - Use dynamic allocation in the IOAPIC space like the kernel does for
>    MSI(-X)
> 
> Thanks,
> 
> 	tglx

I am not claiming that what I have done here is the best possible solution.
However, I will submit to you that it is better than what we had here
prior to this patch.  I will address this and a  whole lot of other issues 
in future patches. 

Regards,

K. Y

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Mon, Feb 21, 2011 at 03:43:58AM +0000, KY Srinivasan wrote:
> I am not claiming that what I have done here is the best possible solution.
> However, I will submit to you that it is better than what we had here
> prior to this patch.  I will address this and a  whole lot of other issues 
> in future patches. 

Care to document it in the TODO file so it doesn't get forgotten?

thanks,

greg k-h

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Thomas Gleixner]

On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > Like most virtualization platforms, Hyper-V also emulates the full PC
> > > platform. So, it is possible that the driver of some other emulated
> > > devices might register for the IRQ line we might have selected. That
> > > is the race this code addresses. For performance reasons, we want
> > > both storage and network traffic to go over the PV drivers.
> > 
> > So in case your driver gets the interrupt line first, which the other
> > driver wants to acquire as well, then what? Do you want to do that
> > probe magic in the other driver as well? What if this is a regular
> > device driver which gets its irq number from ACPI/PCI or
> > whatever. Then that driver simply wont work as it's interrupt line is
> > busy.
> > 
> > > >
> > > > I don't know why the previous reviewer wanted to have that
> > > > dynamic. That just does not make sense to me.
> > >
> > > Prior to this patch, we had a hard coded interrupt line for use by
> > > this driver. If that line was already in use, the load of this driver
> > > would fail. This would be a fatal issue especially for distributions
> > > that have embedded these PV drivers as part of their installation
> > > media. This patch deals with such collisions in a more graceful way -
> > > we would not bail until we have scanned all low interrupt lines.
> > 
> > So you trade breaking the PV stuff against breaking random other
> > drivers? That doesn't sound like a brilliant idea.
> > 
> > There are various ways to solve that proper.
> > 
> >  - You can provide the interrupt number from ACPI/PCI or whatever your HV
> >    provides as enumeration.
> > 
> >  - Use a fixed vector like XEN does for the event channel
> > 
> >  - Use dynamic allocation in the IOAPIC space like the kernel does for
> >    MSI(-X)
> > 
> > Thanks,
> > 
> > 	tglx
> 
> I am not claiming that what I have done here is the best possible solution.
> However, I will submit to you that it is better than what we had here
> prior to this patch.  I will address this and a  whole lot of other issues 
> in future patches. 

No, it's _NOT_ better in any way. You trade breaking your PV thing
against breaking random other drivers. Care to explain why you think
that's better ?

Thanks,

	tglx

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Monday, February 21, 2011 6:03 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > Like most virtualization platforms, Hyper-V also emulates the full PC
> > > > platform. So, it is possible that the driver of some other emulated
> > > > devices might register for the IRQ line we might have selected. That
> > > > is the race this code addresses. For performance reasons, we want
> > > > both storage and network traffic to go over the PV drivers.
> > >
> > > So in case your driver gets the interrupt line first, which the other
> > > driver wants to acquire as well, then what? Do you want to do that
> > > probe magic in the other driver as well? What if this is a regular
> > > device driver which gets its irq number from ACPI/PCI or
> > > whatever. Then that driver simply wont work as it's interrupt line is
> > > busy.
> > >
> > > > >
> > > > > I don't know why the previous reviewer wanted to have that
> > > > > dynamic. That just does not make sense to me.
> > > >
> > > > Prior to this patch, we had a hard coded interrupt line for use by
> > > > this driver. If that line was already in use, the load of this driver
> > > > would fail. This would be a fatal issue especially for distributions
> > > > that have embedded these PV drivers as part of their installation
> > > > media. This patch deals with such collisions in a more graceful way -
> > > > we would not bail until we have scanned all low interrupt lines.
> > >
> > > So you trade breaking the PV stuff against breaking random other
> > > drivers? That doesn't sound like a brilliant idea.
> > >
> > > There are various ways to solve that proper.
> > >
> > >  - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > >    provides as enumeration.
> > >
> > >  - Use a fixed vector like XEN does for the event channel
> > >
> > >  - Use dynamic allocation in the IOAPIC space like the kernel does for
> > >    MSI(-X)
> > >
> > > Thanks,
> > >
> > > 	tglx
> >
> > I am not claiming that what I have done here is the best possible solution.
> > However, I will submit to you that it is better than what we had here
> > prior to this patch.  I will address this and a  whole lot of other issues
> > in future patches.
> 
> No, it's _NOT_ better in any way. You trade breaking your PV thing
> against breaking random other drivers. Care to explain why you think
> that's better ?

The root device for the VM is bound to the PV driver on some distributions.
So, if we cannot load the PV drivers, we do not have a system that boots.
In general, the system performance without these PV drivers is so poor that
for all practical purposes, having the PV drivers is almost a requirement
for having a useable system. While the platform supports configuration of the VM with
some emulated devices, it is not a recommended configuration (because of
performance reasons) for Linux virtual machines on the Hyper-V platform.

I have spent significantly more time debating this patch than developing this patch
that I still think improves the current driver. I will leave it to  Greg and other powers 
that be to decide if this patch will be accepted. Let me know what your verdict is. 

Regards,

K. Y 

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Thomas Gleixner]

On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > There are various ways to solve that proper.
> > > >
> > > >  - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > >    provides as enumeration.
> > > >
> > > >  - Use a fixed vector like XEN does for the event channel
> > > >
> > > >  - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > >    MSI(-X)
> > > >
> > > > Thanks,
> > > >
> > > > 	tglx
> > >
> > > I am not claiming that what I have done here is the best possible solution.
> > > However, I will submit to you that it is better than what we had here
> > > prior to this patch.  I will address this and a  whole lot of other issues
> > > in future patches.
> > 
> > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > against breaking random other drivers. Care to explain why you think
> > that's better ?
> 
> The root device for the VM is bound to the PV driver on some distributions.
> So, if we cannot load the PV drivers, we do not have a system that boots.
> In general, the system performance without these PV drivers is so poor that
> for all practical purposes, having the PV drivers is almost a requirement
> for having a useable system. While the platform supports configuration of the VM with
> some emulated devices, it is not a recommended configuration (because of
> performance reasons) for Linux virtual machines on the Hyper-V platform.

It does not matter whether it's recommended or not. If it results in a
non usable emulated device it's broken. Just imagine you grab the
serial interrupt, which results in not having a debug console. Not at
all acceptable.
 
> I have spent significantly more time debating this patch than
> developing this patch that I still think improves the current
> driver. I will leave it to Greg and other powers that be to decide
> if this patch will be accepted. Let me know what your verdict is.

As I said, any other clean solution (and I pointed out 3) over this
hack.

Thanks,

	tglx

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Mon, Feb 21, 2011 at 03:51:56PM +0100, Thomas Gleixner wrote:
> On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > > There are various ways to solve that proper.
> > > > >
> > > > >  - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > > >    provides as enumeration.
> > > > >
> > > > >  - Use a fixed vector like XEN does for the event channel
> > > > >
> > > > >  - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > >    MSI(-X)
> > > > >
> > > > > Thanks,
> > > > >
> > > > > 	tglx
> > > >
> > > > I am not claiming that what I have done here is the best possible solution.
> > > > However, I will submit to you that it is better than what we had here
> > > > prior to this patch.  I will address this and a  whole lot of other issues
> > > > in future patches.
> > > 
> > > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > > against breaking random other drivers. Care to explain why you think
> > > that's better ?
> > 
> > The root device for the VM is bound to the PV driver on some distributions.
> > So, if we cannot load the PV drivers, we do not have a system that boots.
> > In general, the system performance without these PV drivers is so poor that
> > for all practical purposes, having the PV drivers is almost a requirement
> > for having a useable system. While the platform supports configuration of the VM with
> > some emulated devices, it is not a recommended configuration (because of
> > performance reasons) for Linux virtual machines on the Hyper-V platform.
> 
> It does not matter whether it's recommended or not. If it results in a
> non usable emulated device it's broken. Just imagine you grab the
> serial interrupt, which results in not having a debug console. Not at
> all acceptable.
>  
> > I have spent significantly more time debating this patch than
> > developing this patch that I still think improves the current
> > driver. I will leave it to Greg and other powers that be to decide
> > if this patch will be accepted. Let me know what your verdict is.
> 
> As I said, any other clean solution (and I pointed out 3) over this
> hack.

Yeah, that sounds like the best solution, let's not cause accidental
breakage of systems that were working already.

KY, care to fix this up properly now?  There's no rush to take your
previous patch, as that is working ok for now, right?

thanks,

greg k-h

Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[Greg KH]

On Mon, Feb 21, 2011 at 03:51:56PM +0100, Thomas Gleixner wrote:
> On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > > There are various ways to solve that proper.
> > > > >
> > > > >  - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > > >    provides as enumeration.
> > > > >
> > > > >  - Use a fixed vector like XEN does for the event channel
> > > > >
> > > > >  - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > >    MSI(-X)
> > > > >
> > > > > Thanks,
> > > > >
> > > > > 	tglx
> > > >
> > > > I am not claiming that what I have done here is the best possible solution.
> > > > However, I will submit to you that it is better than what we had here
> > > > prior to this patch.  I will address this and a  whole lot of other issues
> > > > in future patches.
> > > 
> > > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > > against breaking random other drivers. Care to explain why you think
> > > that's better ?
> > 
> > The root device for the VM is bound to the PV driver on some distributions.
> > So, if we cannot load the PV drivers, we do not have a system that boots.
> > In general, the system performance without these PV drivers is so poor that
> > for all practical purposes, having the PV drivers is almost a requirement
> > for having a useable system. While the platform supports configuration of the VM with
> > some emulated devices, it is not a recommended configuration (because of
> > performance reasons) for Linux virtual machines on the Hyper-V platform.
> 
> It does not matter whether it's recommended or not. If it results in a
> non usable emulated device it's broken. Just imagine you grab the
> serial interrupt, which results in not having a debug console. Not at
> all acceptable.
>  
> > I have spent significantly more time debating this patch than
> > developing this patch that I still think improves the current
> > driver. I will leave it to Greg and other powers that be to decide
> > if this patch will be accepted. Let me know what your verdict is.
> 
> As I said, any other clean solution (and I pointed out 3) over this
> hack.

I agree, KY, please fix this up properly, I've dropped this patch from
my queue now.

thanks,

greg k-h

RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, February 23, 2011 2:16 PM
> To: Thomas Gleixner
> Cc: KY Srinivasan; gregkh@suse.de; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org; devel@linuxdriverproject.org
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
> 
> On Mon, Feb 21, 2011 at 03:51:56PM +0100, Thomas Gleixner wrote:
> > On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > > > There are various ways to solve that proper.
> > > > > >
> > > > > >  - You can provide the interrupt number from ACPI/PCI or whatever
> your HV
> > > > > >    provides as enumeration.
> > > > > >
> > > > > >  - Use a fixed vector like XEN does for the event channel
> > > > > >
> > > > > >  - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > > >    MSI(-X)
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > 	tglx
> > > > >
> > > > > I am not claiming that what I have done here is the best possible solution.
> > > > > However, I will submit to you that it is better than what we had here
> > > > > prior to this patch.  I will address this and a  whole lot of other issues
> > > > > in future patches.
> > > >
> > > > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > > > against breaking random other drivers. Care to explain why you think
> > > > that's better ?
> > >
> > > The root device for the VM is bound to the PV driver on some distributions.
> > > So, if we cannot load the PV drivers, we do not have a system that boots.
> > > In general, the system performance without these PV drivers is so poor that
> > > for all practical purposes, having the PV drivers is almost a requirement
> > > for having a useable system. While the platform supports configuration of the
> VM with
> > > some emulated devices, it is not a recommended configuration (because of
> > > performance reasons) for Linux virtual machines on the Hyper-V platform.
> >
> > It does not matter whether it's recommended or not. If it results in a
> > non usable emulated device it's broken. Just imagine you grab the
> > serial interrupt, which results in not having a debug console. Not at
> > all acceptable.
> >
> > > I have spent significantly more time debating this patch than
> > > developing this patch that I still think improves the current
> > > driver. I will leave it to Greg and other powers that be to decide
> > > if this patch will be accepted. Let me know what your verdict is.
> >
> > As I said, any other clean solution (and I pointed out 3) over this
> > hack.
> 
> I agree, KY, please fix this up properly, I've dropped this patch from
> my queue now.

Ok; will do.

K. Y
> 
> thanks,
> 
> greg k-h

[PATCH]: Staging: hv: Allocate the vmbus irq dynamically

[K. Y. Srinivasan]

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/vmbus_drv.c |   72 +++++++++++++++++++++++++++++++---------
 1 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..441ce85 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -36,9 +36,7 @@
 #include "vmbus_private.h"
 
 
-/* FIXME! We need to do this dynamically for PIC and APIC system */
-#define VMBUS_IRQ		0x5
-#define VMBUS_IRQ_VECTOR	IRQ5_VECTOR
+static int vmbus_irq;
 
 /* Main vmbus driver data structure */
 struct vmbus_driver_context {
@@ -57,6 +55,27 @@ struct vmbus_driver_context {
 	struct vm_device device_ctx;
 };
 
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available;
+ * return -EBUSY.
+ */
+static int vmbus_get_irq(void)
+{
+	unsigned int avail_irq_mask;
+	int irq = -EBUSY;
+
+	/*
+	 * Pick the first unused interrupt. HyperV can
+	 * interrupt us on any interrupt line we specify.
+	 */
+
+	avail_irq_mask = probe_irq_on();
+	if (avail_irq_mask != 0)
+		irq = ffs(avail_irq_mask);
+	probe_irq_off(avail_irq_mask);
+	return irq;
+}
+
 static int vmbus_match(struct device *device, struct device_driver *driver);
 static int vmbus_probe(struct device *device);
 static int vmbus_remove(struct device *device);
@@ -80,7 +99,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
 	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
 	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
 
-static int vmbus_irq = VMBUS_IRQ;
 
 /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
 static struct device_attribute vmbus_device_attrs[] = {
@@ -467,6 +485,7 @@ static int vmbus_bus_init(void)
 	struct vm_device *dev_ctx = &vmbus_drv.device_ctx;
 	int ret;
 	unsigned int vector;
+	bool irq_assigned = false;
 
 	DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
 		    HV_DRV_VERSION);
@@ -517,20 +536,42 @@ static int vmbus_bus_init(void)
 		goto cleanup;
 	}
 
-	/* Get the interrupt resource */
-	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
-			  driver->name, NULL);
-
-	if (ret != 0) {
-		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
-			   vmbus_irq);
+	/*
+	 * Get an unused interrupt line and register our handler.
+	 * Since there is a window between getting an unused
+	 * interrupt line and registering our handler, we close
+	 * this window by repeating the sequence if we raced.
+	 * This loop will terminate under the following conditions:
+	 *
+	 * 1) If we succeed in registering our handler OR
+	 * 2) If there are no free interrupt lines for our use OR
+	 * 3) request_irq fails for reasons other than an irq collision
+	 */
+	while (!irq_assigned) {
+		vmbus_irq = vmbus_get_irq();
 
-		bus_unregister(&vmbus_drv_ctx->bus);
+		if (vmbus_irq < 0) {
+			bus_unregister(&vmbus_drv_ctx->bus);
+			ret = -EBUSY;
+			goto cleanup;
+		}
 
-		ret = -1;
-		goto cleanup;
+		ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+			  driver->name, NULL);
+		switch (ret) {
+		case -EBUSY:
+			 /* We raced and lost; try again.*/
+			continue;
+		case 0:
+			irq_assigned = true;
+			break;
+		default:
+			/* Failed to request_irq; cleanup */
+			goto cleanup;
+		}
 	}
-	vector = VMBUS_IRQ_VECTOR;
+
+	vector = IRQ0_VECTOR + vmbus_irq;
 
 	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
 
@@ -1117,7 +1158,6 @@ MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
 
 MODULE_LICENSE("GPL");
 MODULE_VERSION(HV_DRV_VERSION);
-module_param(vmbus_irq, int, S_IRUGO);
 module_param(vmbus_loglevel, int, S_IRUGO);
 
 module_init(vmbus_init);
-- 
1.5.5.6