* [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order
From: Haiyang Zhang @ 2011-02-23 20:19 UTC (permalink / raw)
To: haiyangz, hjanssen, kys, v-abkane, gregkh, linux-kernel, devel,
vir
The patch fixed the code depending on the exact order of fields in the
struct vmbus_driver_context, so the unused field drv_ctx can be removed,
and drv_obj doesn't have to be the second field in this structure.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/blkvsc_drv.c | 2 ++
drivers/staging/hv/netvsc_drv.c | 2 ++
drivers/staging/hv/storvsc_drv.c | 2 ++
drivers/staging/hv/vmbus.h | 2 +-
drivers/staging/hv/vmbus_drv.c | 14 +-------------
5 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
index 36a0adb..293ab8e 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -177,6 +177,8 @@ static int blkvsc_drv_init(int (*drv_init)(struct hv_driver *drv))
struct driver_context *drv_ctx = &g_blkvsc_drv.drv_ctx;
int ret;
+ drv_ctx->hv_drv = &storvsc_drv_obj->base;
+
storvsc_drv_obj->ring_buffer_size = blkvsc_ringbuffer_size;
/* Callback to client driver to complete the initialization */
diff --git a/drivers/staging/hv/netvsc_drv.c b/drivers/staging/hv/netvsc_drv.c
index 03f9740..364b6c7 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -500,6 +500,8 @@ static int netvsc_drv_init(int (*drv_init)(struct hv_driver *drv))
struct driver_context *drv_ctx = &g_netvsc_drv.drv_ctx;
int ret;
+ drv_ctx->hv_drv = &net_drv_obj->base;
+
net_drv_obj->ring_buf_size = ring_size * PAGE_SIZE;
net_drv_obj->recv_cb = netvsc_recv_callback;
net_drv_obj->link_status_change = netvsc_linkstatus_callback;
diff --git a/drivers/staging/hv/storvsc_drv.c b/drivers/staging/hv/storvsc_drv.c
index a8427ff..33acee5 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -140,6 +140,8 @@ static int storvsc_drv_init(int (*drv_init)(struct hv_driver *drv))
struct storvsc_driver_object *storvsc_drv_obj = &g_storvsc_drv.drv_obj;
struct driver_context *drv_ctx = &g_storvsc_drv.drv_ctx;
+ drv_ctx->hv_drv = &storvsc_drv_obj->base;
+
storvsc_drv_obj->ring_buffer_size = storvsc_ringbuffer_size;
/* Callback to client driver to complete the initialization */
diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
index 42f2adb..fd9d00f 100644
--- a/drivers/staging/hv/vmbus.h
+++ b/drivers/staging/hv/vmbus.h
@@ -30,8 +30,8 @@
struct driver_context {
struct hv_guid class_id;
-
struct device_driver driver;
+ struct hv_driver *hv_drv;
/*
* Use these methods instead of the struct device_driver so 2.6 kernel
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..1f4e46d 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -42,11 +42,6 @@
/* Main vmbus driver data structure */
struct vmbus_driver_context {
- /* !! These must be the first 2 fields !! */
- /* FIXME, this is a bug */
- /* The driver field is not used in here. Instead, the bus field is */
- /* used to represent the driver */
- struct driver_context drv_ctx;
struct hv_driver drv_obj;
struct bus_type bus;
@@ -869,14 +864,7 @@ static int vmbus_match(struct device *device, struct device_driver *driver)
/* We found our driver ? */
if (memcmp(&device_ctx->class_id, &driver_ctx->class_id,
sizeof(struct hv_guid)) == 0) {
- /*
- * !! NOTE: The driver_ctx is not a vmbus_drv_ctx. We typecast
- * it here to access the struct hv_driver field
- */
- struct vmbus_driver_context *vmbus_drv_ctx =
- (struct vmbus_driver_context *)driver_ctx;
-
- device_ctx->device_obj.drv = &vmbus_drv_ctx->drv_obj;
+ device_ctx->device_obj.drv = driver_ctx->hv_drv;
DPRINT_INFO(VMBUS_DRV,
"device object (%p) set to driver object (%p)",
&device_ctx->device_obj,
--
1.6.3.2
^ permalink raw reply related
* RE: [PATCH 2/6] Staging: hv: hv.c Removed all DPRINT and debug - using pr_err now
From: Hank Janssen @ 2011-02-23 19:41 UTC (permalink / raw)
To: Greg KH
Cc: Haiyang Zhang, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
KY Srinivasan
In-Reply-To: <20110223191533.GB1030@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, February 23, 2011 11:16 AM
>
> On Tue, Feb 22, 2011 at 03:32:41PM -0800, Hank Janssen wrote:
> > This group of patches removes all DPRINT from hv_vmbus.ko.
> > It is divided in several patches due to size.
> >
> > All DPRINT calls have been removed, and where needed have been
> > replaced with pr_XX native calls. Many debug DPRINT calls have
> > been removed outright.
> >
> > The amount of clutter this driver prints has been
> > significantly reduced.
> >
> > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> >
> > ---
> > drivers/staging/hv/hv.c | 88 +++++++++++--------------------------
> ---------
> > 1 files changed, 21 insertions(+), 67 deletions(-)
> > - DPRINT_INFO(VMBUS, "OS Build:%d-%d.%d-%d-%d.%d",\
> > - eax,
> > - ebx >> 16,
> > - ebx & 0xFFFF,
> > - ecx,
> > - edx >> 24,
> > - edx & 0xFFFFFF);
> > +
> > + pr_info("%s: Hyper-V Host OS Build:%d-%d.%d-%d-%d.%d",
> > + VMBUS_MOD,
> > + eax,
> > + ebx >> 16,
> > + ebx & 0xFFFF,
> > + ecx,
> > + edx >> 24,
> > + edx & 0xFFFFFF);
>
> Why did you keep this one? Why is it needed?
This tells me what version the host is running. I frequently ask customers if
they are running on Host version X or Y. This tells me that with certainty
what they are running. The number of times I got from a customer that
they are running X while I knew that would not be possible has been pretty
high.
>
> > if (!query_hypervisor_presence()) {
> > - DPRINT_ERR(VMBUS, "No Windows hypervisor detected!!");
> > + pr_err("%s: %s No Hyper-V detected", VMBUS_MOD, __func__);
>
> Why the __func__? That should never be needed as it is trivial to see
> what is happening, the user doesn't need to know the function name,
> right?
>
> Please remove them from all of these calls.
The new patch will have these removed. When I checked other drivers
in the drivers subdirectory the __func__ is used 15455 times most
of these are in print, debug or error lines. The __func__ in this
case only shows up if an error occurs.
But as requested, I will remove them.
>
> Oh, and you obviously didn't test these patches as your syslog would be
> a mess if you did. Which is NOT ok, and makes me grumpy:
> http://www.kroah.com/log/linux/maintainer-05.html
>
> bah, I should just make a numbered list and just start saying: "This
> patch fails point #4" or something like that, it would save me in
> typing...
>
They where compile and run tested. And syslog was not a mess. What did
I mess up here? The amount of printouts now are a fraction of what they
where before.
Hank.
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: KY Srinivasan @ 2011-02-23 19:22 UTC (permalink / raw)
To: Greg KH, Thomas Gleixner
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org, devel@linuxdriverproject.org
In-Reply-To: <20110223191617.GC1030@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, February 23, 2011 2:16 PM
> To: Thomas Gleixner
> Cc: KY Srinivasan; gregkh@suse.de; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org; devel@linuxdriverproject.org
> Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
>
> On Mon, Feb 21, 2011 at 03:51:56PM +0100, Thomas Gleixner wrote:
> > On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > > > There are various ways to solve that proper.
> > > > > >
> > > > > > - You can provide the interrupt number from ACPI/PCI or whatever
> your HV
> > > > > > provides as enumeration.
> > > > > >
> > > > > > - Use a fixed vector like XEN does for the event channel
> > > > > >
> > > > > > - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > > > MSI(-X)
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > tglx
> > > > >
> > > > > I am not claiming that what I have done here is the best possible solution.
> > > > > However, I will submit to you that it is better than what we had here
> > > > > prior to this patch. I will address this and a whole lot of other issues
> > > > > in future patches.
> > > >
> > > > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > > > against breaking random other drivers. Care to explain why you think
> > > > that's better ?
> > >
> > > The root device for the VM is bound to the PV driver on some distributions.
> > > So, if we cannot load the PV drivers, we do not have a system that boots.
> > > In general, the system performance without these PV drivers is so poor that
> > > for all practical purposes, having the PV drivers is almost a requirement
> > > for having a useable system. While the platform supports configuration of the
> VM with
> > > some emulated devices, it is not a recommended configuration (because of
> > > performance reasons) for Linux virtual machines on the Hyper-V platform.
> >
> > It does not matter whether it's recommended or not. If it results in a
> > non usable emulated device it's broken. Just imagine you grab the
> > serial interrupt, which results in not having a debug console. Not at
> > all acceptable.
> >
> > > I have spent significantly more time debating this patch than
> > > developing this patch that I still think improves the current
> > > driver. I will leave it to Greg and other powers that be to decide
> > > if this patch will be accepted. Let me know what your verdict is.
> >
> > As I said, any other clean solution (and I pointed out 3) over this
> > hack.
>
> I agree, KY, please fix this up properly, I've dropped this patch from
> my queue now.
Ok; will do.
K. Y
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: Greg KH @ 2011-02-23 19:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: KY Srinivasan, gregkh@suse.de, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org, devel@linuxdriverproject.org
In-Reply-To: <alpine.LFD.2.00.1102211549340.2701@localhost6.localdomain6>
On Mon, Feb 21, 2011 at 03:51:56PM +0100, Thomas Gleixner wrote:
> On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > > There are various ways to solve that proper.
> > > > >
> > > > > - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > > > provides as enumeration.
> > > > >
> > > > > - Use a fixed vector like XEN does for the event channel
> > > > >
> > > > > - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > > MSI(-X)
> > > > >
> > > > > Thanks,
> > > > >
> > > > > tglx
> > > >
> > > > I am not claiming that what I have done here is the best possible solution.
> > > > However, I will submit to you that it is better than what we had here
> > > > prior to this patch. I will address this and a whole lot of other issues
> > > > in future patches.
> > >
> > > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > > against breaking random other drivers. Care to explain why you think
> > > that's better ?
> >
> > The root device for the VM is bound to the PV driver on some distributions.
> > So, if we cannot load the PV drivers, we do not have a system that boots.
> > In general, the system performance without these PV drivers is so poor that
> > for all practical purposes, having the PV drivers is almost a requirement
> > for having a useable system. While the platform supports configuration of the VM with
> > some emulated devices, it is not a recommended configuration (because of
> > performance reasons) for Linux virtual machines on the Hyper-V platform.
>
> It does not matter whether it's recommended or not. If it results in a
> non usable emulated device it's broken. Just imagine you grab the
> serial interrupt, which results in not having a debug console. Not at
> all acceptable.
>
> > I have spent significantly more time debating this patch than
> > developing this patch that I still think improves the current
> > driver. I will leave it to Greg and other powers that be to decide
> > if this patch will be accepted. Let me know what your verdict is.
>
> As I said, any other clean solution (and I pointed out 3) over this
> hack.
I agree, KY, please fix this up properly, I've dropped this patch from
my queue now.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/6] Staging: hv: hv.c Removed all DPRINT and debug - using pr_err now
From: Greg KH @ 2011-02-23 19:15 UTC (permalink / raw)
To: Hank Janssen
Cc: haiyangz, gregkh, linux-kernel, devel, virtualization,
K. Y. Srinivasan
In-Reply-To: <1298417565-12356-2-git-send-email-hjanssen@microsoft.com>
On Tue, Feb 22, 2011 at 03:32:41PM -0800, Hank Janssen wrote:
> This group of patches removes all DPRINT from hv_vmbus.ko.
> It is divided in several patches due to size.
>
> All DPRINT calls have been removed, and where needed have been
> replaced with pr_XX native calls. Many debug DPRINT calls have
> been removed outright.
>
> The amount of clutter this driver prints has been
> significantly reduced.
>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
>
> ---
> drivers/staging/hv/hv.c | 88 +++++++++++-----------------------------------
> 1 files changed, 21 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
> index 2d492ad..e3ce26d 100644
> --- a/drivers/staging/hv/hv.c
> +++ b/drivers/staging/hv/hv.c
> @@ -80,20 +80,6 @@ static int query_hypervisor_info(void)
> op = HVCPUID_VENDOR_MAXFUNCTION;
> cpuid(op, &eax, &ebx, &ecx, &edx);
>
> - DPRINT_INFO(VMBUS, "Vendor ID: %c%c%c%c%c%c%c%c%c%c%c%c",
> - (ebx & 0xFF),
> - ((ebx >> 8) & 0xFF),
> - ((ebx >> 16) & 0xFF),
> - ((ebx >> 24) & 0xFF),
> - (ecx & 0xFF),
> - ((ecx >> 8) & 0xFF),
> - ((ecx >> 16) & 0xFF),
> - ((ecx >> 24) & 0xFF),
> - (edx & 0xFF),
> - ((edx >> 8) & 0xFF),
> - ((edx >> 16) & 0xFF),
> - ((edx >> 24) & 0xFF));
> -
> max_leaf = eax;
> eax = 0;
> ebx = 0;
> @@ -102,12 +88,6 @@ static int query_hypervisor_info(void)
> op = HVCPUID_INTERFACE;
> cpuid(op, &eax, &ebx, &ecx, &edx);
>
> - DPRINT_INFO(VMBUS, "Interface ID: %c%c%c%c",
> - (eax & 0xFF),
> - ((eax >> 8) & 0xFF),
> - ((eax >> 16) & 0xFF),
> - ((eax >> 24) & 0xFF));
> -
> if (max_leaf >= HVCPUID_VERSION) {
> eax = 0;
> ebx = 0;
> @@ -115,14 +95,17 @@ static int query_hypervisor_info(void)
> edx = 0;
> op = HVCPUID_VERSION;
> cpuid(op, &eax, &ebx, &ecx, &edx);
> - DPRINT_INFO(VMBUS, "OS Build:%d-%d.%d-%d-%d.%d",\
> - eax,
> - ebx >> 16,
> - ebx & 0xFFFF,
> - ecx,
> - edx >> 24,
> - edx & 0xFFFFFF);
> +
> + pr_info("%s: Hyper-V Host OS Build:%d-%d.%d-%d-%d.%d",
> + VMBUS_MOD,
> + eax,
> + ebx >> 16,
> + ebx & 0xFFFF,
> + ecx,
> + edx >> 24,
> + edx & 0xFFFFFF);
Why did you keep this one? Why is it needed?
> if (!query_hypervisor_presence()) {
> - DPRINT_ERR(VMBUS, "No Windows hypervisor detected!!");
> + pr_err("%s: %s No Hyper-V detected", VMBUS_MOD, __func__);
Why the __func__? That should never be needed as it is trivial to see
what is happening, the user doesn't need to know the function name,
right?
Please remove them from all of these calls.
Oh, and you obviously didn't test these patches as your syslog would be
a mess if you did. Which is NOT ok, and makes me grumpy:
http://www.kroah.com/log/linux/maintainer-05.html
bah, I should just make a numbered list and just start saying: "This
patch fails point #4" or something like that, it would save me in
typing...
Please redo this entire series...
greg k-h
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Greg KH @ 2011-02-23 19:11 UTC (permalink / raw)
To: Hank Janssen; +Cc: gregkh, linux-kernel, virtualization, devel
In-Reply-To: <1298417565-12356-1-git-send-email-hjanssen@microsoft.com>
On Tue, Feb 22, 2011 at 03:32:40PM -0800, Hank Janssen wrote:
> This group of patches removes all DPRINT from hv_vmbus.ko.
> It is divided in several patches due to size.
Why say this in the 1/6 patch? It should be in the 0/6 introduction.
>
> All DPRINT calls have been removed, and where needed have been
> replaced with pr_XX native calls. Many debug DPRINT calls have
> been removed outright.
I think a lot of these pr_XX calls can be switched to dev_XX calls
instead, right?
How about you break this up into a different series of patches to make
it more readable:
- remove unneeded DPRINT calls
- convert remaining DPRINT calls to pr_XX or dev_XX as needed
That would make it easier to review, as it is, it's quite difficult.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Hank Janssen @ 2011-02-23 17:53 UTC (permalink / raw)
To: Joe Perches
Cc: devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <1298480998.32283.8.camel@Joe-Laptop>
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Wednesday, February 23, 2011 9:10 AM
> To: Hank Janssen
> On Wed, 2011-02-23 at 16:58 +0000, Hank Janssen wrote:
> > > #define pr_fmt(fmt) "%s: " fmt, VMBUS_MOD
> > > or
> > > #define pr_fmt(fmt) "%s:%s " fmt, VMBUS_MOD, __func__
> > > (if you must)
> > I wrestled with that when I did the conversion, The reason I did not
> > Do that is when I check other drivers very few do it that way, most
> do
> > It the way I do it or actually hard code the module name to be
> printed.
>
> My preference would have been to use the KBUILD_MODNAME that
> almost every other driver/module uses.
>
> > So what is the general consensus. The current way I implemented it
> seems
> > to be how other drivers use it to.
>
> Your code, your choices.
Joe,
Hey I am easy! I am just not cheap! :)
Based on your feedback and Dan Carpenter's feedback I will implement
It the way you suggested it.
I should have the patch out later today.
Hank.
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Dan Carpenter @ 2011-02-23 17:10 UTC (permalink / raw)
To: Hank Janssen
Cc: Joe Perches, devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C56233FAC60E@TK5EX14MBXC118.redmond.corp.microsoft.com>
On Wed, Feb 23, 2011 at 04:58:10PM +0000, Hank Janssen wrote:
> > Also, ff all the pr_<level>'s are using VMBUS_MOD,
> > then perhaps it would look better to add
> >
> > #define pr_fmt(fmt) "%s: " fmt, VMBUS_MOD
> > or
> > #define pr_fmt(fmt) "%s:%s " fmt, VMBUS_MOD, __func__
> > (if you must)
>
> I wrestled with that when I did the conversion, The reason I did not
> Do that is when I check other drivers very few do it that way, most do
> It the way I do it or actually hard code the module name to be printed.
> And since the original objection was that DPRINT seems to implement it's
> own logging I did not want to re-implement the pr_XXX calls with this layer
> of indirection which almost looks like I am going down the path of DPRINT
> again.
>
> So what is the general consensus. The current way I implemented it seems
> to be how other drivers use it to.
The way Joe suggested is best.
regards,
dan carpenter
^ permalink raw reply
* RE: [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Joe Perches @ 2011-02-23 17:09 UTC (permalink / raw)
To: Hank Janssen
Cc: devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C56233FAC60E@TK5EX14MBXC118.redmond.corp.microsoft.com>
On Wed, 2011-02-23 at 16:58 +0000, Hank Janssen wrote:
> > #define pr_fmt(fmt) "%s: " fmt, VMBUS_MOD
> > or
> > #define pr_fmt(fmt) "%s:%s " fmt, VMBUS_MOD, __func__
> > (if you must)
> I wrestled with that when I did the conversion, The reason I did not
> Do that is when I check other drivers very few do it that way, most do
> It the way I do it or actually hard code the module name to be printed.
My preference would have been to use the KBUILD_MODNAME that
almost every other driver/module uses.
> So what is the general consensus. The current way I implemented it seems
> to be how other drivers use it to.
Your code, your choices.
cheers, Joe
^ permalink raw reply
* RE: [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Hank Janssen @ 2011-02-23 16:58 UTC (permalink / raw)
To: Joe Perches
Cc: devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <1298436685.26511.381.camel@Joe-Laptop>
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Tuesday, February 22, 2011 8:51 PM
> On Tue, 2011-02-22 at 15:32 -0800, Hank Janssen wrote:
> > This group of patches removes all DPRINT from hv_vmbus.ko.
> > It is divided in several patches due to size.
> []
> > - DPRINT_ERR(VMBUS_DRV,
> > - "ERROR - Unable to register vmbus root device");
> > + pr_err("%s: %s ERROR Unable to register vmbus root device",
> > + VMBUS_MOD, __func__);
>
> All of the pr_<level> calls should probably have a terminating "\n"
I will correct them and resubmit.
>
> Also, ff all the pr_<level>'s are using VMBUS_MOD,
> then perhaps it would look better to add
>
> #define pr_fmt(fmt) "%s: " fmt, VMBUS_MOD
> or
> #define pr_fmt(fmt) "%s:%s " fmt, VMBUS_MOD, __func__
> (if you must)
I wrestled with that when I did the conversion, The reason I did not
Do that is when I check other drivers very few do it that way, most do
It the way I do it or actually hard code the module name to be printed.
And since the original objection was that DPRINT seems to implement it's
own logging I did not want to re-implement the pr_XXX calls with this layer
of indirection which almost looks like I am going down the path of DPRINT
again.
So what is the general consensus. The current way I implemented it seems
to be how other drivers use it to.
Hank.
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Joe Perches @ 2011-02-23 4:51 UTC (permalink / raw)
To: Hank Janssen; +Cc: devel, gregkh, linux-kernel, virtualization
In-Reply-To: <1298417565-12356-1-git-send-email-hjanssen@microsoft.com>
On Tue, 2011-02-22 at 15:32 -0800, Hank Janssen wrote:
> This group of patches removes all DPRINT from hv_vmbus.ko.
> It is divided in several patches due to size.
[]
> - DPRINT_ERR(VMBUS_DRV,
> - "ERROR - Unable to register vmbus root device");
> + pr_err("%s: %s ERROR Unable to register vmbus root device",
> + VMBUS_MOD, __func__);
All of the pr_<level> calls should probably have a terminating "\n"
Also, ff all the pr_<level>'s are using VMBUS_MOD,
then perhaps it would look better to add
#define pr_fmt(fmt) "%s: " fmt, VMBUS_MOD
or
#define pr_fmt(fmt) "%s:%s " fmt, VMBUS_MOD, __func__
(if you must)
and then use:
pr_err("ERROR Unable to register vmbus root device\n");
^ permalink raw reply
* [PATCH 6/6] Staging: hv: connection.c Removed DPRINT replaced with pr_XX
From: Hank Janssen @ 2011-02-22 23:32 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
In-Reply-To: <1298417565-12356-5-git-send-email-hjanssen@microsoft.com>
This group of patches removes all DPRINT from hv_vmbus.ko.
It is divided in several patches due to size.
All DPRINT calls have been removed, and where needed have been
replaced with pr_XX native calls. Many debug DPRINT calls have
been removed outright.
The amount of clutter this driver prints has been
significantly reduced.
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/connection.c | 27 ++++++++++++---------------
1 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/connection.c b/drivers/staging/hv/connection.c
index f7df479..2e9c0b7 100644
--- a/drivers/staging/hv/connection.c
+++ b/drivers/staging/hv/connection.c
@@ -121,11 +121,6 @@ int vmbus_connect(void)
spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
- DPRINT_DBG(VMBUS, "Vmbus connection - interrupt pfn %llx, "
- "monitor1 pfn %llx,, monitor2 pfn %llx",
- msg->interrupt_page, msg->monitor_page1, msg->monitor_page2);
-
- DPRINT_DBG(VMBUS, "Sending channel initiate msg...");
ret = vmbus_post_msg(msg,
sizeof(struct vmbus_channel_initiate_contact));
if (ret != 0) {
@@ -156,13 +151,12 @@ int vmbus_connect(void)
/* Check if successful */
if (msginfo->response.version_response.version_supported) {
- DPRINT_INFO(VMBUS, "Vmbus connected!!");
+ pr_info("%s: Connected to Hyper-V.", VMBUS_MOD);
vmbus_connection.conn_state = CONNECTED;
-
} else {
- DPRINT_ERR(VMBUS, "Vmbus connection failed!!..."
- "current version (%d) not supported",
- VMBUS_REVISION_NUMBER);
+ pr_err("%s: %s Unable to connect, "
+ "Version %d not supported by Hyper-V ",
+ VMBUS_MOD, __func__, VMBUS_REVISION_NUMBER);
ret = -1;
goto Cleanup;
}
@@ -225,7 +219,7 @@ int vmbus_disconnect(void)
vmbus_connection.conn_state = DISCONNECTED;
- DPRINT_INFO(VMBUS, "Vmbus disconnected!!");
+ pr_info("%s: Vmbus disconnected.", VMBUS_MOD);
Cleanup:
kfree(msg);
@@ -278,7 +272,8 @@ static void process_chn_event(void *context)
* (void*)channel);
*/
} else {
- DPRINT_ERR(VMBUS, "channel not found for relid - %d.", relid);
+ pr_err("%s: %s channel not found for relid - %d.",
+ VMBUS_MOD, __func__, relid);
}
}
@@ -302,11 +297,13 @@ void vmbus_on_event(void)
(unsigned long *)
&recv_int_page[dword])) {
relid = (dword << 5) + bit;
- DPRINT_DBG(VMBUS, "event detected for relid - %d", relid);
if (relid == 0) {
- /* special case - vmbus channel protocol msg */
- DPRINT_DBG(VMBUS, "invalid relid - %d", relid);
+ /*
+ * special case -
+ * vmbus channel
+ * protocol msg
+ */
continue;
} else {
/* QueueWorkItem(VmbusProcessEvent, (void*)relid); */
--
1.6.0.2
^ permalink raw reply related
* [PATCH 5/6] Staging: hv: ring_buffer.c Removed DPRINT replaced with pr_XX
From: Hank Janssen @ 2011-02-22 23:32 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
In-Reply-To: <1298417565-12356-4-git-send-email-hjanssen@microsoft.com>
This group of patches removes all DPRINT from hv_vmbus.ko.
It is divided in several patches due to size.
All DPRINT calls have been removed, and where needed have been
replaced with pr_XX native calls. Many debug DPRINT calls have
been removed outright.
The amount of clutter this driver prints has been
significantly reduced.
Several DPRINT calls remain in this file, they will be removed
in a subsequent patch. They are designed to print out a common
debug stream that will be implemented differently.
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/ring_buffer.c | 25 ++++++++++---------------
1 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/ring_buffer.c b/drivers/staging/hv/ring_buffer.c
index 66688fb..95dd75c 100644
--- a/drivers/staging/hv/ring_buffer.c
+++ b/drivers/staging/hv/ring_buffer.c
@@ -372,19 +372,16 @@ int ringbuffer_write(struct hv_ring_buffer_info *outring_info,
&bytes_avail_toread,
&bytes_avail_towrite);
- DPRINT_DBG(VMBUS, "Writing %u bytes...", totalbytes_towrite);
-
/* Dumpring_info(Outring_info, "BEFORE "); */
/* If there is only room for the packet, assume it is full. */
/* Otherwise, the next time around, we think the ring buffer */
/* is empty since the read index == write index */
if (bytes_avail_towrite <= totalbytes_towrite) {
- DPRINT_DBG(VMBUS,
- "No more space left on outbound ring buffer "
+ pr_debug("%s: %s No more space left on outbound ring buffer "
"(needed %u, avail %u)",
- totalbytes_towrite,
- bytes_avail_towrite);
+ VMBUS_MOD, __func__, totalbytes_towrite,
+ bytes_avail_towrite);
spin_unlock_irqrestore(&outring_info->ring_lock, flags);
return -1;
@@ -499,17 +496,13 @@ int ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
&bytes_avail_toread,
&bytes_avail_towrite);
- DPRINT_DBG(VMBUS, "Reading %u bytes...", buflen);
-
/* Dumpring_info(Inring_info, "BEFORE "); */
/* Make sure there is something to read */
if (bytes_avail_toread < buflen) {
- DPRINT_DBG(VMBUS,
- "got callback but not enough to read "
- "<avail to read %d read size %d>!!",
- bytes_avail_toread,
- buflen);
+ pr_debug("%s: %s got callback but not enough to read "
+ "<avail to read %d read size %d>",
+ VMBUS_MOD, __func__, bytes_avail_toread, buflen);
spin_unlock_irqrestore(&inring_info->ring_lock, flags);
@@ -568,7 +561,8 @@ copyto_ringbuffer(
/* wrap-around detected! */
if (srclen > ring_buffer_size - start_write_offset) {
- DPRINT_DBG(VMBUS, "wrap-around detected!");
+ pr_debug("%s: %s destination wrap-around detected!",
+ VMBUS_MOD, __func__);
frag_len = ring_buffer_size - start_write_offset;
memcpy(ring_buffer + start_write_offset, src, frag_len);
@@ -607,7 +601,8 @@ copyfrom_ringbuffer(
/* wrap-around detected at the src */
if (destlen > ring_buffer_size - start_read_offset) {
- DPRINT_DBG(VMBUS, "src wrap-around detected!");
+ pr_debug("%s: %s source wrap-around detected!",
+ VMBUS_MOD, __func__);
frag_len = ring_buffer_size - start_read_offset;
--
1.6.0.2
^ permalink raw reply related
* [PATCH 4/6] Staging: hv: channel_mgmt.c Removed DPRINT and implemented pr_XX
From: Hank Janssen @ 2011-02-22 23:32 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
In-Reply-To: <1298417565-12356-3-git-send-email-hjanssen@microsoft.com>
This group of patches removes all DPRINT from hv_vmbus.ko.
It is divided in several patches due to size.
All DPRINT calls have been removed, and where needed have been
replaced with pr_XX native calls. Many debug DPRINT calls have
been removed outright.
The amount of clutter this driver prints has been
significantly reduced.
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 73 +++++++------------------------------
1 files changed, 14 insertions(+), 59 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 0781c0e..752de54 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -290,9 +290,7 @@ static void release_channel(struct work_struct *work)
struct vmbus_channel,
work);
- DPRINT_DBG(VMBUS, "releasing channel (%p)", channel);
destroy_workqueue(channel->controlwq);
- DPRINT_DBG(VMBUS, "channel released (%p)", channel);
kfree(channel);
}
@@ -384,8 +382,6 @@ static void vmbus_process_offer(struct work_struct *work)
spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
if (!fnew) {
- DPRINT_DBG(VMBUS, "Ignoring duplicate offer for relid (%d)",
- newchannel->offermsg.child_relid);
free_channel(newchannel);
return;
}
@@ -400,9 +396,6 @@ static void vmbus_process_offer(struct work_struct *work)
&newchannel->offermsg.offer.if_instance,
newchannel);
- DPRINT_DBG(VMBUS, "child device object allocated - %p",
- newchannel->device_obj);
-
/*
* Add the new device to the bus. This will kick off device-driver
* binding which eventually invokes the device driver's AddDevice()
@@ -410,9 +403,8 @@ static void vmbus_process_offer(struct work_struct *work)
*/
ret = vmbus_child_dev_add(newchannel->device_obj);
if (ret != 0) {
- DPRINT_ERR(VMBUS,
- "unable to add child device object (relid %d)",
- newchannel->offermsg.child_relid);
+ pr_err("%s: %s Unable to add child device object (relid %d)",
+ VMBUS_MOD, __func__, newchannel->offermsg.child_relid);
spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
list_del(&newchannel->listentry);
@@ -437,8 +429,8 @@ static void vmbus_process_offer(struct work_struct *work)
hv_cb_utils[cnt].callback,
newchannel) == 0) {
hv_cb_utils[cnt].channel = newchannel;
- DPRINT_INFO(VMBUS, "%s",
- hv_cb_utils[cnt].log_msg);
+ pr_info("%s: %s",
+ VMBUS_MOD, hv_cb_utils[cnt].log_msg);
count_hv_channel();
}
}
@@ -471,48 +463,20 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
}
if (!fsupported) {
- DPRINT_DBG(VMBUS, "Ignoring channel offer notification for "
- "child relid %d", offer->child_relid);
return;
}
guidtype = &offer->offer.if_type;
guidinstance = &offer->offer.if_instance;
- DPRINT_INFO(VMBUS, "Channel offer notification - "
- "child relid %d monitor id %d allocated %d, "
- "type {%02x%02x%02x%02x-%02x%02x-%02x%02x-"
- "%02x%02x%02x%02x%02x%02x%02x%02x} "
- "instance {%02x%02x%02x%02x-%02x%02x-%02x%02x-"
- "%02x%02x%02x%02x%02x%02x%02x%02x}",
- offer->child_relid, offer->monitorid,
- offer->monitor_allocated,
- guidtype->data[3], guidtype->data[2],
- guidtype->data[1], guidtype->data[0],
- guidtype->data[5], guidtype->data[4],
- guidtype->data[7], guidtype->data[6],
- guidtype->data[8], guidtype->data[9],
- guidtype->data[10], guidtype->data[11],
- guidtype->data[12], guidtype->data[13],
- guidtype->data[14], guidtype->data[15],
- guidinstance->data[3], guidinstance->data[2],
- guidinstance->data[1], guidinstance->data[0],
- guidinstance->data[5], guidinstance->data[4],
- guidinstance->data[7], guidinstance->data[6],
- guidinstance->data[8], guidinstance->data[9],
- guidinstance->data[10], guidinstance->data[11],
- guidinstance->data[12], guidinstance->data[13],
- guidinstance->data[14], guidinstance->data[15]);
-
/* Allocate the channel object and save this offer. */
newchannel = alloc_channel();
if (!newchannel) {
- DPRINT_ERR(VMBUS, "unable to allocate channel object");
+ pr_err("%s: %s Unable to allocate channel object",
+ VMBUS_MOD, __func__);
return;
}
- DPRINT_DBG(VMBUS, "channel object allocated - %p", newchannel);
-
memcpy(&newchannel->offermsg, offer,
sizeof(struct vmbus_channel_offer_channel));
newchannel->monitor_grp = (u8)offer->monitorid / 32;
@@ -536,8 +500,6 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
rescind = (struct vmbus_channel_rescind_offer *)hdr;
channel = relid2channel(rescind->child_relid);
if (channel == NULL) {
- DPRINT_DBG(VMBUS, "channel not found for relId %d",
- rescind->child_relid);
return;
}
@@ -573,7 +535,6 @@ static void vmbus_onopen_result(struct vmbus_channel_message_header *hdr)
unsigned long flags;
result = (struct vmbus_channel_open_result *)hdr;
- DPRINT_DBG(VMBUS, "vmbus open result - %d", result->status);
/*
* Find the open msg, copy the result and signal/unblock the wait event
@@ -618,8 +579,6 @@ static void vmbus_ongpadl_created(struct vmbus_channel_message_header *hdr)
unsigned long flags;
gpadlcreated = (struct vmbus_channel_gpadl_created *)hdr;
- DPRINT_DBG(VMBUS, "vmbus gpadl created result - %d",
- gpadlcreated->creation_status);
/*
* Find the establish msg, copy the result and signal/unblock the wait
@@ -770,12 +729,9 @@ void vmbus_onmessage(void *context)
hdr = (struct vmbus_channel_message_header *)msg->u.payload;
size = msg->header.payload_size;
- DPRINT_DBG(VMBUS, "message type %d size %d", hdr->msgtype, size);
-
if (hdr->msgtype >= CHANNELMSG_COUNT) {
- DPRINT_ERR(VMBUS,
- "Received invalid channel message type %d size %d",
- hdr->msgtype, size);
+ pr_err("%s: %s Received invalid channel msg type %d size %d",
+ VMBUS_MOD, __func__, hdr->msgtype, size);
print_hex_dump_bytes("", DUMP_PREFIX_NONE,
(unsigned char *)msg->u.payload, size);
return;
@@ -784,8 +740,8 @@ void vmbus_onmessage(void *context)
if (gChannelMessageTable[hdr->msgtype].messageHandler)
gChannelMessageTable[hdr->msgtype].messageHandler(hdr);
else
- DPRINT_ERR(VMBUS, "Unhandled channel message type %d",
- hdr->msgtype);
+ pr_err("%s: %s Unhandled channel message type %d",
+ VMBUS_MOD, __func__, hdr->msgtype);
}
/*
@@ -813,8 +769,8 @@ int vmbus_request_offers(void)
ret = vmbus_post_msg(msg,
sizeof(struct vmbus_channel_message_header));
if (ret != 0) {
- DPRINT_ERR(VMBUS, "Unable to request offers - %d", ret);
-
+ pr_err("%s: %s Unable to request offers - %d",
+ VMBUS_MOD, __func__, ret);
goto cleanup;
}
@@ -854,9 +810,8 @@ void vmbus_release_unattached_channels(void)
if (!channel->device_obj->drv) {
list_del(&channel->listentry);
- DPRINT_INFO(VMBUS,
- "Releasing unattached device object %p",
- channel->device_obj);
+ pr_info("%s: Releasing unattached dev object %p",
+ VMBUS_MOD, channel->device_obj);
vmbus_child_device_unregister(channel->device_obj);
free_channel(channel);
--
1.6.0.2
^ permalink raw reply related
* [PATCH 3/6] Staging: hv: channel.c Removed debug DPRINTS use pr_err for errors
From: Hank Janssen @ 2011-02-22 23:32 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
In-Reply-To: <1298417565-12356-2-git-send-email-hjanssen@microsoft.com>
This group of patches removes all DPRINT from hv_vmbus.ko.
It is divided in several patches due to size.
All DPRINT calls have been removed, and where needed have been
replaced with pr_XX native calls. Many debug DPRINT calls have
been removed outright.
The amount of clutter this driver prints has been
significantly reduced.
Several DPRINT calls remain in this file, they will be removed
in a subsequent patch. They are designed to print out a common
debug stream that will be implemented differently.
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/channel.c | 71 ++++++-----------------------------------
1 files changed, 11 insertions(+), 60 deletions(-)
diff --git a/drivers/staging/hv/channel.c b/drivers/staging/hv/channel.c
index 775a52a..654e80c 100644
--- a/drivers/staging/hv/channel.c
+++ b/drivers/staging/hv/channel.c
@@ -213,9 +213,6 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
/* Establish the gpadl for the ring buffer */
- DPRINT_DBG(VMBUS, "Establishing ring buffer's gpadl for channel %p...",
- newchannel);
-
newchannel->ringbuffer_gpadlhandle = 0;
ret = vmbus_establish_gpadl(newchannel,
@@ -229,16 +226,6 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
goto errorout;
}
- DPRINT_DBG(VMBUS, "channel %p <relid %d gpadl 0x%x send ring %p "
- "size %d recv ring %p size %d, downstreamoffset %d>",
- newchannel, newchannel->offermsg.child_relid,
- newchannel->ringbuffer_gpadlhandle,
- newchannel->outbound.ring_buffer,
- newchannel->outbound.ring_size,
- newchannel->inbound.ring_buffer,
- newchannel->inbound.ring_size,
- send_ringbuffer_size);
-
/* Create and init the channel open message */
openInfo = kmalloc(sizeof(*openInfo) +
sizeof(struct vmbus_channel_open_channel),
@@ -272,12 +259,11 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
&vmbus_connection.chn_msg_list);
spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
- DPRINT_DBG(VMBUS, "Sending channel open msg...");
-
ret = vmbus_post_msg(openMsg,
sizeof(struct vmbus_channel_open_channel));
if (ret != 0) {
- DPRINT_ERR(VMBUS, "unable to open channel - %d", ret);
+ pr_err("%s: %s Unable to open channel - %d",
+ VMBUS_MOD, __func__, ret);
goto Cleanup;
}
@@ -291,10 +277,9 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
}
- if (openInfo->response.open_result.status == 0)
- DPRINT_INFO(VMBUS, "channel <%p> open success!!", newchannel);
- else
- DPRINT_INFO(VMBUS, "channel <%p> open failed - %d!!",
+ if (openInfo->response.open_result.status)
+ pr_err("%s: %s Channel <%p> open failed - %d!!",
+ VMBUS_MOD, __func__,
newchannel, openInfo->response.open_result.status);
Cleanup:
@@ -530,17 +515,13 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
&vmbus_connection.chn_msg_list);
spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
- DPRINT_DBG(VMBUS, "buffer %p, size %d msg cnt %d",
- kbuffer, size, msgcount);
-
- DPRINT_DBG(VMBUS, "Sending GPADL Header - len %zd",
- msginfo->msgsize - sizeof(*msginfo));
msginfo->wait_condition = 0;
ret = vmbus_post_msg(gpadlmsg, msginfo->msgsize -
sizeof(*msginfo));
if (ret != 0) {
- DPRINT_ERR(VMBUS, "Unable to open channel - %d", ret);
+ pr_err("%s: %s Unable to open channel - %d",
+ VMBUS_MOD, __func__, ret);
goto Cleanup;
}
@@ -556,10 +537,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
CHANNELMSG_GPADL_BODY;
gpadl_body->gpadl = next_gpadl_handle;
- DPRINT_DBG(VMBUS, "Sending GPADL Body - len %zd",
- submsginfo->msgsize -
- sizeof(*submsginfo));
-
dump_gpadl_body(gpadl_body, submsginfo->msgsize -
sizeof(*submsginfo));
ret = vmbus_post_msg(gpadl_body,
@@ -577,12 +554,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
/* At this point, we received the gpadl created msg */
- DPRINT_DBG(VMBUS, "Received GPADL created "
- "(relid %d, status %d handle %x)",
- channel->offermsg.child_relid,
- msginfo->response.gpadl_created.creation_status,
- gpadlmsg->gpadl);
-
*gpadl_handle = gpadlmsg->gpadl;
Cleanup:
@@ -730,9 +701,6 @@ int vmbus_sendpacket(struct vmbus_channel *channel, const void *buffer,
u64 aligned_data = 0;
int ret;
- DPRINT_DBG(VMBUS, "channel %p buffer %p len %d",
- channel, buffer, bufferlen);
-
dump_vmbus_channel(channel);
/* ASSERT((packetLenAligned - packetLen) < sizeof(u64)); */
@@ -846,10 +814,6 @@ int vmbus_sendpacket_multipagebuffer(struct vmbus_channel *channel,
dump_vmbus_channel(channel);
- DPRINT_DBG(VMBUS, "data buffer - offset %u len %u pfn count %u",
- multi_pagebuffer->offset,
- multi_pagebuffer->len, pfncount);
-
if ((pfncount < 0) || (pfncount > MAX_MULTIPAGE_BUFFER_COUNT))
return -EINVAL;
@@ -927,7 +891,6 @@ int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
if (ret != 0) {
spin_unlock_irqrestore(&channel->inbound_lock, flags);
- /* DPRINT_DBG(VMBUS, "nothing to read!!"); */
return 0;
}
@@ -937,18 +900,13 @@ int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
userlen = packetlen - (desc.offset8 << 3);
/* ASSERT(userLen > 0); */
- DPRINT_DBG(VMBUS, "packet received on channel %p relid %d <type %d "
- "flag %d tid %llx pktlen %d datalen %d> ",
- channel, channel->offermsg.child_relid, desc.type,
- desc.flags, desc.trans_id, packetlen, userlen);
-
*buffer_actual_len = userlen;
if (userlen > bufferlen) {
spin_unlock_irqrestore(&channel->inbound_lock, flags);
- DPRINT_ERR(VMBUS, "buffer too small - got %d needs %d",
- bufferlen, userlen);
+ pr_err("%s: %s Buffer too small - got %d need %d",
+ VMBUS_MOD, __func__, bufferlen, userlen);
return -1;
}
@@ -987,7 +945,6 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
if (ret != 0) {
spin_unlock_irqrestore(&channel->inbound_lock, flags);
- /* DPRINT_DBG(VMBUS, "nothing to read!!"); */
return 0;
}
@@ -996,18 +953,13 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
packetlen = desc.len8 << 3;
userlen = packetlen - (desc.offset8 << 3);
- DPRINT_DBG(VMBUS, "packet received on channel %p relid %d <type %d "
- "flag %d tid %llx pktlen %d datalen %d> ",
- channel, channel->offermsg.child_relid, desc.type,
- desc.flags, desc.trans_id, packetlen, userlen);
-
*buffer_actual_len = packetlen;
if (packetlen > bufferlen) {
spin_unlock_irqrestore(&channel->inbound_lock, flags);
- DPRINT_ERR(VMBUS, "buffer too small - needed %d bytes but "
- "got space for only %d bytes", packetlen, bufferlen);
+ pr_err("%s: %s Buffer too small - needed %d bytes got "
+ "%d bytes", VMBUS_MOD, __func__, packetlen, bufferlen);
return -2;
}
@@ -1050,7 +1002,6 @@ void vmbus_ontimer(unsigned long data)
*/
static void dump_vmbus_channel(struct vmbus_channel *channel)
{
- DPRINT_DBG(VMBUS, "Channel (%d)", channel->offermsg.child_relid);
dump_ring_info(&channel->outbound, "Outbound ");
dump_ring_info(&channel->inbound, "Inbound ");
}
--
1.6.0.2
^ permalink raw reply related
* [PATCH 2/6] Staging: hv: hv.c Removed all DPRINT and debug - using pr_err now
From: Hank Janssen @ 2011-02-22 23:32 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
Cc: K. Y. Srinivasan
In-Reply-To: <1298417565-12356-1-git-send-email-hjanssen@microsoft.com>
This group of patches removes all DPRINT from hv_vmbus.ko.
It is divided in several patches due to size.
All DPRINT calls have been removed, and where needed have been
replaced with pr_XX native calls. Many debug DPRINT calls have
been removed outright.
The amount of clutter this driver prints has been
significantly reduced.
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/hv.c | 88 +++++++++++-----------------------------------
1 files changed, 21 insertions(+), 67 deletions(-)
diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
index 2d492ad..e3ce26d 100644
--- a/drivers/staging/hv/hv.c
+++ b/drivers/staging/hv/hv.c
@@ -80,20 +80,6 @@ static int query_hypervisor_info(void)
op = HVCPUID_VENDOR_MAXFUNCTION;
cpuid(op, &eax, &ebx, &ecx, &edx);
- DPRINT_INFO(VMBUS, "Vendor ID: %c%c%c%c%c%c%c%c%c%c%c%c",
- (ebx & 0xFF),
- ((ebx >> 8) & 0xFF),
- ((ebx >> 16) & 0xFF),
- ((ebx >> 24) & 0xFF),
- (ecx & 0xFF),
- ((ecx >> 8) & 0xFF),
- ((ecx >> 16) & 0xFF),
- ((ecx >> 24) & 0xFF),
- (edx & 0xFF),
- ((edx >> 8) & 0xFF),
- ((edx >> 16) & 0xFF),
- ((edx >> 24) & 0xFF));
-
max_leaf = eax;
eax = 0;
ebx = 0;
@@ -102,12 +88,6 @@ static int query_hypervisor_info(void)
op = HVCPUID_INTERFACE;
cpuid(op, &eax, &ebx, &ecx, &edx);
- DPRINT_INFO(VMBUS, "Interface ID: %c%c%c%c",
- (eax & 0xFF),
- ((eax >> 8) & 0xFF),
- ((eax >> 16) & 0xFF),
- ((eax >> 24) & 0xFF));
-
if (max_leaf >= HVCPUID_VERSION) {
eax = 0;
ebx = 0;
@@ -115,14 +95,17 @@ static int query_hypervisor_info(void)
edx = 0;
op = HVCPUID_VERSION;
cpuid(op, &eax, &ebx, &ecx, &edx);
- DPRINT_INFO(VMBUS, "OS Build:%d-%d.%d-%d-%d.%d",\
- eax,
- ebx >> 16,
- ebx & 0xFFFF,
- ecx,
- edx >> 24,
- edx & 0xFFFFFF);
+
+ pr_info("%s: Hyper-V Host OS Build:%d-%d.%d-%d-%d.%d",
+ VMBUS_MOD,
+ eax,
+ ebx >> 16,
+ ebx & 0xFFFF,
+ ecx,
+ edx >> 24,
+ edx & 0xFFFFFF);
}
+
return max_leaf;
}
@@ -137,20 +120,12 @@ static u64 do_hypercall(u64 control, void *input, void *output)
u64 output_address = (output) ? virt_to_phys(output) : 0;
volatile void *hypercall_page = hv_context.hypercall_page;
- DPRINT_DBG(VMBUS, "Hypercall <control %llx input phys %llx virt %p "
- "output phys %llx virt %p hypercall %p>",
- control, input_address, input,
- output_address, output, hypercall_page);
-
__asm__ __volatile__("mov %0, %%r8" : : "r" (output_address) : "r8");
__asm__ __volatile__("call *%3" : "=a" (hv_status) :
"c" (control), "d" (input_address),
"m" (hypercall_page));
- DPRINT_DBG(VMBUS, "Hypercall <return %llx>", hv_status);
-
return hv_status;
-
#else
u32 control_hi = control >> 32;
@@ -165,18 +140,12 @@ static u64 do_hypercall(u64 control, void *input, void *output)
u32 output_address_lo = output_address & 0xFFFFFFFF;
volatile void *hypercall_page = hv_context.hypercall_page;
- DPRINT_DBG(VMBUS, "Hypercall <control %llx input %p output %p>",
- control, input, output);
-
__asm__ __volatile__ ("call *%8" : "=d"(hv_status_hi),
"=a"(hv_status_lo) : "d" (control_hi),
"a" (control_lo), "b" (input_address_hi),
"c" (input_address_lo), "D"(output_address_hi),
"S"(output_address_lo), "m" (hypercall_page));
- DPRINT_DBG(VMBUS, "Hypercall <return %llx>",
- hv_status_lo | ((u64)hv_status_hi << 32));
-
return hv_status_lo | ((u64)hv_status_hi << 32);
#endif /* !x86_64 */
}
@@ -198,13 +167,10 @@ int hv_init(void)
sizeof(void *) * MAX_NUM_CPUS);
if (!query_hypervisor_presence()) {
- DPRINT_ERR(VMBUS, "No Windows hypervisor detected!!");
+ pr_err("%s: %s No Hyper-V detected", VMBUS_MOD, __func__);
goto Cleanup;
}
- DPRINT_INFO(VMBUS,
- "Windows hypervisor detected! Retrieving more info...");
-
max_leaf = query_hypervisor_info();
/* HvQueryHypervisorFeatures(maxLeaf); */
@@ -214,8 +180,8 @@ int hv_init(void)
rdmsrl(HV_X64_MSR_GUEST_OS_ID, hv_context.guestid);
if (hv_context.guestid != 0) {
- DPRINT_ERR(VMBUS, "Unknown guest id (0x%llx)!!",
- hv_context.guestid);
+ pr_err("%s: %s Unknown guest id (0x%llx)",
+ VMBUS_MOD, __func__, hv_context.guestid);
goto Cleanup;
}
@@ -233,8 +199,8 @@ int hv_init(void)
virtaddr = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_EXEC);
if (!virtaddr) {
- DPRINT_ERR(VMBUS,
- "unable to allocate hypercall page!!");
+ pr_err("%s: %s unable to allocate hypercall page",
+ VMBUS_MOD, __func__);
goto Cleanup;
}
@@ -248,16 +214,13 @@ int hv_init(void)
rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
if (!hypercall_msr.enable) {
- DPRINT_ERR(VMBUS, "unable to set hypercall page!!");
+ pr_err("%s: %s Unable to set hypercall page",
+ VMBUS_MOD, __func__);
goto Cleanup;
}
hv_context.hypercall_page = virtaddr;
- DPRINT_INFO(VMBUS, "Hypercall page VA=%p, PA=0x%0llx",
- hv_context.hypercall_page,
- (u64)hypercall_msr.guest_physical_address << PAGE_SHIFT);
-
/* Setup the global signal event param for the signal event hypercall */
hv_context.signal_event_buffer =
kmalloc(sizeof(struct hv_input_signal_event_buffer),
@@ -394,14 +357,12 @@ void hv_synic_init(void *irqarg)
/* Check the version */
rdmsrl(HV_X64_MSR_SVERSION, version);
- DPRINT_INFO(VMBUS, "SynIC version: %llx", version);
-
hv_context.synic_message_page[cpu] =
(void *)get_zeroed_page(GFP_ATOMIC);
if (hv_context.synic_message_page[cpu] == NULL) {
- DPRINT_ERR(VMBUS,
- "unable to allocate SYNIC message page!!");
+ pr_err("%s: %s Unable to allocate SYNIC message page",
+ VMBUS_MOD, __func__);
goto Cleanup;
}
@@ -409,8 +370,8 @@ void hv_synic_init(void *irqarg)
(void *)get_zeroed_page(GFP_ATOMIC);
if (hv_context.synic_event_page[cpu] == NULL) {
- DPRINT_ERR(VMBUS,
- "unable to allocate SYNIC event page!!");
+ pr_err("%s: %s Unable to allocate SYNIC event page",
+ VMBUS_MOD, __func__);
goto Cleanup;
}
@@ -420,8 +381,6 @@ void hv_synic_init(void *irqarg)
simp.base_simp_gpa = virt_to_phys(hv_context.synic_message_page[cpu])
>> PAGE_SHIFT;
- DPRINT_DBG(VMBUS, "HV_X64_MSR_SIMP msr set to: %llx", simp.as_uint64);
-
wrmsrl(HV_X64_MSR_SIMP, simp.as_uint64);
/* Setup the Synic's event page */
@@ -430,8 +389,6 @@ void hv_synic_init(void *irqarg)
siefp.base_siefp_gpa = virt_to_phys(hv_context.synic_event_page[cpu])
>> PAGE_SHIFT;
- DPRINT_DBG(VMBUS, "HV_X64_MSR_SIEFP msr set to: %llx", siefp.as_uint64);
-
wrmsrl(HV_X64_MSR_SIEFP, siefp.as_uint64);
/* Setup the interception SINT. */
@@ -446,9 +403,6 @@ void hv_synic_init(void *irqarg)
shared_sint.masked = false;
shared_sint.auto_eoi = true;
- DPRINT_DBG(VMBUS, "HV_X64_MSR_SINT1 msr set to: %llx",
- shared_sint.as_uint64);
-
wrmsrl(HV_X64_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
/* Enable the global synic bit */
--
1.6.0.2
^ permalink raw reply related
* [PATCH 1/6] Staging: hv: vmbus_drv.c Replaced DPRINT with native pr_XXX
From: Hank Janssen @ 2011-02-22 23:32 UTC (permalink / raw)
To: hjanssen, haiyangz, gregkh, linux-kernel, devel, virtualization
This group of patches removes all DPRINT from hv_vmbus.ko.
It is divided in several patches due to size.
All DPRINT calls have been removed, and where needed have been
replaced with pr_XX native calls. Many debug DPRINT calls have
been removed outright.
The amount of clutter this driver prints has been
significantly reduced.
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/staging/hv/logging.h | 1 +
drivers/staging/hv/vmbus_drv.c | 145 +++++++++++-----------------------------
2 files changed, 39 insertions(+), 107 deletions(-)
diff --git a/drivers/staging/hv/logging.h b/drivers/staging/hv/logging.h
index 1799951..517d721 100644
--- a/drivers/staging/hv/logging.h
+++ b/drivers/staging/hv/logging.h
@@ -31,6 +31,7 @@
/* #include <linux/init.h> */
/* #include <linux/module.h> */
+#define VMBUS_MOD "hv_vmbus"
#define VMBUS 0x0001
#define STORVSC 0x0002
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..a560a80 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -279,22 +279,16 @@ static int vmbus_on_isr(struct hv_driver *drv)
msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
/* Check if there are actual msgs to be process */
- if (msg->header.message_type != HVMSG_NONE) {
- DPRINT_DBG(VMBUS, "received msg type %d size %d",
- msg->header.message_type,
- msg->header.payload_size);
+ if (msg->header.message_type != HVMSG_NONE)
ret |= 0x1;
- }
/* TODO: Check if there are events to be process */
page_addr = hv_context.synic_event_page[cpu];
event = (union hv_synic_event_flags *)page_addr + VMBUS_MESSAGE_SINT;
/* Since we are a child, we only need to check bit 0 */
- if (test_and_clear_bit(0, (unsigned long *) &event->flags32[0])) {
- DPRINT_DBG(VMBUS, "received event %d", event->flags32[0]);
+ if (test_and_clear_bit(0, (unsigned long *) &event->flags32[0]))
ret |= 0x2;
- }
return ret;
}
@@ -468,17 +462,6 @@ static int vmbus_bus_init(void)
int ret;
unsigned int vector;
- DPRINT_INFO(VMBUS, "+++++++ HV Driver version = %s +++++++",
- HV_DRV_VERSION);
- DPRINT_INFO(VMBUS, "+++++++ Vmbus supported version = %d +++++++",
- VMBUS_REVISION_NUMBER);
- DPRINT_INFO(VMBUS, "+++++++ Vmbus using SINT %d +++++++",
- VMBUS_MESSAGE_SINT);
- DPRINT_DBG(VMBUS, "sizeof(vmbus_channel_packet_page_buffer)=%zd, "
- "sizeof(VMBUS_CHANNEL_PACKET_MULITPAGE_BUFFER)=%zd",
- sizeof(struct vmbus_channel_packet_page_buffer),
- sizeof(struct vmbus_channel_packet_multipage_buffer));
-
driver->name = driver_name;
memcpy(&driver->dev_type, &device_type, sizeof(struct hv_guid));
@@ -490,15 +473,8 @@ static int vmbus_bus_init(void)
/* Hypervisor initialization...setup hypercall page..etc */
ret = hv_init();
if (ret != 0) {
- DPRINT_ERR(VMBUS, "Unable to initialize the hypervisor - 0x%x",
- ret);
- goto cleanup;
- }
-
- /* Sanity checks */
- if (!driver->dev_add) {
- DPRINT_ERR(VMBUS_DRV, "OnDeviceAdd() routine not set");
- ret = -1;
+ pr_err("%s: %s - Unable to initialize hypervisor - 0x%x",
+ VMBUS_MOD, __func__, ret);
goto cleanup;
}
@@ -522,8 +498,8 @@ static int vmbus_bus_init(void)
driver->name, NULL);
if (ret != 0) {
- DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
- vmbus_irq);
+ pr_err("%s: %s ERROR - Unable to request IRQ %d",
+ VMBUS_MOD, __func__, vmbus_irq);
bus_unregister(&vmbus_drv_ctx->bus);
@@ -532,15 +508,15 @@ static int vmbus_bus_init(void)
}
vector = VMBUS_IRQ_VECTOR;
- DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
+ pr_info("%s: irq 0x%x vector 0x%x", VMBUS_MOD, vmbus_irq, vector);
/* Call to bus driver to add the root device */
memset(dev_ctx, 0, sizeof(struct vm_device));
ret = driver->dev_add(&dev_ctx->device_obj, &vector);
if (ret != 0) {
- DPRINT_ERR(VMBUS_DRV,
- "ERROR - Unable to add vmbus root device");
+ pr_err("%s: %s ERROR - Unable to add hv_vmbus root device",
+ VMBUS_MOD, __func__);
free_irq(vmbus_irq, NULL);
@@ -567,8 +543,8 @@ static int vmbus_bus_init(void)
/* Setup the bus as root device */
ret = device_register(&dev_ctx->device);
if (ret) {
- DPRINT_ERR(VMBUS_DRV,
- "ERROR - Unable to register vmbus root device");
+ pr_err("%s: %s ERROR Unable to register vmbus root device",
+ VMBUS_MOD, __func__);
free_irq(vmbus_irq, NULL);
bus_unregister(&vmbus_drv_ctx->bus);
@@ -631,9 +607,6 @@ int vmbus_child_driver_register(struct driver_context *driver_ctx)
{
int ret;
- DPRINT_INFO(VMBUS_DRV, "child driver (%p) registering - name %s",
- driver_ctx, driver_ctx->driver.name);
-
/* The child driver on this vmbus */
driver_ctx->driver.bus = &vmbus_drv.bus;
@@ -641,6 +614,13 @@ int vmbus_child_driver_register(struct driver_context *driver_ctx)
vmbus_request_offers();
+ if (ret)
+ pr_err("%s: %s Unable to register Hyper-V driver %s",
+ VMBUS_MOD, __func__, driver_ctx->driver.name);
+ else
+ pr_info("%s: Hyper-V driver registering %s", VMBUS_MOD,
+ driver_ctx->driver.name);
+
return ret;
}
EXPORT_SYMBOL(vmbus_child_driver_register);
@@ -658,11 +638,11 @@ EXPORT_SYMBOL(vmbus_child_driver_register);
*/
void vmbus_child_driver_unregister(struct driver_context *driver_ctx)
{
- DPRINT_INFO(VMBUS_DRV, "child driver (%p) unregistering - name %s",
- driver_ctx, driver_ctx->driver.name);
-
driver_unregister(&driver_ctx->driver);
+ pr_info("%s: child driver unregistering - %s", VMBUS_MOD,
+ driver_ctx->driver.name);
+
driver_ctx->driver.bus = NULL;
}
EXPORT_SYMBOL(vmbus_child_driver_unregister);
@@ -681,30 +661,11 @@ struct hv_device *vmbus_child_device_create(struct hv_guid *type,
/* Allocate the new child device */
child_device_ctx = kzalloc(sizeof(struct vm_device), GFP_KERNEL);
if (!child_device_ctx) {
- DPRINT_ERR(VMBUS_DRV,
- "unable to allocate device_context for child device");
+ pr_err("%s: %s Unable to allocate device_context child device",
+ VMBUS_MOD, __func__);
return NULL;
}
- DPRINT_DBG(VMBUS_DRV, "child device (%p) allocated - "
- "type {%02x%02x%02x%02x-%02x%02x-%02x%02x-"
- "%02x%02x%02x%02x%02x%02x%02x%02x},"
- "id {%02x%02x%02x%02x-%02x%02x-%02x%02x-"
- "%02x%02x%02x%02x%02x%02x%02x%02x}",
- &child_device_ctx->device,
- type->data[3], type->data[2], type->data[1], type->data[0],
- type->data[5], type->data[4], type->data[7], type->data[6],
- type->data[8], type->data[9], type->data[10], type->data[11],
- type->data[12], type->data[13], type->data[14], type->data[15],
- instance->data[3], instance->data[2],
- instance->data[1], instance->data[0],
- instance->data[5], instance->data[4],
- instance->data[7], instance->data[6],
- instance->data[8], instance->data[9],
- instance->data[10], instance->data[11],
- instance->data[12], instance->data[13],
- instance->data[14], instance->data[15]);
-
child_device_obj = &child_device_ctx->device_obj;
child_device_obj->channel = channel;
memcpy(&child_device_obj->dev_type, type, sizeof(struct hv_guid));
@@ -730,9 +691,6 @@ int vmbus_child_device_register(struct hv_device *root_device_obj,
to_vm_device(child_device_obj);
static atomic_t device_num = ATOMIC_INIT(0);
- DPRINT_DBG(VMBUS_DRV, "child device (%p) registering",
- child_device_ctx);
-
/* Set the device name. Otherwise, device_register() will fail. */
dev_set_name(&child_device_ctx->device, "vmbus_0_%d",
atomic_inc_return(&device_num));
@@ -752,11 +710,11 @@ int vmbus_child_device_register(struct hv_device *root_device_obj,
ret = child_device_ctx->probe_error;
if (ret)
- DPRINT_ERR(VMBUS_DRV, "unable to register child device (%p)",
- &child_device_ctx->device);
+ pr_err("%s: %s Unable to register child device",
+ VMBUS_MOD, __func__);
else
- DPRINT_INFO(VMBUS_DRV, "child device (%p) registered",
- &child_device_ctx->device);
+ pr_info("%s: Child device (%s) registered", VMBUS_MOD,
+ dev_name(&child_device_ctx->device));
return ret;
}
@@ -769,17 +727,14 @@ void vmbus_child_device_unregister(struct hv_device *device_obj)
{
struct vm_device *device_ctx = to_vm_device(device_obj);
- DPRINT_INFO(VMBUS_DRV, "unregistering child device (%p)",
- &device_ctx->device);
-
/*
* Kick off the process of unregistering the device.
* This will call vmbus_remove() and eventually vmbus_device_release()
*/
device_unregister(&device_ctx->device);
- DPRINT_INFO(VMBUS_DRV, "child device (%p) unregistered",
- &device_ctx->device);
+ pr_info("%s: child device %s unregistered", VMBUS_MOD,
+ dev_name(&device_ctx->device));
}
/*
@@ -794,21 +749,6 @@ static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
struct vm_device *device_ctx = device_to_vm_device(device);
int ret;
- DPRINT_INFO(VMBUS_DRV, "generating uevent - VMBUS_DEVICE_CLASS_GUID={"
- "%02x%02x%02x%02x-%02x%02x-%02x%02x-"
- "%02x%02x%02x%02x%02x%02x%02x%02x}",
- device_ctx->class_id.data[3], device_ctx->class_id.data[2],
- device_ctx->class_id.data[1], device_ctx->class_id.data[0],
- device_ctx->class_id.data[5], device_ctx->class_id.data[4],
- device_ctx->class_id.data[7], device_ctx->class_id.data[6],
- device_ctx->class_id.data[8], device_ctx->class_id.data[9],
- device_ctx->class_id.data[10],
- device_ctx->class_id.data[11],
- device_ctx->class_id.data[12],
- device_ctx->class_id.data[13],
- device_ctx->class_id.data[14],
- device_ctx->class_id.data[15]);
-
ret = add_uevent_var(env, "VMBUS_DEVICE_CLASS_GUID={"
"%02x%02x%02x%02x-%02x%02x-%02x%02x-"
"%02x%02x%02x%02x%02x%02x%02x%02x}",
@@ -877,10 +817,6 @@ static int vmbus_match(struct device *device, struct device_driver *driver)
(struct vmbus_driver_context *)driver_ctx;
device_ctx->device_obj.drv = &vmbus_drv_ctx->drv_obj;
- DPRINT_INFO(VMBUS_DRV,
- "device object (%p) set to driver object (%p)",
- &device_ctx->device_obj,
- device_ctx->device_obj.drv);
match = 1;
}
@@ -922,18 +858,17 @@ static int vmbus_probe(struct device *child_device)
if (driver_ctx->probe) {
ret = device_ctx->probe_error = driver_ctx->probe(child_device);
if (ret != 0) {
- DPRINT_ERR(VMBUS_DRV, "probe() failed for device %s "
- "(%p) on driver %s (%d)...",
- dev_name(child_device), child_device,
- child_device->driver->name, ret);
+ pr_err("%s: %s failed for device %s (%d)",
+ VMBUS_MOD, __func__,
+ dev_name(child_device), ret);
INIT_WORK(&device_ctx->probe_failed_work_item,
vmbus_probe_failed_cb);
schedule_work(&device_ctx->probe_failed_work_item);
}
} else {
- DPRINT_ERR(VMBUS_DRV, "probe() method not set for driver - %s",
- child_device->driver->name);
+ pr_err("%s: %s not set for driver - %s",
+ VMBUS_MOD, __func__, dev_name(child_device));
ret = -1;
}
return ret;
@@ -966,9 +901,8 @@ static int vmbus_remove(struct device *child_device)
if (driver_ctx->remove) {
ret = driver_ctx->remove(child_device);
} else {
- DPRINT_ERR(VMBUS_DRV,
- "remove() method not set for driver - %s",
- child_device->driver->name);
+ pr_err("%s: %s not set for driver - %s",
+ VMBUS_MOD, __func__, dev_name(child_device));
ret = -1;
}
}
@@ -1086,10 +1020,8 @@ MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
static int __init vmbus_init(void)
{
- DPRINT_INFO(VMBUS_DRV,
- "Vmbus initializing.... current log level 0x%x (%x,%x)",
- vmbus_loglevel, HIWORD(vmbus_loglevel), LOWORD(vmbus_loglevel));
- /* Todo: it is used for loglevel, to be ported to new kernel. */
+ pr_info("%s: initializing Version %s. Supported Hyper-V Rev %d.",
+ VMBUS_MOD, HV_DRV_VERSION, VMBUS_REVISION_NUMBER);
if (!dmi_check_system(microsoft_hv_dmi_table))
return -ENODEV;
@@ -1100,7 +1032,6 @@ static int __init vmbus_init(void)
static void __exit vmbus_exit(void)
{
vmbus_bus_exit();
- /* Todo: it is used for loglevel, to be ported to new kernel. */
}
/*
--
1.6.0.2
^ permalink raw reply related
* Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: Greg KH @ 2011-02-21 15:43 UTC (permalink / raw)
To: Thomas Gleixner
Cc: KY Srinivasan, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <alpine.LFD.2.00.1102211549340.2701@localhost6.localdomain6>
On Mon, Feb 21, 2011 at 03:51:56PM +0100, Thomas Gleixner wrote:
> On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > > There are various ways to solve that proper.
> > > > >
> > > > > - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > > > provides as enumeration.
> > > > >
> > > > > - Use a fixed vector like XEN does for the event channel
> > > > >
> > > > > - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > > MSI(-X)
> > > > >
> > > > > Thanks,
> > > > >
> > > > > tglx
> > > >
> > > > I am not claiming that what I have done here is the best possible solution.
> > > > However, I will submit to you that it is better than what we had here
> > > > prior to this patch. I will address this and a whole lot of other issues
> > > > in future patches.
> > >
> > > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > > against breaking random other drivers. Care to explain why you think
> > > that's better ?
> >
> > The root device for the VM is bound to the PV driver on some distributions.
> > So, if we cannot load the PV drivers, we do not have a system that boots.
> > In general, the system performance without these PV drivers is so poor that
> > for all practical purposes, having the PV drivers is almost a requirement
> > for having a useable system. While the platform supports configuration of the VM with
> > some emulated devices, it is not a recommended configuration (because of
> > performance reasons) for Linux virtual machines on the Hyper-V platform.
>
> It does not matter whether it's recommended or not. If it results in a
> non usable emulated device it's broken. Just imagine you grab the
> serial interrupt, which results in not having a debug console. Not at
> all acceptable.
>
> > I have spent significantly more time debating this patch than
> > developing this patch that I still think improves the current
> > driver. I will leave it to Greg and other powers that be to decide
> > if this patch will be accepted. Let me know what your verdict is.
>
> As I said, any other clean solution (and I pointed out 3) over this
> hack.
Yeah, that sounds like the best solution, let's not cause accidental
breakage of systems that were working already.
KY, care to fix this up properly now? There's no rush to take your
previous patch, as that is working ok for now, right?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: Thomas Gleixner @ 2011-02-21 14:51 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E048013C67@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > There are various ways to solve that proper.
> > > >
> > > > - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > > provides as enumeration.
> > > >
> > > > - Use a fixed vector like XEN does for the event channel
> > > >
> > > > - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > > MSI(-X)
> > > >
> > > > Thanks,
> > > >
> > > > tglx
> > >
> > > I am not claiming that what I have done here is the best possible solution.
> > > However, I will submit to you that it is better than what we had here
> > > prior to this patch. I will address this and a whole lot of other issues
> > > in future patches.
> >
> > No, it's _NOT_ better in any way. You trade breaking your PV thing
> > against breaking random other drivers. Care to explain why you think
> > that's better ?
>
> The root device for the VM is bound to the PV driver on some distributions.
> So, if we cannot load the PV drivers, we do not have a system that boots.
> In general, the system performance without these PV drivers is so poor that
> for all practical purposes, having the PV drivers is almost a requirement
> for having a useable system. While the platform supports configuration of the VM with
> some emulated devices, it is not a recommended configuration (because of
> performance reasons) for Linux virtual machines on the Hyper-V platform.
It does not matter whether it's recommended or not. If it results in a
non usable emulated device it's broken. Just imagine you grab the
serial interrupt, which results in not having a debug console. Not at
all acceptable.
> I have spent significantly more time debating this patch than
> developing this patch that I still think improves the current
> driver. I will leave it to Greg and other powers that be to decide
> if this patch will be accepted. Let me know what your verdict is.
As I said, any other clean solution (and I pointed out 3) over this
hack.
Thanks,
tglx
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: KY Srinivasan @ 2011-02-21 14:40 UTC (permalink / raw)
To: Thomas Gleixner
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <alpine.LFD.2.00.1102211200270.2701@localhost6.localdomain6>
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Monday, February 21, 2011 6:03 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
>
> On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > > Like most virtualization platforms, Hyper-V also emulates the full PC
> > > > platform. So, it is possible that the driver of some other emulated
> > > > devices might register for the IRQ line we might have selected. That
> > > > is the race this code addresses. For performance reasons, we want
> > > > both storage and network traffic to go over the PV drivers.
> > >
> > > So in case your driver gets the interrupt line first, which the other
> > > driver wants to acquire as well, then what? Do you want to do that
> > > probe magic in the other driver as well? What if this is a regular
> > > device driver which gets its irq number from ACPI/PCI or
> > > whatever. Then that driver simply wont work as it's interrupt line is
> > > busy.
> > >
> > > > >
> > > > > I don't know why the previous reviewer wanted to have that
> > > > > dynamic. That just does not make sense to me.
> > > >
> > > > Prior to this patch, we had a hard coded interrupt line for use by
> > > > this driver. If that line was already in use, the load of this driver
> > > > would fail. This would be a fatal issue especially for distributions
> > > > that have embedded these PV drivers as part of their installation
> > > > media. This patch deals with such collisions in a more graceful way -
> > > > we would not bail until we have scanned all low interrupt lines.
> > >
> > > So you trade breaking the PV stuff against breaking random other
> > > drivers? That doesn't sound like a brilliant idea.
> > >
> > > There are various ways to solve that proper.
> > >
> > > - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > > provides as enumeration.
> > >
> > > - Use a fixed vector like XEN does for the event channel
> > >
> > > - Use dynamic allocation in the IOAPIC space like the kernel does for
> > > MSI(-X)
> > >
> > > Thanks,
> > >
> > > tglx
> >
> > I am not claiming that what I have done here is the best possible solution.
> > However, I will submit to you that it is better than what we had here
> > prior to this patch. I will address this and a whole lot of other issues
> > in future patches.
>
> No, it's _NOT_ better in any way. You trade breaking your PV thing
> against breaking random other drivers. Care to explain why you think
> that's better ?
The root device for the VM is bound to the PV driver on some distributions.
So, if we cannot load the PV drivers, we do not have a system that boots.
In general, the system performance without these PV drivers is so poor that
for all practical purposes, having the PV drivers is almost a requirement
for having a useable system. While the platform supports configuration of the VM with
some emulated devices, it is not a recommended configuration (because of
performance reasons) for Linux virtual machines on the Hyper-V platform.
I have spent significantly more time debating this patch than developing this patch
that I still think improves the current driver. I will leave it to Greg and other powers
that be to decide if this patch will be accepted. Let me know what your verdict is.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: Thomas Gleixner @ 2011-02-21 11:02 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E04801191C@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Mon, 21 Feb 2011, KY Srinivasan wrote:
> > > Like most virtualization platforms, Hyper-V also emulates the full PC
> > > platform. So, it is possible that the driver of some other emulated
> > > devices might register for the IRQ line we might have selected. That
> > > is the race this code addresses. For performance reasons, we want
> > > both storage and network traffic to go over the PV drivers.
> >
> > So in case your driver gets the interrupt line first, which the other
> > driver wants to acquire as well, then what? Do you want to do that
> > probe magic in the other driver as well? What if this is a regular
> > device driver which gets its irq number from ACPI/PCI or
> > whatever. Then that driver simply wont work as it's interrupt line is
> > busy.
> >
> > > >
> > > > I don't know why the previous reviewer wanted to have that
> > > > dynamic. That just does not make sense to me.
> > >
> > > Prior to this patch, we had a hard coded interrupt line for use by
> > > this driver. If that line was already in use, the load of this driver
> > > would fail. This would be a fatal issue especially for distributions
> > > that have embedded these PV drivers as part of their installation
> > > media. This patch deals with such collisions in a more graceful way -
> > > we would not bail until we have scanned all low interrupt lines.
> >
> > So you trade breaking the PV stuff against breaking random other
> > drivers? That doesn't sound like a brilliant idea.
> >
> > There are various ways to solve that proper.
> >
> > - You can provide the interrupt number from ACPI/PCI or whatever your HV
> > provides as enumeration.
> >
> > - Use a fixed vector like XEN does for the event channel
> >
> > - Use dynamic allocation in the IOAPIC space like the kernel does for
> > MSI(-X)
> >
> > Thanks,
> >
> > tglx
>
> I am not claiming that what I have done here is the best possible solution.
> However, I will submit to you that it is better than what we had here
> prior to this patch. I will address this and a whole lot of other issues
> in future patches.
No, it's _NOT_ better in any way. You trade breaking your PV thing
against breaking random other drivers. Care to explain why you think
that's better ?
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: Greg KH @ 2011-02-21 3:50 UTC (permalink / raw)
To: KY Srinivasan
Cc: Thomas Gleixner, gregkh@suse.de, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E04801191C@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Mon, Feb 21, 2011 at 03:43:58AM +0000, KY Srinivasan wrote:
> I am not claiming that what I have done here is the best possible solution.
> However, I will submit to you that it is better than what we had here
> prior to this patch. I will address this and a whole lot of other issues
> in future patches.
Care to document it in the TODO file so it doesn't get forgotten?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: KY Srinivasan @ 2011-02-21 3:43 UTC (permalink / raw)
To: Thomas Gleixner
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <alpine.LFD.2.00.1102201705380.2701@localhost6.localdomain6>
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Sunday, February 20, 2011 11:16 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
>
> On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > > When grabbing some random irq from the PIC is not an issue, then
> > > what's the point of this probing, retry loop and the comments about
> > > racing ? What races here? That does not make sense at all.
> >
> > Like most virtualization platforms, Hyper-V also emulates the full PC
> > platform. So, it is possible that the driver of some other emulated
> > devices might register for the IRQ line we might have selected. That
> > is the race this code addresses. For performance reasons, we want
> > both storage and network traffic to go over the PV drivers.
>
> So in case your driver gets the interrupt line first, which the other
> driver wants to acquire as well, then what? Do you want to do that
> probe magic in the other driver as well? What if this is a regular
> device driver which gets its irq number from ACPI/PCI or
> whatever. Then that driver simply wont work as it's interrupt line is
> busy.
>
> > >
> > > I don't know why the previous reviewer wanted to have that
> > > dynamic. That just does not make sense to me.
> >
> > Prior to this patch, we had a hard coded interrupt line for use by
> > this driver. If that line was already in use, the load of this driver
> > would fail. This would be a fatal issue especially for distributions
> > that have embedded these PV drivers as part of their installation
> > media. This patch deals with such collisions in a more graceful way -
> > we would not bail until we have scanned all low interrupt lines.
>
> So you trade breaking the PV stuff against breaking random other
> drivers? That doesn't sound like a brilliant idea.
>
> There are various ways to solve that proper.
>
> - You can provide the interrupt number from ACPI/PCI or whatever your HV
> provides as enumeration.
>
> - Use a fixed vector like XEN does for the event channel
>
> - Use dynamic allocation in the IOAPIC space like the kernel does for
> MSI(-X)
>
> Thanks,
>
> tglx
I am not claiming that what I have done here is the best possible solution.
However, I will submit to you that it is better than what we had here
prior to this patch. I will address this and a whole lot of other issues
in future patches.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: Thomas Gleixner @ 2011-02-20 16:15 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E048011787@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > When grabbing some random irq from the PIC is not an issue, then
> > what's the point of this probing, retry loop and the comments about
> > racing ? What races here? That does not make sense at all.
>
> Like most virtualization platforms, Hyper-V also emulates the full PC
> platform. So, it is possible that the driver of some other emulated
> devices might register for the IRQ line we might have selected. That
> is the race this code addresses. For performance reasons, we want
> both storage and network traffic to go over the PV drivers.
So in case your driver gets the interrupt line first, which the other
driver wants to acquire as well, then what? Do you want to do that
probe magic in the other driver as well? What if this is a regular
device driver which gets its irq number from ACPI/PCI or
whatever. Then that driver simply wont work as it's interrupt line is
busy.
> >
> > I don't know why the previous reviewer wanted to have that
> > dynamic. That just does not make sense to me.
>
> Prior to this patch, we had a hard coded interrupt line for use by
> this driver. If that line was already in use, the load of this driver
> would fail. This would be a fatal issue especially for distributions
> that have embedded these PV drivers as part of their installation
> media. This patch deals with such collisions in a more graceful way -
> we would not bail until we have scanned all low interrupt lines.
So you trade breaking the PV stuff against breaking random other
drivers? That doesn't sound like a brilliant idea.
There are various ways to solve that proper.
- You can provide the interrupt number from ACPI/PCI or whatever your HV
provides as enumeration.
- Use a fixed vector like XEN does for the event channel
- Use dynamic allocation in the IOAPIC space like the kernel does for
MSI(-X)
Thanks,
tglx
^ permalink raw reply
* RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
From: KY Srinivasan @ 2011-02-19 16:46 UTC (permalink / raw)
To: Thomas Gleixner
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Hank Janssen
In-Reply-To: <alpine.LFD.2.00.1102191540290.2701@localhost6.localdomain6>
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Saturday, February 19, 2011 10:12 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Hank
> Janssen
> Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
>
> On Sat, 19 Feb 2011, KY Srinivasan wrote:
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > Please do not use probe_irq_on for dynamic irq allocation. Highjacking
> > > the lower PIC irqs is really not a good idea. Depending on when this
> > > runs, you might grab an irq required by a driver which gets loaded
> > > later.
> > >
> > > Could you please explain what you're trying to do here ?
> >
> > The IRQ being allocated is for the VMBUS driver for Linux guests running on
> > a Windows virtualization platform (Hyper-V hypervisor).
> > The hypervisor is capable of notifying events on the VMBUS via
> > a guest specified interrupt line. Prior to this patch,
> > the code was statically selecting an interrupt line for
> > use by VMBUS. One of the long standing review comments
> > on that code was to make this irq allocation dynamic and that
> > is what this patch does. For the Linux guest running as a VM
> > on Hyper-V, the concern you raise is not an issue.
>
> That patch does a whole lot of useless crap.
>
> When grabbing some random irq from the PIC is not an issue, then
> what's the point of this probing, retry loop and the comments about
> racing ? What races here? That does not make sense at all.
Like most virtualization platforms, Hyper-V also emulates the full PC
platform. So, it is possible that the driver of some other emulated
devices might register for the IRQ line we might have selected. That
is the race this code addresses. For performance reasons, we want
both storage and network traffic to go over the PV drivers.
>
> I don't know why the previous reviewer wanted to have that
> dynamic. That just does not make sense to me.
Prior to this patch, we had a hard coded interrupt line for use by
this driver. If that line was already in use, the load of this driver
would fail. This would be a fatal issue especially for distributions
that have embedded these PV drivers as part of their installation
media. This patch deals with such collisions in a more graceful way -
we would not bail until we have scanned all low interrupt lines.
>
> Btw, that whole interrupt handler with two tasklets, one of them
> scheduling work is just screaming threaded interrupt handler.
We are in the process of cleaning up these drivers; I am looking at
some of these and other issues.
Regards,
K. Y
^ 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