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

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxx]
> Sent: Tuesday, February 15, 2011 11:00 AM
> To: KY Srinivasan
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxx; 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@xxxxxxxxxxxxx>
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > Signed-off-by: Hank Janssen <hjanssen@xxxxxxxxxxxxx>
> >
> > ---
> >  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/