* RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: Hank Janssen @ 2011-02-15 17:28 UTC (permalink / raw)
To: Greg KH
Cc: Hashir Abdi, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org, Mike Sterling,
devel@linuxdriverproject.org
In-Reply-To: <20110215172253.GB18437@suse.de>
> -----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.
^ permalink raw reply
* Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: Greg KH @ 2011-02-15 17:25 UTC (permalink / raw)
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
virtualization@lists.osdl.org, Haiyang Zhang, Hank Janssen
In-Reply-To: <FB42D5CCD7B5934EB1827DB5ED9B850E07085DA1@TK5EX14MBXC104.redmond.corp.microsoft.com>
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
^ permalink raw reply
* Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: Greg KH @ 2011-02-15 17:22 UTC (permalink / raw)
To: Hank Janssen
Cc: KY Srinivasan, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hashir Abdi, Mike Sterling
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C56233F9714A@TK5EX14MBXC114.redmond.corp.microsoft.com>
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
^ permalink raw reply
* RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: Hank Janssen @ 2011-02-15 16:59 UTC (permalink / raw)
To: KY Srinivasan, Greg KH
Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
virtualization@lists.osdl.org, Haiyang Zhang, Hashir Abdi,
Mike Sterling
In-Reply-To: <FB42D5CCD7B5934EB1827DB5ED9B850E07085DA1@TK5EX14MBXC104.redmond.corp.microsoft.com>
> -----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.
^ permalink raw reply
* RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: KY Srinivasan @ 2011-02-15 16:53 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
virtualization@lists.osdl.org, Haiyang Zhang, Hank Janssen
In-Reply-To: <20110215155954.GA32313@suse.de>
> -----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
^ permalink raw reply
* Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
From: Greg KH @ 2011-02-15 16:29 UTC (permalink / raw)
To: KY Srinivasan
Cc: Jiri Slaby, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <FB42D5CCD7B5934EB1827DB5ED9B850E07085D7F@TK5EX14MBXC104.redmond.corp.microsoft.com>
On Tue, Feb 15, 2011 at 04:22:20PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Tuesday, February 15, 2011 9:03 AM
> > To: KY Srinivasan
> > Cc: Jiri Slaby; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org
> > Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
> >
> > On Tue, Feb 15, 2011 at 01:35:56PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jiri Slaby [mailto:jirislaby@gmail.com]
> > > > Sent: Tuesday, February 15, 2011 4:21 AM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > > Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
> > > >
> > > > On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote:
> > > > > In preperation for getting rid of the osd layer; change
> > > > > the code to use native wait interfaces. As part of this,
> > > > > fixed the buggy implementation in the osd_wait_primitive
> > > > > where the condition was cleared potentially after the
> > > > > condition was signalled.
> > > > ...
> > > > > @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel
> > > > *channel, void *kbuffer,
> > > > >
> > > > > }
> > > > > }
> > > > > - osd_waitevent_wait(msginfo->waitevent);
> > > > > + wait_event_timeout(msginfo->waitevent,
> > > > > + msginfo->wait_condition,
> > > > > + msecs_to_jiffies(1000));
> > > > > + BUG_ON(msginfo->wait_condition == 0);
> > > >
> > > > The added BUG_ONs all over the code look scary. These shouldn't be
> > > > BUG_ONs at all. You should maybe warn and bail out, but not kill the
> > > > whole machine.
> > >
> > > This is Linux code running as a guest on a Windows host; and so the guest
> > cannot
> > > tolerate a failure of the host. In the cases where I have chosen to BUG_ON,
> > there
> > > is no reasonable recovery possible when the host is non-functional (as
> > determined
> > > by a non-responsive host).
> >
> > If you have a non-responsive host, wouldn't that imply that this guest
> > code wouldn't run at all? :)
>
> The fact that on a particular transaction the host has not responded within an expected
> time interval does not necessarily mean that the guest code would not be running. There may be
> issues on the host side that may be either transient or permanent that may cause problems like
> this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host
> and so, guest would get scheduled.
>
> >
> > Having BUG_ON() in drivers is not a good idea either way. Please remove
> > these in future patches.
>
> In situations where there is not a reasonable rollback strategy (for
> instance in one of the cases, we are granting access to the guest
> physical pages to the host) we really have only 2 options:
>
> 1) Wait until the host responds. This wait could potentially be unbounded
> and in fact this was the way the code was to begin with. One of the reviewers
> had suggested that unbounded wait was to be corrected.
> 2) Wait for a specific period and if the host does not respond
> within a reasonable period, kill the guest since there is no recovery
> possible.
Killing the guest is a very serious thing, causing all sorts of possible
problems with it, right?
> I chose option 2, as part of addressing some of the prior review
> comments. If the consensus now is to go back to option 1, I am fine with that;
Unbounded waits aren't ok either, you need some sort of timeout.
But, as this is a bit preferable to dieing, I suggest doing this, and
comment the heck out of it to explain all of this for anyone who reads
it.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
From: KY Srinivasan @ 2011-02-15 16:22 UTC (permalink / raw)
To: Greg KH
Cc: Jiri Slaby, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110215140305.GA10301@suse.de>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Tuesday, February 15, 2011 9:03 AM
> To: KY Srinivasan
> Cc: Jiri Slaby; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
>
> On Tue, Feb 15, 2011 at 01:35:56PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jiri Slaby [mailto:jirislaby@gmail.com]
> > > Sent: Tuesday, February 15, 2011 4:21 AM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
> > >
> > > On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote:
> > > > In preperation for getting rid of the osd layer; change
> > > > the code to use native wait interfaces. As part of this,
> > > > fixed the buggy implementation in the osd_wait_primitive
> > > > where the condition was cleared potentially after the
> > > > condition was signalled.
> > > ...
> > > > @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel
> > > *channel, void *kbuffer,
> > > >
> > > > }
> > > > }
> > > > - osd_waitevent_wait(msginfo->waitevent);
> > > > + wait_event_timeout(msginfo->waitevent,
> > > > + msginfo->wait_condition,
> > > > + msecs_to_jiffies(1000));
> > > > + BUG_ON(msginfo->wait_condition == 0);
> > >
> > > The added BUG_ONs all over the code look scary. These shouldn't be
> > > BUG_ONs at all. You should maybe warn and bail out, but not kill the
> > > whole machine.
> >
> > This is Linux code running as a guest on a Windows host; and so the guest
> cannot
> > tolerate a failure of the host. In the cases where I have chosen to BUG_ON,
> there
> > is no reasonable recovery possible when the host is non-functional (as
> determined
> > by a non-responsive host).
>
> If you have a non-responsive host, wouldn't that imply that this guest
> code wouldn't run at all? :)
The fact that on a particular transaction the host has not responded within an expected
time interval does not necessarily mean that the guest code would not be running. There may be
issues on the host side that may be either transient or permanent that may cause problems like
this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host
and so, guest would get scheduled.
>
> Having BUG_ON() in drivers is not a good idea either way. Please remove
> these in future patches.
In situations where there is not a reasonable rollback strategy (for
instance in one of the cases, we are granting access to the guest
physical pages to the host) we really have only 2 options:
1) Wait until the host responds. This wait could potentially be unbounded
and in fact this was the way the code was to begin with. One of the reviewers
had suggested that unbounded wait was to be corrected.
2) Wait for a specific period and if the host does not respond
within a reasonable period, kill the guest since there is no recovery
possible.
I chose option 2, as part of addressing some of the prior review
comments. If the consensus now is to go back to option 1, I am fine with that;
I will send you a patch to rectify this.
Regards,
K. Y
>
> > > And looking at the code, more appropriate would be completion instead of
> > > wait events.
> > >
> > > And msecs_to_jiffies(1000) == HZ.
> >
> > Agreed. In this first round of cleanup, I chose to keep the primitives
> > as they were in osd.c. Greg, if it is ok with you, I will send you a
> > patch that fixes these issues on top of the patches I have already
> > sent.
>
> Yes, that is fine.
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: Greg KH @ 2011-02-15 15:59 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: linux-kernel, devel, virtualization, Haiyang Zhang, Hank Janssen
In-Reply-To: <1297782937-24082-1-git-send-email-kys@microsoft.com>
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
^ permalink raw reply
* [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
From: K. Y. Srinivasan @ 2011-02-15 15:15 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan, Haiyang Zhang, Hank Janssen
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
^ permalink raw reply related
* Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
From: Greg KH @ 2011-02-15 14:03 UTC (permalink / raw)
To: KY Srinivasan
Cc: Jiri Slaby, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <FB42D5CCD7B5934EB1827DB5ED9B850E07085CF8@TK5EX14MBXC104.redmond.corp.microsoft.com>
On Tue, Feb 15, 2011 at 01:35:56PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Jiri Slaby [mailto:jirislaby@gmail.com]
> > Sent: Tuesday, February 15, 2011 4:21 AM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
> >
> > On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote:
> > > In preperation for getting rid of the osd layer; change
> > > the code to use native wait interfaces. As part of this,
> > > fixed the buggy implementation in the osd_wait_primitive
> > > where the condition was cleared potentially after the
> > > condition was signalled.
> > ...
> > > @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel
> > *channel, void *kbuffer,
> > >
> > > }
> > > }
> > > - osd_waitevent_wait(msginfo->waitevent);
> > > + wait_event_timeout(msginfo->waitevent,
> > > + msginfo->wait_condition,
> > > + msecs_to_jiffies(1000));
> > > + BUG_ON(msginfo->wait_condition == 0);
> >
> > The added BUG_ONs all over the code look scary. These shouldn't be
> > BUG_ONs at all. You should maybe warn and bail out, but not kill the
> > whole machine.
>
> This is Linux code running as a guest on a Windows host; and so the guest cannot
> tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there
> is no reasonable recovery possible when the host is non-functional (as determined
> by a non-responsive host).
If you have a non-responsive host, wouldn't that imply that this guest
code wouldn't run at all? :)
Having BUG_ON() in drivers is not a good idea either way. Please remove
these in future patches.
> > And looking at the code, more appropriate would be completion instead of
> > wait events.
> >
> > And msecs_to_jiffies(1000) == HZ.
>
> Agreed. In this first round of cleanup, I chose to keep the primitives
> as they were in osd.c. Greg, if it is ok with you, I will send you a
> patch that fixes these issues on top of the patches I have already
> sent.
Yes, that is fine.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
From: KY Srinivasan @ 2011-02-15 13:35 UTC (permalink / raw)
To: Jiri Slaby
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <4D5A4565.5030501@gmail.com>
> -----Original Message-----
> From: Jiri Slaby [mailto:jirislaby@gmail.com]
> Sent: Tuesday, February 15, 2011 4:21 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
>
> On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote:
> > In preperation for getting rid of the osd layer; change
> > the code to use native wait interfaces. As part of this,
> > fixed the buggy implementation in the osd_wait_primitive
> > where the condition was cleared potentially after the
> > condition was signalled.
> ...
> > @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
> >
> > }
> > }
> > - osd_waitevent_wait(msginfo->waitevent);
> > + wait_event_timeout(msginfo->waitevent,
> > + msginfo->wait_condition,
> > + msecs_to_jiffies(1000));
> > + BUG_ON(msginfo->wait_condition == 0);
>
> The added BUG_ONs all over the code look scary. These shouldn't be
> BUG_ONs at all. You should maybe warn and bail out, but not kill the
> whole machine.
This is Linux code running as a guest on a Windows host; and so the guest cannot
tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there
is no reasonable recovery possible when the host is non-functional (as determined
by a non-responsive host).
>
> And looking at the code, more appropriate would be completion instead of
> wait events.
>
> And msecs_to_jiffies(1000) == HZ.
Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent.
Regards,
K. Y
>
> > @@ -689,7 +693,8 @@ static void vmbus_ongpadl_torndown(
> > memcpy(&msginfo->response.gpadl_torndown,
> > gpadl_torndown,
> > sizeof(struct
> vmbus_channel_gpadl_torndown));
> > - osd_waitevent_set(msginfo->waitevent);
> > + msginfo->wait_condition = 1;
> > + wake_up(&msginfo->waitevent);
> > break;
> > }
> > }
> > @@ -730,7 +735,8 @@ static void vmbus_onversion_response(
> > memcpy(&msginfo->response.version_response,
> > version_response,
> > sizeof(struct vmbus_channel_version_response));
> > - osd_waitevent_set(msginfo->waitevent);
> > + msginfo->wait_condition = 1;
> > + wake_up(&msginfo->waitevent);
> > }
> > }
> > spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>
> regards,
> --
> js
^ permalink raw reply
* Re: [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
From: Michal Hocko @ 2011-02-15 12:42 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <201102152139.03451.rusty@rustcorp.com.au>
On Tue 15-02-11 21:39:03, Rusty Russell wrote:
> On Tue, 15 Feb 2011 08:55:50 pm Michal Hocko wrote:
> > On Tue 15-02-11 20:41:29, Rusty Russell wrote:
> > > On Tue, 15 Feb 2011 08:05:27 pm Michal Hocko wrote:
> > > > Hi,
> > > > we have started seeing a lot of allocator messages complaining about
> > > > failed allocations from virtnet_poll in soft IRQ. Could you consider the
> > > > following patch, please?
> > >
> > > Do we really want to silence this? Isn't warning about it kind of the
> > > point? Your network is probably sucking if this happens...
> >
> > What can user do about it? Is the low level memory allocator message
> > very much usefull for him? Maybe we can add a printk_once in the fail
> > path with some more useful and virtio specific message.
>
> That's an argument against ever printing any message.
Well, honestly, I do not see much point for this message but it is there
for ages so it maybe it is valueable for somebody...
>
> What we need to know is why does this happen with virtio_net and not other
> cards?
The machine just happened to be short on memory due to a strong memory
pressure.
> If it happens to them too, and they silently fall back, all good.
>
> I want to make sure we're not papering over a real problem...
>
> Thanks,
> Rusty.
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
^ permalink raw reply
* Re: [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
From: Michal Hocko @ 2011-02-15 12:39 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <20110215093527.GB8341@tiehlicka.suse.cz>
On Tue 15-02-11 10:35:27, Michal Hocko wrote:
[...]
> [22798.508903] The following is only an harmless informational message.
> [22798.508909] Unless you get a _continuous_flood_ of these messages it means
> [22798.508911] everything is working fine. Allocations from irqs cannot be
> [22798.508913] perfectly reliable and the kernel is designed to handle that.
I have just realized that the above text is SLES specific so only the
line below with stack trace and memory info is printed. Sorry for confusion
> [22798.508917] loop3: page allocation failure. order:0, mode:0x20, alloc_flags:0x30 pflags:0x80208040
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
^ permalink raw reply
* Re: [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
From: Rusty Russell @ 2011-02-15 11:09 UTC (permalink / raw)
To: Michal Hocko; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <20110215102549.GD8341@tiehlicka.suse.cz>
On Tue, 15 Feb 2011 08:55:50 pm Michal Hocko wrote:
> On Tue 15-02-11 20:41:29, Rusty Russell wrote:
> > On Tue, 15 Feb 2011 08:05:27 pm Michal Hocko wrote:
> > > Hi,
> > > we have started seeing a lot of allocator messages complaining about
> > > failed allocations from virtnet_poll in soft IRQ. Could you consider the
> > > following patch, please?
> >
> > Do we really want to silence this? Isn't warning about it kind of the
> > point? Your network is probably sucking if this happens...
>
> What can user do about it? Is the low level memory allocator message
> very much usefull for him? Maybe we can add a printk_once in the fail
> path with some more useful and virtio specific message.
That's an argument against ever printing any message.
What we need to know is why does this happen with virtio_net and not other
cards? If it happens to them too, and they silently fall back, all good.
I want to make sure we're not papering over a real problem...
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
From: Michal Hocko @ 2011-02-15 10:25 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <201102152041.29179.rusty@rustcorp.com.au>
On Tue 15-02-11 20:41:29, Rusty Russell wrote:
> On Tue, 15 Feb 2011 08:05:27 pm Michal Hocko wrote:
> > Hi,
> > we have started seeing a lot of allocator messages complaining about
> > failed allocations from virtnet_poll in soft IRQ. Could you consider the
> > following patch, please?
>
> Do we really want to silence this? Isn't warning about it kind of the
> point? Your network is probably sucking if this happens...
What can user do about it? Is the low level memory allocator message
very much usefull for him? Maybe we can add a printk_once in the fail
path with some more useful and virtio specific message.
Thanks
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
^ permalink raw reply
* Re: [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
From: Rusty Russell @ 2011-02-15 10:11 UTC (permalink / raw)
To: Michal Hocko; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
In-Reply-To: <20110215093527.GB8341@tiehlicka.suse.cz>
On Tue, 15 Feb 2011 08:05:27 pm Michal Hocko wrote:
> Hi,
> we have started seeing a lot of allocator messages complaining about
> failed allocations from virtnet_poll in soft IRQ. Could you consider the
> following patch, please?
Do we really want to silence this? Isn't warning about it kind of the
point? Your network is probably sucking if this happens...
Cheers,
Rusty.
^ permalink raw reply
* [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
From: Michal Hocko @ 2011-02-15 9:35 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, virtualization, linux-kernel, Michael S. Tsirkin
Hi,
we have started seeing a lot of allocator messages complaining about
failed allocations from virtnet_poll in soft IRQ. Could you consider the
following patch, please?
The patch is based on 2.6.38-rc4.
---
From aabc19f22915dafeac0f1f6aa7cb7e49a8021ba1 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Tue, 15 Feb 2011 10:20:59 +0100
Subject: [PATCH] virtio: use __GFP_NOWARN for try_fill_recv in virtnet_poll
virtnet_poll is called from soft IRQ and it tries to allocate GFP_ATOMIC
memory (through try_fill_recv). This allocation can fail and we are
falling back to schedule_delayed_work in that case.
Let's add __GFP_NOWARN to the allocation flags to get rid of the
allocator complains for failed allocations:
[22798.508903] The following is only an harmless informational message.
[22798.508909] Unless you get a _continuous_flood_ of these messages it means
[22798.508911] everything is working fine. Allocations from irqs cannot be
[22798.508913] perfectly reliable and the kernel is designed to handle that.
[22798.508917] loop3: page allocation failure. order:0, mode:0x20, alloc_flags:0x30 pflags:0x80208040
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
drivers/net/virtio_net.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 90a23e4..aea1e51 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -477,7 +477,7 @@ again:
}
if (vi->num < vi->max / 2) {
- if (!try_fill_recv(vi, GFP_ATOMIC))
+ if (!try_fill_recv(vi, GFP_ATOMIC|__GFP_NOWARN))
schedule_delayed_work(&vi->refill, 0);
}
--
1.7.2.3
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
^ permalink raw reply related
* Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
From: Jiri Slaby @ 2011-02-15 9:20 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, devel, virtualization
In-Reply-To: <1297447183-21807-1-git-send-email-kys@microsoft.com>
On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote:
> In preperation for getting rid of the osd layer; change
> the code to use native wait interfaces. As part of this,
> fixed the buggy implementation in the osd_wait_primitive
> where the condition was cleared potentially after the
> condition was signalled.
...
> @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
>
> }
> }
> - osd_waitevent_wait(msginfo->waitevent);
> + wait_event_timeout(msginfo->waitevent,
> + msginfo->wait_condition,
> + msecs_to_jiffies(1000));
> + BUG_ON(msginfo->wait_condition == 0);
The added BUG_ONs all over the code look scary. These shouldn't be
BUG_ONs at all. You should maybe warn and bail out, but not kill the
whole machine.
And looking at the code, more appropriate would be completion instead of
wait events.
And msecs_to_jiffies(1000) == HZ.
> @@ -689,7 +693,8 @@ static void vmbus_ongpadl_torndown(
> memcpy(&msginfo->response.gpadl_torndown,
> gpadl_torndown,
> sizeof(struct vmbus_channel_gpadl_torndown));
> - osd_waitevent_set(msginfo->waitevent);
> + msginfo->wait_condition = 1;
> + wake_up(&msginfo->waitevent);
> break;
> }
> }
> @@ -730,7 +735,8 @@ static void vmbus_onversion_response(
> memcpy(&msginfo->response.version_response,
> version_response,
> sizeof(struct vmbus_channel_version_response));
> - osd_waitevent_set(msginfo->waitevent);
> + msginfo->wait_condition = 1;
> + wake_up(&msginfo->waitevent);
> }
> }
> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
regards,
--
js
^ permalink raw reply
* Re: [PATCH 1/2] staging: hv: Remove dead code from netvsc.c
From: Jesper Juhl @ 2011-02-15 0:07 UTC (permalink / raw)
To: Haiyang Zhang
Cc: hjanssen, gregkh, linux-kernel, devel, virtualization,
K. Y. Srinivasan
In-Reply-To: <1297723902-18176-1-git-send-email-haiyangz@microsoft.com>
On Mon, 14 Feb 2011, Haiyang Zhang wrote:
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/staging/hv/netvsc.c | 34 ----------------------------------
> 1 files changed, 0 insertions(+), 34 deletions(-)
>
...
Reviewed-by: Jesper Juhl <jj@chaosbits.net>
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
^ permalink raw reply
* Re: [PATCH 2/2] staging: hv: Remove dead code from rndis_filter.c
From: Jesper Juhl @ 2011-02-15 0:04 UTC (permalink / raw)
To: Haiyang Zhang
Cc: hjanssen, gregkh, linux-kernel, devel, virtualization,
K. Y. Srinivasan
In-Reply-To: <1297723902-18176-2-git-send-email-haiyangz@microsoft.com>
On Mon, 14 Feb 2011, Haiyang Zhang wrote:
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/staging/hv/rndis_filter.c | 15 ---------------
> 1 files changed, 0 insertions(+), 15 deletions(-)
>
...
I would probably have said "remove commented out code from rndis_filter.c"
rather than "remove dead code ...", but nevermind :-)
Looks good to me.
Reviewed-by: Jesper Juhl <jj@chaosbits.net>
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
^ permalink raw reply
* [PATCH 2/2] staging: hv: Remove dead code from rndis_filter.c
From: Haiyang Zhang @ 2011-02-14 22:51 UTC (permalink / raw)
To: haiyangz, hjanssen, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
In-Reply-To: <1297723902-18176-1-git-send-email-haiyangz@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/rndis_filter.c | 15 ---------------
1 files changed, 0 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/rndis_filter.c b/drivers/staging/hv/rndis_filter.c
index 9dde936..e7189cd 100644
--- a/drivers/staging/hv/rndis_filter.c
+++ b/drivers/staging/hv/rndis_filter.c
@@ -355,10 +355,6 @@ static void rndis_filter_receive_data(struct rndis_device *dev,
struct rndis_packet *rndis_pkt;
u32 data_offset;
- /* empty ethernet frame ?? */
- /* ASSERT(Packet->PageBuffers[0].Length > */
- /* RNDIS_MESSAGE_SIZE(struct rndis_packet)); */
-
rndis_pkt = &msg->msg.pkt;
/*
@@ -455,8 +451,6 @@ static int rndis_filter_receive(struct hv_device *dev,
case REMOTE_NDIS_INITIALIZE_CMPLT:
case REMOTE_NDIS_QUERY_CMPLT:
case REMOTE_NDIS_SET_CMPLT:
- /* case REMOTE_NDIS_RESET_CMPLT: */
- /* case REMOTE_NDIS_KEEPALIVE_CMPLT: */
/* completion msgs */
rndis_filter_receive_response(rndis_dev, &rndis_msg);
break;
@@ -563,9 +557,6 @@ static int rndis_filter_set_packet_filter(struct rndis_device *dev,
u32 status;
int ret;
- /* ASSERT(RNDIS_MESSAGE_SIZE(struct rndis_set_request) + sizeof(u32) <= */
- /* sizeof(struct rndis_message)); */
-
request = get_rndis_request(dev, REMOTE_NDIS_SET_MSG,
RNDIS_MESSAGE_SIZE(struct rndis_set_request) +
sizeof(u32));
@@ -634,8 +625,6 @@ int rndis_filter_init(struct netvsc_driver *drv)
drv->base.dev_rm;
rndis_filter.inner_drv.base.cleanup = drv->base.cleanup;
- /* ASSERT(Driver->OnSend); */
- /* ASSERT(Driver->OnReceiveCallback); */
rndis_filter.inner_drv.send = drv->send;
rndis_filter.inner_drv.recv_cb = drv->recv_cb;
rndis_filter.inner_drv.link_status_change =
@@ -646,7 +635,6 @@ int rndis_filter_init(struct netvsc_driver *drv)
drv->base.dev_rm = rndis_filter_device_remove;
drv->base.cleanup = rndis_filter_cleanup;
drv->send = rndis_filter_send;
- /* Driver->QueryLinkStatus = RndisFilterQueryDeviceLinkStatus; */
drv->recv_cb = rndis_filter_receive;
return 0;
@@ -793,8 +781,6 @@ static int rndis_filte_device_add(struct hv_device *dev,
/* Initialize the rndis device */
netDevice = dev->ext;
- /* ASSERT(netDevice); */
- /* ASSERT(netDevice->Device); */
netDevice->extension = rndisDevice;
rndisDevice->net_dev = netDevice;
@@ -882,7 +868,6 @@ static int rndis_filter_send(struct hv_device *dev,
/* Add the rndis header */
filterPacket = (struct rndis_filter_packet *)pkt->extension;
- /* ASSERT(filterPacket); */
memset(filterPacket, 0, sizeof(struct rndis_filter_packet));
--
1.6.3.2
^ permalink raw reply related
* [PATCH 1/2] staging: hv: Remove dead code from netvsc.c
From: Haiyang Zhang @ 2011-02-14 22:51 UTC (permalink / raw)
To: haiyangz, hjanssen, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/netvsc.c | 34 ----------------------------------
1 files changed, 0 insertions(+), 34 deletions(-)
diff --git a/drivers/staging/hv/netvsc.c b/drivers/staging/hv/netvsc.c
index fa46a7e..8c6d4ae 100644
--- a/drivers/staging/hv/netvsc.c
+++ b/drivers/staging/hv/netvsc.c
@@ -134,7 +134,6 @@ static void put_net_device(struct hv_device *device)
struct netvsc_device *net_device;
net_device = device->ext;
- /* ASSERT(netDevice); */
atomic_dec(&net_device->refcnt);
}
@@ -186,9 +185,6 @@ int netvsc_initialize(struct hv_driver *drv)
sizeof(struct nvsp_message),
sizeof(struct vmtransfer_page_packet_header));
- /* Make sure we are at least 2 pages since 1 page is used for control */
- /* ASSERT(driver->RingBufferSize >= (PAGE_SIZE << 1)); */
-
drv->name = driver_name;
memcpy(&drv->dev_type, &netvsc_device_type, sizeof(struct hv_guid));
@@ -220,9 +216,6 @@ static int netvsc_init_recv_buf(struct hv_device *device)
"device being destroyed?");
return -1;
}
- /* ASSERT(netDevice->ReceiveBufferSize > 0); */
- /* page-size grandularity */
- /* ASSERT((netDevice->ReceiveBufferSize & (PAGE_SIZE - 1)) == 0); */
net_device->recv_buf =
(void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
@@ -234,9 +227,6 @@ static int netvsc_init_recv_buf(struct hv_device *device)
ret = -1;
goto cleanup;
}
- /* page-aligned buffer */
- /* ASSERT(((unsigned long)netDevice->ReceiveBuffer & (PAGE_SIZE - 1)) == */
- /* 0); */
DPRINT_INFO(NETVSC, "Establishing receive buffer's GPADL...");
@@ -299,8 +289,6 @@ static int netvsc_init_recv_buf(struct hv_device *device)
}
/* Parse the response */
- /* ASSERT(netDevice->ReceiveSectionCount == 0); */
- /* ASSERT(netDevice->ReceiveSections == NULL); */
net_device->recv_section_cnt = init_packet->msg.
v1_msg.send_recv_buf_complete.num_sections;
@@ -363,9 +351,6 @@ static int netvsc_init_send_buf(struct hv_device *device)
goto cleanup;
}
- /* page-size grandularity */
- /* ASSERT((netDevice->SendBufferSize & (PAGE_SIZE - 1)) == 0); */
-
net_device->send_buf =
(void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
get_order(net_device->send_buf_size));
@@ -375,8 +360,6 @@ static int netvsc_init_send_buf(struct hv_device *device)
ret = -1;
goto cleanup;
}
- /* page-aligned buffer */
- /* ASSERT(((unsigned long)netDevice->SendBuffer & (PAGE_SIZE - 1)) == 0); */
DPRINT_INFO(NETVSC, "Establishing send buffer's GPADL...");
@@ -639,8 +622,6 @@ static int netvsc_connect_vsp(struct hv_device *device)
goto cleanup;
}
- /* Now, check the response */
- /* ASSERT(initPacket->Messages.InitMessages.InitComplete.MaximumMdlChainLength <= MAX_MULTIPAGE_BUFFER_COUNT); */
DPRINT_INFO(NETVSC, "NvspMessageTypeInit status(%d) max mdl chain (%d)",
init_packet->msg.init_msg.init_complete.status,
init_packet->msg.init_msg.
@@ -895,7 +876,6 @@ static void netvsc_send_completion(struct hv_device *device,
/* Get the send context */
nvsc_packet = (struct hv_netvsc_packet *)(unsigned long)
packet->trans_id;
- /* ASSERT(nvscPacket); */
/* Notify the layer above us */
nvsc_packet->completion.send.send_completion(
@@ -1072,8 +1052,6 @@ static void netvsc_receive(struct hv_device *device,
/* This is how much we can satisfy */
xferpage_packet->count = count - 1;
- /* ASSERT(xferpagePacket->Count > 0 && xferpagePacket->Count <= */
- /* vmxferpagePacket->RangeCount); */
if (xferpage_packet->count != vmxferpage_packet->range_cnt) {
DPRINT_INFO(NETVSC, "Needed %d netvsc pkts to satisy this xfer "
@@ -1101,10 +1079,6 @@ static void netvsc_receive(struct hv_device *device,
vmxferpage_packet->ranges[i].byte_count;
netvsc_packet->page_buf_cnt = 1;
- /* ASSERT(vmxferpagePacket->Ranges[i].ByteOffset + */
- /* vmxferpagePacket->Ranges[i].ByteCount < */
- /* netDevice->ReceiveBufferSize); */
-
netvsc_packet->page_buf[0].len =
vmxferpage_packet->ranges[i].byte_count;
@@ -1147,7 +1121,6 @@ static void netvsc_receive(struct hv_device *device,
if (bytes_remain == 0)
break;
}
- /* ASSERT(bytesRemain == 0); */
}
DPRINT_DBG(NETVSC, "[%d] - (abs offset %u len %u) => "
"(pfn %llx, offset %u, len %u)", i,
@@ -1165,8 +1138,6 @@ static void netvsc_receive(struct hv_device *device,
completion.recv.recv_completion_ctx);
}
- /* ASSERT(list_empty(&listHead)); */
-
put_net_device(device);
}
@@ -1225,8 +1196,6 @@ static void netvsc_receive_completion(void *context)
bool fsend_receive_comp = false;
unsigned long flags;
- /* ASSERT(packet->XferPagePacket); */
-
/*
* Even though it seems logical to do a GetOutboundNetDevice() here to
* send out receive completion, we are using GetInboundNetDevice()
@@ -1242,7 +1211,6 @@ static void netvsc_receive_completion(void *context)
/* Overloading use of the lock. */
spin_lock_irqsave(&net_device->recv_pkt_list_lock, flags);
- /* ASSERT(packet->XferPagePacket->Count > 0); */
packet->xfer_page_pkt->count--;
/*
@@ -1280,8 +1248,6 @@ static void netvsc_channel_cb(void *context)
unsigned char *buffer;
int bufferlen = NETVSC_PACKET_SIZE;
- /* ASSERT(device); */
-
packet = kzalloc(NETVSC_PACKET_SIZE * sizeof(unsigned char),
GFP_ATOMIC);
if (!packet)
--
1.6.3.2
^ permalink raw reply related
* [PATCH] Staging: hv: Use list_entry for msg_info and remove associated comment
From: Hank Janssen @ 2011-02-14 18:35 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
Cc: Abhishek Kane
Change code to use list_entry api. And removed comment
suggesting that change.
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: K.Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index a9c9d49..6c497e9 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -580,8 +580,9 @@ static void vmbus_onopen_result(struct vmbus_channel_message_header *hdr)
spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
list_for_each(curr, &vmbus_connection.chn_msg_list) {
-/* FIXME: this should probably use list_entry() instead */
- msginfo = (struct vmbus_channel_msginfo *)curr;
+ msginfo = list_entry(curr, struct vmbus_channel_msginfo,
+ msglistentry);
+
requestheader =
(struct vmbus_channel_message_header *)msginfo->msg;
@@ -628,8 +629,8 @@ static void vmbus_ongpadl_created(struct vmbus_channel_message_header *hdr)
spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
list_for_each(curr, &vmbus_connection.chn_msg_list) {
-/* FIXME: this should probably use list_entry() instead */
- msginfo = (struct vmbus_channel_msginfo *)curr;
+ msginfo = list_entry(curr, struct vmbus_channel_msginfo,
+ msglistentry);
requestheader =
(struct vmbus_channel_message_header *)msginfo->msg;
@@ -676,8 +677,8 @@ static void vmbus_ongpadl_torndown(
spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
list_for_each(curr, &vmbus_connection.chn_msg_list) {
-/* FIXME: this should probably use list_entry() instead */
- msginfo = (struct vmbus_channel_msginfo *)curr;
+ msginfo = list_entry(curr, struct vmbus_channel_msginfo,
+ msglistentry);
requestheader =
(struct vmbus_channel_message_header *)msginfo->msg;
@@ -718,8 +719,8 @@ static void vmbus_onversion_response(
spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
list_for_each(curr, &vmbus_connection.chn_msg_list) {
-/* FIXME: this should probably use list_entry() instead */
- msginfo = (struct vmbus_channel_msginfo *)curr;
+ msginfo = list_entry(curr, struct vmbus_channel_msginfo,
+ msglistentry);
requestheader =
(struct vmbus_channel_message_header *)msginfo->msg;
--
1.6.0.2
^ permalink raw reply related
* Re: [PATCH] Staging: hv: Use list_entry for msg_info and remove associated comment
From: Greg KH @ 2011-02-14 18:33 UTC (permalink / raw)
To: Hank Janssen
Cc: haiyangz, linux-kernel, devel, virtualization, Abhishek Kane,
K.Y. Srinivasan
In-Reply-To: <1297708537-15981-1-git-send-email-hjanssen@microsoft.com>
On Mon, Feb 14, 2011 at 10:35:37AM -0800, Hank Janssen wrote:
> Change code to use list_entry api. And removed comment
> suggesting that change.
>
> Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> Signed-off-by: K.Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
>
> ---
> drivers/staging/hv/channel_mgmt.c | 17 +++++++++--------
> 1 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
> index a9c9d49..6c497e9 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -580,8 +580,9 @@ static void vmbus_onopen_result(struct vmbus_channel_message_header *hdr)
> spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
>
> list_for_each(curr, &vmbus_connection.chn_msg_list) {
> -/* FIXME: this should probably use list_entry() instead */
> - msginfo = (struct vmbus_channel_msginfo *)curr;
> + msginfo = list_entry(curr, struct vmbus_channel_msginfo,
> + msglistentry);
How about using list_for_each_entry() instead? That's the "preferred"
way to do things these days, and it saves you an extra line of code :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH]: Staging: hv: Cleanup vmalloc calls
From: Greg KH @ 2011-02-11 22:53 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: linux-kernel, devel, virtualization, Hank Janssen
In-Reply-To: <1297464525-23254-1-git-send-email-kys@microsoft.com>
On Fri, Feb 11, 2011 at 02:48:45PM -0800, K. Y. Srinivasan wrote:
> The subject says it all. There is no need to
> specify different page protection bits based
> on the architecture.
Wonderful, thanks for doing this. I'll queue it up after your previous
patches sometime next week.
thanks,
greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox