Re: Bug inkvm_set_irq
Le 01/03/2011 08:03, Michael S. Tsirkin a écrit : On Mon, Feb 28, 2011 at 11:34:16PM +0100, Jean-Philippe Menil wrote: Hi, here is another trace with kvm.ko compiled with debug flags. the bug: [12099.503414] BUG: unable to handle kernel paging request at 0b6635e9 [12099.503462] IP: [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.503521] PGD 45d8d2067 PUD 45d58e067 PMD 0 [12099.503560] Oops: [#1] SMP [12099.503591] last sysfs file: /sys/devices/system/cpu/cpu11/cache/index2/shared_cpu_map [12099.503641] CPU 0 [12099.503648] Modules linked in: netconsole configfs vhost_net macvtap macvlan tun veth powernow_k8 mperf cpufreq_userspace cpufreq_stats cpufreq_powersave cpufreq_ondemand freq_table cpufreq_conservative fuse xt_physdev ip6t_LOG ip6table_filter ip6_tables ipt_LOG xt_multiport xt_limit xt_tcpudp xt_state iptable_filter ip_tables x_tables nf_conntrack_tftp nf_conntrack_ftp nf_conntrack_ipv4 nf_defrag_ipv4 8021q bridge stp ext2 mbcache dm_round_robin dm_multipath nf_conntrack_ipv6 nf_conntrack nf_defrag_ipv6 kvm_amd kvm ipv6 snd_pcm snd_timer snd soundcore snd_page_alloc shpchp pci_hotplug tpm_tis i2c_nforce2 tpm i2c_core pcspkr evdev psmouse joydev tpm_bios processor ghes dcdbas hed button serio_raw thermal_sys xfs exportfs dm_mod sg sr_mod cdrom usbhid hid usb_storage ses sd_mod enclosure megaraid_sas ohci_hcd lpfc scsi_transport_fc bnx2 scsi_tgt scsi_mod ehci_hcd [last unloaded: scsi_wait_scan] [12099.504277] [12099.504302] Pid: 1742, comm: kworker/0:2 Not tainted 2.6.37.2-dsiun-110105+ #2 Dell Inc. PowerEdge M605/0K543T [12099.504373] RIP: 0010:[a03ee877] [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.50] RSP: 0018:88045e013d00 EFLAGS: 00010246 [12099.504474] RAX: 0b6634c1 RBX: 0018 RCX: 0001 [12099.504508] RDX: RSI: RDI: 880419b600c0 [12099.504541] RBP: 88045e013dd0 R08: 88045e012000 R09: [12099.504575] R10: R11: R12: 880419b600c0 [12099.504609] R13: 880419b600c0 R14: a03efaa0 R15: 0001 [12099.504643] FS: 7f3abaa05710() GS:88007f80() knlGS: [12099.504693] CS: 0010 DS: ES: CR0: 8005003b [12099.504724] CR2: 0b6635e9 CR3: 00045e2bc000 CR4: 06f0 [12099.504757] DR0: DR1: DR2: [12099.504791] DR3: DR6: 0ff0 DR7: 0400 [12099.504825] Process kworker/0:2 (pid: 1742, threadinfo ffkvm_set_irqff88045e012000, task 88045ffb0d60) [12099.504874] Stack: [12099.504897] 000119c0 000119c0 000119c0 88045ffb0d60 [12099.504953] 88045ffb1010 88045e013fd8 88045ffb1018 88045e012010 [12099.505009] 000119c0 88045e013fd8 000119c0 000119c0 [12099.505065] Call Trace: [12099.505099] [813818ce] ? common_interrupt+0xe/0x13 [12099.505145] [a03efaa0] ? irqfd_inject+0x0/0x50 [kvm] [12099.505145] [a03efaca] irqfd_inject+0x2a/0x50 [kvm] [12099.505145] [8106b7bb] process_one_work+0x11b/0x450 [12099.505145] [8106bf37] worker_thread+0x157/0x410 [12099.505145] [8103a569] ? __wake_up_common+0x59/0x90 [12099.505145] [8106bde0] ? worker_thread+0x0/0x410 [12099.505145] [8106f996] kthread+0x96/0xa0 [12099.505145] [81003c64] kernel_thread_helper+0x4/0x10 [12099.505145] [8106f900] ? kthread+0x0/0xa0 [12099.505145] [81003c60] ? kernel_thread_helper+0x0/0x10 [12099.505145] Code: 55 49 89 fd 41 54 53 89 d3 48 81 ec a8 00 00 00 8b 15 a6 75 03 00 89 b5 3c ff ff ff 85 d2 0f 85 d5 00 00 00 49 8b 85 58 24 00 003b 98 28 01 00 00 73 61 89 db 48 8b 84 d8 30 01 00 00 48 85 c0 [12099.505145] RIP [a03ee877] kvm_set_irq+0x37/0x140 [kvm] [12099.505145] RSP88045e013d00 [12099.505145] CR2: 0b6635e9 markup_oops result: root@ayrshire:~# cat bug.txt | perl markup_oops.pl -m /lib/modules/2.6.37.2-dsiun-110105+/kernel/arch/x86/kvm/kvm.ko /boot/vmlinuz-2.6.37.2-dsiun-110105+ vmaoffset = 18446744072103034880 a03ee841: 48 89 e5mov %rsp,%rbp a03ee844: 41 57 push %r15 a03ee846: 41 89 cfmov%ecx,%r15d | %r15 = 1 %ecx = 1 a03ee849: 41 56 push %r14| %r14 = a03efaa0 a03ee84b: 41 55 push %r13 a03ee84d: 49 89 fdmov%rdi,%r13 | %edi = 880419b600c0 %r13 = 880419b600c0 a03ee850: 41 54 push %r12| %r12 = 880419b600c0 a03ee852: 53 push %rbx a03ee853: 89 d3 mov%edx,%ebx | %ebx = 18 a03ee855: 48 81 ec a8 00 00 00sub$0xa8,%rsp a03ee85c:
RE: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:35 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
On Fri, Feb 25, 2011 at 06:05:30PM -0800, K. Y. Srinivasan wrote:
Hyper-V drivers have supported two device abstractions.
This patch implements a single device abstraction.
This patch or This patch series?
Greg, If you recall, last week I had sent a single patch that did what these 6
patches do.
While I agree, the description could have been more informative; I have tried
to
follow the guidelines I got in terms of what should go into each patch:
The first set of 3 patches (1 through 3) deal with the device related issues:
Patch 1: Combines the state in hv_device into vm_device to create a
single device abstraction called vm_device.
Patch 2: Changes the name of vm_device to hyperv_device.
Patch 3: Cleanup the names of variables that refer to hyperv_device
Next set of 3 patches (4 through 6) deal with the driver related issues:
Patch 4: Combines the state maintained in hv_driver into driver_context to
Create a single driver abstraction called driver_context.
Patch 5: Renames the driver context to hyperv_driver.
Patch 6: Cleanup the names of variables that refer to hyperv_driver.
I am beginning to wonder if perhaps I chose the wrong granularity in splitting
up these patches.
Regards,
K. Y
This simplifies the code and avoids duplication
of state.
This patch is confusing, you are renaming structures (from hv_ to vm_)
which I didn't think you wanted to do.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc.c| 17 ---
drivers/staging/hv/blkvsc_drv.c| 14 +++---
drivers/staging/hv/channel_mgmt.c |1 +
drivers/staging/hv/channel_mgmt.h |2 +-
drivers/staging/hv/netvsc.c| 55 ---
drivers/staging/hv/netvsc.h|2 +-
drivers/staging/hv/netvsc_api.h| 12 +++---
drivers/staging/hv/netvsc_drv.c| 28 +---
drivers/staging/hv/rndis_filter.c | 19
drivers/staging/hv/storvsc.c | 37
drivers/staging/hv/storvsc_api.h |4 +-
drivers/staging/hv/storvsc_drv.c | 21 -
drivers/staging/hv/vmbus.h | 13 +++---
drivers/staging/hv/vmbus_api.h | 29 ++--
drivers/staging/hv/vmbus_drv.c | 84
+++-
drivers/staging/hv/vmbus_private.h | 12 +++---
16 files changed, 157 insertions(+), 193 deletions(-)
diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c
index 7c8729b..ecface3 100644
--- a/drivers/staging/hv/blkvsc.c
+++ b/drivers/staging/hv/blkvsc.c
@@ -35,7 +35,8 @@ static const struct hv_guid g_blk_device_type = {
}
};
-static int blk_vsc_on_device_add(struct hv_device *device, void
*additional_info)
+static int blk_vsc_on_device_add(struct vm_device *device,
+ void *additional_info)
Huh? What was this change for? 80 column issues for function
definitions is not a big deal, if any, and should not be burried in a
patch that claims to do something else.
Still totally confused,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/6] Staging: hv: Rename vm_device to hyperv_device
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:33 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 2/6] Staging: hv: Rename vm_device to hyperv_device
On Fri, Feb 25, 2011 at 06:06:03PM -0800, K. Y. Srinivasan wrote:
Rename the vm_device abstraction as hyperv_device.
That's a nice name, but it's the first one with a hyperv_ prefix in
this subsystem. Ok, second, but hyperv_service_context doesn't really
count as it's not really used.
Everything else is named hv_ here. Which is it going to be, hv_ or
hyperv_?
Either is fine, just be aware of the work involved if you pick
hyperv_...
As far as the device state and driver state in this subsystem is concerned;
I thought the consensus was to name them hyperv_*
You also rename vm_device_info to hyperv_device_info here, doing more
than one thing in a single patch.
Please don't do that, this should be 2 patches.
I changed all device related state to be consistently named. However,
I will break it up into 2 patches.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc.c|4 +-
drivers/staging/hv/blkvsc_drv.c|8 ++--
drivers/staging/hv/channel_mgmt.h |2 +-
drivers/staging/hv/netvsc.c| 56
+++
drivers/staging/hv/netvsc.h|2 +-
drivers/staging/hv/netvsc_api.h| 12
drivers/staging/hv/netvsc_drv.c| 14
drivers/staging/hv/rndis_filter.c | 18 ++--
drivers/staging/hv/storvsc.c | 36 ---
drivers/staging/hv/storvsc_api.h |4 +-
drivers/staging/hv/storvsc_drv.c | 10 +++---
drivers/staging/hv/vmbus.h |6 ++--
drivers/staging/hv/vmbus_api.h |8 ++--
drivers/staging/hv/vmbus_drv.c | 50
drivers/staging/hv/vmbus_private.h | 12
15 files changed, 124 insertions(+), 118 deletions(-)
diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c
index ecface3..47ccec2 100644
--- a/drivers/staging/hv/blkvsc.c
+++ b/drivers/staging/hv/blkvsc.c
@@ -35,8 +35,8 @@ static const struct hv_guid g_blk_device_type = {
}
};
-static int blk_vsc_on_device_add(struct vm_device *device,
- void *additional_info)
+static int
+blk_vsc_on_device_add(struct hyperv_device *device, void *additional_info)
Ick, why break the formatting like this? Please keep the return type on
the same line as the function name.
You do that in a number of places, please don't.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:44 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
On Fri, Feb 25, 2011 at 06:06:32PM -0800, K. Y. Srinivasan wrote:
Cleanup the names of variables that refer to the
hyperv_device abstraction.
Clean them up to be what? Shorter? Nice? Full of rounded edges so
that when we bump into them in the dark they don't poke us and cause us
to shreak in pain?
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Sweet, you cloned yourself, I thought only Alan Cox had achieved that
goal...
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc_drv.c | 12 ++--
drivers/staging/hv/netvsc.c |4 +-
drivers/staging/hv/netvsc_drv.c | 36
drivers/staging/hv/storvsc_drv.c | 44 +-
drivers/staging/hv/vmbus_drv.c | 164
+++-
--
5 files changed, 130 insertions(+), 130 deletions(-)
diff --git a/drivers/staging/hv/blkvsc_drv.c
b/drivers/staging/hv/blkvsc_drv.c
index 58ab0e8..305a665 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -95,7 +95,7 @@ struct blkvsc_request {
/* Per device structure */
struct block_device_context {
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
Hey, I was right, it does have more rounded edges, nicely done.
-static int netvsc_device_add(struct hyperv_device *device,
- void *additional_info);
+static int
+netvsc_device_add(struct hyperv_device *device, void *additional_info);
Again with the function return value hiding. Please don't.
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -43,7 +43,7 @@ struct host_device_context {
/* must be 1st field
* FIXME this is a bug */
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
I really don't understand this change at all. obj is just as vapid
and clueless as ctx is, and it seems very gratuitous to change this.
And I should know, I have made a lot of gratuitous renames in my time in
the kernel...
Greg, there are not that many options here. As I recall there was universal
objection to the use of *context/*ctx to refer to device or driver objects.
The name I chose is fairly descriptive of what it represents. If there is
consensus
on a better name, I will use it.
static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
{
- struct hyperv_device *device_ctx = device_to_hyperv_device(device);
+ struct hyperv_device *device_obj = device_to_hyperv_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]);
+ device_obj-class_id.data[3], device_obj-class_id.data[2],
+ device_obj-class_id.data[1], device_obj-class_id.data[0],
+ device_obj-class_id.data[5], device_obj-class_id.data[4],
+ device_obj-class_id.data[7], device_obj-class_id.data[6],
+ device_obj-class_id.data[8], device_obj-class_id.data[9],
+ device_obj-class_id.data[10],
+ device_obj-class_id.data[11],
+ device_obj-class_id.data[12],
+ device_obj-class_id.data[13],
+ device_obj-class_id.data[14],
+ device_obj-class_id.data[15]);
After seeing this stuff so many times I'm waiting for a helper function
to be added for it in this subsystem. I'm sure you really don't want to
edit GUID printk-like functions ever again, right?
I will have a separate patch for this.
static void vmbus_device_release(struct device *device)
{
- struct
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. Greg, would it be better if I just had one patch for dealing with all device related issues and a Separate patch for dealing all driver related issues? Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:59 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names On Fri, Feb 25, 2011 at 06:07:58PM -0800, K. Y. Srinivasan wrote: The title says it all. That's a horrible changelog comment. So bad that I would rather see an empty message than this one. Seriously, it give no description, and makes us think that the whole patch is obvious, when it really isn't. What did you change them to? What did you change them from? What was your motivation in changing them? How were you feeling when the names changed? Ok, maybe not the last one, but you get the idea. Greg, these changes (patches 1 through 6) change so much in this sub-system that until these changes go in, our cleanup efforts are stalled. That is the main reason I was so hasty in submitting these patches. Clearly, I need to provide a better changelog comment; and I will. Looking at your other comments, I am wondering if the granularity I chose for breaking up the changes that had to be done is also a significant part of the problem. If it is ok with you, I could generate a patch that deals with all device related issues and a patch that deals with all driver related issues. These patches obviously will do more than one thing (however they will all be related); but at least they won't have intermediate state that would be objectionable. Let me know. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
On Wed, Mar 02, 2011 at 01:42:37AM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:44 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
On Fri, Feb 25, 2011 at 06:06:32PM -0800, K. Y. Srinivasan wrote:
Cleanup the names of variables that refer to the
hyperv_device abstraction.
Clean them up to be what? Shorter? Nice? Full of rounded edges so
that when we bump into them in the dark they don't poke us and cause us
to shreak in pain?
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Sweet, you cloned yourself, I thought only Alan Cox had achieved that
goal...
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc_drv.c | 12 ++--
drivers/staging/hv/netvsc.c |4 +-
drivers/staging/hv/netvsc_drv.c | 36
drivers/staging/hv/storvsc_drv.c | 44 +-
drivers/staging/hv/vmbus_drv.c | 164
+++-
--
5 files changed, 130 insertions(+), 130 deletions(-)
diff --git a/drivers/staging/hv/blkvsc_drv.c
b/drivers/staging/hv/blkvsc_drv.c
index 58ab0e8..305a665 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -95,7 +95,7 @@ struct blkvsc_request {
/* Per device structure */
struct block_device_context {
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
Hey, I was right, it does have more rounded edges, nicely done.
-static int netvsc_device_add(struct hyperv_device *device,
- void *additional_info);
+static int
+netvsc_device_add(struct hyperv_device *device, void *additional_info);
Again with the function return value hiding. Please don't.
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -43,7 +43,7 @@ struct host_device_context {
/* must be 1st field
* FIXME this is a bug */
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
I really don't understand this change at all. obj is just as vapid
and clueless as ctx is, and it seems very gratuitous to change this.
And I should know, I have made a lot of gratuitous renames in my time in
the kernel...
Greg, there are not that many options here. As I recall there was universal
objection to the use of *context/*ctx to refer to device or driver objects.
The name I chose is fairly descriptive of what it represents. If there is
consensus
on a better name, I will use it.
Do it like other subsystems, call it a device with the prefix for the
type. So for this one, it would be:
struct hyperv_device *hyperv_device;
or
struct hyperv_device *hyperv_dev;
Come on, global search-and-replace needs to be done in a sane manner,
other wise you can just send me a vi macro to run on the code, it would
be the same thing in the end (hint, don't do that, only one person has
ever gotten away with doing that in the history of the kernel, in an act
never to be ever repeated again.)
Greg, apart from your objection to the name I picked to refer to variable
referring to struct hyperv_device; what else is the problem here.
Changing the comment that should be removed instead.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
On Wed, Mar 02, 2011 at 01:43:13AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. Greg, would it be better if I just had one patch for dealing with all device related issues and a Separate patch for dealing all driver related issues? Again, only do one thing per patch. So yes, of course they should be separate. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names
On Wed, Mar 02, 2011 at 01:44:11AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:59 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names On Fri, Feb 25, 2011 at 06:07:58PM -0800, K. Y. Srinivasan wrote: The title says it all. That's a horrible changelog comment. So bad that I would rather see an empty message than this one. Seriously, it give no description, and makes us think that the whole patch is obvious, when it really isn't. What did you change them to? What did you change them from? What was your motivation in changing them? How were you feeling when the names changed? Ok, maybe not the last one, but you get the idea. Greg, these changes (patches 1 through 6) change so much in this sub-system that until these changes go in, our cleanup efforts are stalled. Ok, but that's not my issue, it's yours :) And what's the rush? That is the main reason I was so hasty in submitting these patches. Clearly, I need to provide a better changelog comment; and I will. Looking at your other comments, I am wondering if the granularity I chose for breaking up the changes that had to be done is also a significant part of the problem. If it is ok with you, I could generate a patch that deals with all device related issues and a patch that deals with all driver related issues. These patches obviously will do more than one thing (however they will all be related); but at least they won't have intermediate state that would be objectionable. Let me know. Ick, no, you need to break these up into smaller pieces than just the 6 you did here, as you were still doing more than one thing per patch. Please make them simpler, not more complex. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
The problem is that everyone reading [patch 1/6] thinks you're renaming hv_device to vm_device or introducing a new struct vm_device. That makes people annoyed. If you had written the patch description like this: In the original code, the structs vm_device included a struct hv_device. This patch moves the members from hv_device directly into struct vm_device. The members -dev_type and -dev_instance from hv_device were the same as -class_id and -device_id in vm_device so those were not copied over. Now that everything is included into vm_device directly, hv_device is unused and we can delete the definition. There still might be issues with the patch, but at least you would be talking about the same thing. regards, dan carpenter ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
