Signed-off-by: K. Y. Srinivasan <[email protected]>
Signed-off-by: Haiyang Zhang <[email protected]>
Signed-off-by: Hank Janssen <[email protected]>
---
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
On Tue, Feb 15, 2011 at 07:15:37AM -0800, K. Y. Srinivasan wrote:
>
> Signed-off-by: K. Y. Srinivasan <[email protected]>
> Signed-off-by: Haiyang Zhang <[email protected]>
> Signed-off-by: Hank Janssen <[email protected]>
>
> ---
> 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
> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> Sent: Tuesday, February 15, 2011 11:00 AM
> To: KY Srinivasan
> Cc: [email protected]; [email protected];
> [email protected]; 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 <[email protected]>
> > Signed-off-by: Haiyang Zhang <[email protected]>
> > Signed-off-by: Hank Janssen <[email protected]>
> >
> > ---
> > 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
> -----Original Message-----
> From: KY Srinivasan
> Sent: Tuesday, February 15, 2011 8:54 AM
> > -----Original Message-----
> > From: Greg KH [mailto:[email protected]]
> > Sent: Tuesday, February 15, 2011 11:00 AM
> > To: KY Srinivasan
> > Cc: [email protected]; [email protected];
> > [email protected]; 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.
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:[email protected]]
> > > Sent: Tuesday, February 15, 2011 11:00 AM
> > > To: KY Srinivasan
> > > Cc: [email protected]; [email protected];
> > > [email protected]; 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
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
> -----Original Message-----
> From: Greg KH [mailto:[email protected]]
> 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.
> > -----Original Message-----
> > From: Greg KH [mailto:[email protected]]
> > 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.
On Tue, Feb 15, 2011 at 07:09:34PM +0000, Hank Janssen wrote:
>
> > > -----Original Message-----
> > > From: Greg KH [mailto:[email protected]]
> > > 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