:Staging: hv: Allocate the vmbus irq dynamically
Subsystems (2)
PATCH 1/1 :Staging: hv: Allocate the vmbus irq dynamically
--- drivers/staging/hv/vmbus_drv.c | 49 +++++++++++++++++++++++++++------------ 1 files changed, 34 insertions(+), 15 deletions(-)
drivers/staging/hv/vmbus_drv.c +34 -15
@@ -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);
Signature · 1 line
1.5.5.6
Quoted 25 lines · K. Y. Srinivasan
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(-)drivers/staging/hv/vmbus_drv.c +1
-3@@ -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 {
drivers/staging/hv/vmbus_drv.c struct vmbus_driver_context { · line 62
Quoted 8 lines
drivers/staging/hv/vmbus_drv.c +5
-0@@ -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.
drivers/staging/hv/vmbus_drv.c struct vmbus_driver_context { · line 66
Quoted 4 lines
drivers/staging/hv/vmbus_drv.c +4
-0+{ +unsigned int avail_irq_mask; +int irq =-1 ; +/*
Put an extra blank line after your variables please.
Quoted 38 lines
drivers/staging/hv/vmbus_drv.c +11
-6+* 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 ) {
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 542
Quoted 10 lines
drivers/staging/hv/vmbus_drv.c +4
-4-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.
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 551
Quoted 10 lines
drivers/staging/hv/vmbus_drv.c +7
-1goto 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.
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 553
Quoted 2 lines
drivers/staging/hv/vmbus_drv.c +2
-0+ + vector =IRQ0_VECTOR + vmbus_irq;
What's this for?
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 555
Quoted 1 line
drivers/staging/hv/vmbus_drv.c +0
-0DPRINT_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
Quoted 40 lines
-----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 dynamicallyQuoted 7 lines · K. Y. Srinivasan
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 +++++++++++++++++++++++++++------------
Quoted 21 lines
1 files changed, 34 insertions(+), 15 deletions(-)drivers/staging/hv/vmbus_drv.c +1
-3@@ -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; };
Quoted 7 lines
drivers/staging/hv/vmbus_drv.c struct vmbus_driver_context { · line 62
Quoted 5 lines
drivers/staging/hv/vmbus_drv.c +5
-0+/* +* 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.
Quoted 7 lines
drivers/staging/hv/vmbus_drv.c struct vmbus_driver_context { · line 66
Quoted 5 lines
drivers/staging/hv/vmbus_drv.c +4
-0+{ +unsigned int avail_irq_mask; +int irq =-1 ; +/* Put an extra blank line after your variables please.
Will do.
Quoted 43 lines
Quoted 43 lines
drivers/staging/hv/vmbus_drv.c +12
-9+* 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:
Quoted 8 lines
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 542
Quoted 6 lines
drivers/staging/hv/vmbus_drv.c +3
-1+ 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.
Quoted 14 lines
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 551
Quoted 11 lines
drivers/staging/hv/vmbus_drv.c +7
-1goto 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.
Quoted 5 lines
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 553
Quoted 3 lines
drivers/staging/hv/vmbus_drv.c +2
-0+ + 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.
Quoted 4 lines
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 555
Quoted 2 lines
drivers/staging/hv/vmbus_drv.c +0
-0DPRINT_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
Quoted 4 lines
thanks,
greg k-h
Quoted 14 lines
-----Original Message-----
From: KY Srinivasan
Sent: Tuesday, February 15, 2011 8:54 AMQuoted 8 lines
-----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
Quoted 4 lines
Quoted 1 line
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.
Quoted 14 lines · Hank Janssen
Quoted 13 lines
-----Original Message-----
From: KY Srinivasan
Sent: Tuesday, February 15, 2011 8:54 AMQuoted 8 lines
-----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
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 1 line
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
Quoted 11 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Tuesday, February 15, 2011 9:23 AMQuoted 3 lines
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 :)
Quoted 4 lines
Quoted 1 line
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.
Quoted 4 lines
Quoted 4 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Tuesday, February 15, 2011 9:23 AM
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 8 lines · Hank Janssen
Quoted 8 lines
Quoted 8 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Tuesday, February 15, 2011 9:23 AMQuoted 3 lines
Before the end of the week I will submit two patches for this;
Remove DPRINT and change it to printk
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
The three drivers network/SCSI and Block of course will use
dev_XXfamily.
Good, but note, I think the network has their own version of this macro as well :)
thanks,
greg k-h
Quoted 44 lines · KY Srinivasan
Quoted 12 lines
Quoted 12 lines
drivers/staging/hv/vmbus_drv.c +1
-7@@ -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:Quoted 8 lines
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 542
Quoted 6 lines
drivers/staging/hv/vmbus_drv.c +3
-1+ 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.
Quoted 14 lines
drivers/staging/hv/vmbus_drv.c static int vmbus_bus_init(void) · line 551
Quoted 11 lines
drivers/staging/hv/vmbus_drv.c +7
-1goto 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 callrequest_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.
Quoted 4 lines
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
tovmbus_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