Does this site look plain?

This site uses advanced css techniques

In July 2009, Hell froze over and Microsoft contributed drivers to the Linux kernel under GPL in order to allow Linux to host better in their Hyper-V virtual machine.

Date Mon, 20 Jul 2009 09:00:25 -0700 From: Greg Kroah-Hartman Subject: [patch 00/54] [Announce] Microsoft Hyper-V drivers for Linux Hi all, I'm happy to announce, that after many months of discussions, Microsoft has released their Hyper-V Linux drivers under the GPLv2. Following this message, will be the patches that add the drivers to the drivers/staging/ tree, and a whole bunch of cleanups. ... thanks, greg k-h

It's been reported that it's taken a lot of work ("more than 200 patches") to bring the drivers into a state where they can be merged into the kernel, and some suggest that this weighs into the competence of the Microsoft engineers.

It may, or it may not: Greg has commented that the patches are about style — harmonizing the submitted code to have the same coding style as the rest of the project — and I wanted to find out for myself.

I searched for all 212 patches and have included them here as links, which should allow any competent engineer to assess them easily.

Some disclaimers:

Why so many patches?

This is the first thing one notices when looking at how this code was subsumed into the project, and it's an entirely fair question: though all new drivers take work to integrate, this required 212: Greg K-H called this excessive.

Some might conclude that it was simply poor code quality that lead to so many patches, but I don't believe it's the case even though I'd not have submitted the driver in the current state. There are several contributing factors to the large patch count.

The main reason is that the driver was written with a Windows API dialect, which certainly matches the dialect of the other side of the Hyper-V code (the part that this driver talks to).

Coding style is mainly about consistency, and there isn't any one right style: it's more important that everybody on a project agree with how the code should look than what the particular formatting rules are.

In the Windows dialect, it's common to typedef essentially everything, and to rely on some common types universally defined by the Windows SDK. Everybody who's done Win32 programming knows about DWORD (32-bit unsigned integer), PVOID (void *), and HANDLE (opaque handle to a resource), among many others.

This is done with structs defined by the project as well, with things like this (chosen at random): 

typedef struct _VMBUS_CHANNEL_MESSAGE_HEADER
{
    VMBUS_CHANNEL_MESSAGE_TYPE  MessageType;
    UINT32                      Padding;
} VMBUS_CHANNEL_MESSAGE_HEADER, *PVMBUS_CHANNEL_MESSAGE_HEADER;

This defines the types VMBUS_CHANNEL_MESSAGE_HEADER and PVMBUS_CHANNEL_MESSAGE_HEADER, and though it looks awkward and busy for a Linux developer, this is the Win32 style: it's simply the way it's been done for at least 20 years. I don't care for it much either, but it is what it is.

The Linux style prefers not to hide type information, so it would be changed to: 

struct vmbus_channel_message_header
{
    enum vmbus_channel_message_type MessageType;
    u32 Padding;
}

I saw numerous comments on the Linux kernel mailing list chastising Microsoft for such style, but this chastisement is ignorant.

It's more than fair to say that though the style may be appropriate for Windows drivers, it doesn't belong in the Linux kernel (and I mostly agree with this), but to call the style wrong outright reflects a lack of cross-platform experience.

And even if one decides that the Win32 style is inappropriate — as I do — the overarching decision to do it this way is what one takes issue with, not the number of patches it takes to harmonize the styles.

The second reason why there were so many patches was the careful, methodical approach Greg used in making his changes. Rather than just run through and fix everything at once, he changed essentially one thing at a time across the whole codebase, and submitted a patch for it.

This is exactly the way to make these changes: it makes the patches far easier to review, and to unwind in case of a mistake. This process was repeated, slowly and methodically, for each of the driver-defined typedefs, and this adds up to a lot of patches for relatively small changes (and none of these changes had anything to do with actual code functionality).

Other patches were to remove some wrapper functions that put a Win32 face on Linux kernel functions: 

void* MemAllocZeroed(unsigned int size)
{
	void *p = kmalloc(size, GFP_KERNEL);
	if (p) memset(p, 0, size);
	return p;
}

Calls to this function were replaced with kzalloc(size, GFP_KERNEL), and there were maybe a dozen patches for these simple wrappers. Greg repeatedly made comments on the lines of "make it a real lock so that we know what is going on".

And of course he's right: though a wrapper function may be helpful for somebody coming from a Win32 background, all it does is confuse a real Linux driver developer. The existing set of functions in the kernel form a vocabulary that everybody uses, and when one sees (for instance) kzalloc, one knows that it's a zero-fill kernel memory allocation.

But when one sees MemAllocZeroed, one will assume that it does something different: after all, why give a different name for something everybody already understands? These patches to remove the wrappers were entirely appropriate.

A fair number of patches were little more than moving the whitespace around.

A few of the patches fixed outright bugs, or code that was undoubtedly sloppy, but I didn't see anything that looked like a show stopper or was indicative of underlying poor development.

Assessment

Ultimately, I believe that the driver should have been converted to the Linux dialect before submission to the project, as this is the way to best make it accepted. There's no real upside in letting the world draw such negative inferences about Microsoft's developers.

But I can think of three plausible reasons for having kept it as-is, and though I am not convinced by them, they're worth considering:

  1. Since this driver is talking to Windows code on the other side of the API call, there's a benefit in at least the interface code — certainly the structures that pass back and forth — be defined identically on each side.
  2. The developers most likely to refine this code's functionality are the ones who wrote it, and there's some benefit to keeping it in a style they are most familiar with.

  3. The folks that wrote this are certainly domain experts in Hyper-V, but probably less so in the Linux dialect. It's not foolish to suggest that their time is better spent on the areas where they truly are expert — Hyper-V stuff — leaving the Linuxification to those who can do it correctly even while sleeping and/or drinking.

    In fact, the very purpose of the staging tree is to get code into proper style shape before being considered for the main tree.

In the end, I think it would have been best for Microsoft to find a developer who's truly experienced with the Linux kernel dialect to massage the the source before turning it over to the project.

I know that I could have made around a quarter of these changes easily — moving away from the Win32 types, dumping the wrappers, other mechanical changes — as these are routine tasks in migrating codebases. The ones that required a real knowledge of the kernel (interrupts, spinlocks) I'd not have been qualified to fool with.

I dare say — putting on our fantasy hat now — that if Greg K-H had submitted a large body of Linux code to a Windows project, that code would go through essentially the same harmonization process in reverse.

The patches

There were two sets of patches: first were some gradual ones in batches over time from July to September, and then one big set of them submitted as part of a much larger project to promote stuff to a final staging build. This is why the patches here run from 145-out-of-641 to 355-out-of-641: patches outside this range were not about the hv driver.

There were some discussions on the earlier patches about the best approaches to take, including a few erroneous ones, so I chose to evaluate the resulting patches that survived that vetting process.

I'll note that Greg was clear that this was a coding style pass, and though a few minor bugs were fixed, this did not really dig into the overall logic of the driver. I'm sure that at some level it works, but for all we know it may not be threadsafe, deal badly with memory, or commit other sins: these would not be found during this review effort.

Greg's patch comments are shown in a standout format, and the unadorned commentary is mine. Note that sometimes the patch comments were actually made by others (Greg just did the submission), but I didn't split this out.

I welcome feedback on possible mischaracterizations found here.

Update: as of 2019, these links to the individual patches are broken and I did not bother to track down updated ones - it's not clear there's any point in it a decade later.

[PATCH 145/641] Staging: hv: add the Hyper-V api header files
GKH These are the header files for the API to talk to the Hyper-V core.
[PATCH 146/641] Staging: hv: add the Hyper-V driver header files
GKH These are the header files for the different Linux Hyper-V drivers to use.
[PATCH 147/641] Staging: hv: add the Hyper-V virtual bus
GKH This is the virtual bus that all of the Linux Hyper-V drivers use.
[PATCH 148/641] Staging: hv: add the Hyper-V virtual block driver
GKH This is the virtual block driver when running Linux on top of Hyper-V.
Virtual block driver supports hard drives, CDs, DVDs.
[PATCH 149/641] Staging: hv: add the Hyper-V virtual network driver
GKH This is the virtual network driver when running Linux on top of Hyper-V.
Let the Linux kernel "see" the network adapter in an optimized way.
[PATCH 150/641] Staging: hv: add the Hyper-V virtual storage driver
GKH This is the virtual storage driver when running Linux on top of Hyper-V.
Virtual Storage looks like it emulates SCSI.
[PATCH 151/641] Staging: hv: add a TODO file
GKH First cut at what needs to be done to this codebase.
The TODO contains:
  • fix checkpatch warnings/errors
  • fix sparse issues
  • remove compatibility layer
  • fix HANDLE usage to be "real" pointers
  • audit the vmbus to verify it is working properly with the driver model
  • see if the vmbus can be merged with the other virtual busses in the kernel
  • audit the network driver
  • audit the block driver
  • audit the scsi driver
[PATCH 152/641] Staging: hv: make the Hyper-V virtual bus code build
GKH

The #define KERNEL_2_6_27 needs to be set, and I adjusted the include directories a bit to get things to build properly.

I also fixed up the direct access of bus_id, as that field is now gone.

The hv_vmbus code should now build properly, with no errors.

Looks like it moved some #include files around and changes how the bus ID name is assigned: the use of dev_set_name() appears to be new as of 2.6.27. 
-	sprintf(dev_ctx->device.bus_id, "vmbus_0_0");
+	dev_set_name(&dev_ctx->device, "vmbus_0_0");
So this is just version skew, but who uses sprintf() for a simple string copy?
[PATCH 153/641] Staging: hv: use the correct #ifdef for x86-64
GKH x86-64 needs a different config check. Thanks to Hank for the debugging to determine the fix for this.
Hank = Hank Janssen, Microsoft engineer
[PATCH 154/641] Staging: hv: add the Hyper-V virtual bus to the build
GKH Add the Hyper-V virtual bus to the kernel build system.
Nothing builds unless you tell it where to find the files!
[PATCH 155/641] Staging: hv: make the Hyper-V virtual storage driver build
GKH

The #define KERNEL_2_6_27 needs to be set, and I adjusted the include directories a bit to get things to build properly.

I also fixed up the direct access of bus_id, as that field is now gone. Some minor scsi api changes were needed as well.

The hv_storvsc code should now build properly, with no errors.

[PATCH 156/641] Staging: hv: add the Hyper-V virtual scsi driver to the build
GKH Add the Hyper-V virtual scsi driver to the kernel build system.
.. added to the Makefile
[PATCH 157/641] Staging: hv: storvsc: fix up driver_data usage
GKH driver_data is gone now from struct device, so use the proper functions to access it instead.
I see the ->driver_data field in struct device in the 2.6.25.8 kernel source, so it must have been removed pretty recently (staging is now on 2.6.27?). This replaces the direct access with a function providing a more general (safer?) mechanism. 
-	struct Scsi_Host *host = (struct Scsi_Host *)device->driver_data;
+	struct Scsi_Host *host = dev_get_drvdata(device);
I suspect that Microsoft had built this driver on an older kernel version, so this is just a bit of version skew.
[PATCH 158/641] Staging: hv: make the Hyper-V virtual block driver build
GKH

The #define KERNEL_2_6_27 needs to be set, and I adjusted the include directories a bit to get things to build properly.

I also fixed up the direct access of bus_id, as that field is now gone. Lots of block api changes were needed, and I don't think I got it all correct. It would be great of someone who knows the block api better could review it.

The hv_blkvsc code should now build, with no errors.

Mods for the 2.6.27 kernel API changes.
[PATCH 159/641] Staging: hv: add the Hyper-V virtual block driver to the build
GKH Add the Hyper-V virtual block driver to the kernel build system.
... added to the Makefile and KConfig.
[PATCH 160/641] Staging: hv: blkvsc: fix up driver_data usage
GKH driver_data is gone now from struct device, so use the proper functions to access it instead. Thanks to Bill Pemberton for pointing out this build error.
Changes for the 2.6.27 kernel API changes.
[PATCH 161/641] Staging: hv: make the Hyper-V virtual network driver build
GKH The #define KERNEL_2_6_27 needs to be set, and I adjusted the include directories a bit to get things to build properly. The driver was changed to use net_device_ops, as that is needed to build and operate properly now. The hv_netvsc code should now build with no errors.
2.6.27 API changes
[PATCH 162/641] Staging: hv: add the Hyper-V virtual network driver to the build
GKH Add the Hyper-V virtual network driver to the kernel build system.
... added to the Makefile and KConfig
[PATCH 163/641] Staging: hv: netvsc: fix up driver_data usage
GKH driver_data is gone now from struct device, so use the proper functions to access it instead. Thanks to Bill Pemberton for pointing out this build error.
Changes for the 2.6.27 kernel API changes.
[PATCH 164/641] Staging: hv: remove INTERNAL typedef
GKH The INTERNAL typedef is now removed from the Hyper-V driver code.
Microsoft defined a macro (not a typedef) for INTERNAL to static, and this just removed that macro and used static directly where appropriate.
[PATCH 165/641] Staging: hv: remove PVOID typedef
GKH The PVOID typedef is now removed from the Hyper-V driver code.
PVOID was a typedef for void* (a void pointer), and the typedef was replaced with the direct void*.
[PATCH 166/641] Staging: hv: remove VOID typedef
GKH The VOID typedef is now removed from the Hyper-V driver code.
VOID was a typedef for void, and the typedef was replaced with the direct void.
[PATCH 167/641] Staging: hv: remove UINT8 and INT8 typedefs
GKH The UINT8 and INT8 typedefs are now removed from the Hyper-V driver code.
UINT8 and INT8 are Win32 typedefs for unsigned and signed 8-bit values, and they were replaced with the more Linux-familiar i8 and u8, then the original typedefs removed.
[PATCH 168/641] Staging: hv: remove UINT16 and INT16 typedefs
GKH The UINT16 and INT16 typedefs are now removed from the Hyper-V driver code.
As above, for 16-bit values.
[PATCH 169/641] Staging: hv: remove UINT32 and INT32 typedefs
GKH The UINT32 and INT32 typedefs are now removed from the Hyper-V driver code.
As above, for 32-bit values.
[PATCH 170/641] Staging: hv: remove UINT64 and INT64 and UCHAR typedefs
GKH The UINT64 and INT64 and UCHAR typedefs are now removed from the Hyper-V driver code.
As above, for 64-bit values, plus unsigned char.
[PATCH 171/641] Staging: hv: remove USHORT typedef
GKH The USHORT typedef is now removed from the Hyper-V driver code.
Changed typedef USHORT to unsigned short, then deleted the typedef.
[PATCH 172/641] Staging: hv: remove ULONGLONG and LONGLONG typedefs
GKH The ULONGLONG and LONGLONG typedefs are now removed from the Hyper-V driver code.
As above, with ULONGLONG » unsigned long long and LONGLONG»long long.
[PATCH 173/641] Staging: hv: remove ULONG_PTR typedef
GKH The ULONG_PTR typedef is now removed from the Hyper-V driver code.
Curiously, ULONG_PTR is really just unsigned long (not a pointer!)
[PATCH 174/641] Staging: hv: remove ULONG and LONG typedefs
GKH The ULONG and LONG typedefs are now removed from the Hyper-V driver code.
Changed ULONG»unsigned int and LONG»int.
[PATCH 175/641] Staging: hv: remove SIZE_T typedef
GKH The SIZE_T typedef is now removed from the Hyper-V driver code.
Changed SIZE_T to size_t.
[PATCH 176/641] Staging: hv: remove DWORD and BYTE typedefs
GKH No one was even using them...
DWORD and BYTE are common Win32 API typedefs.
[PATCH 177/641] Staging: hv: remove BOOL and BOOLEAN typedefs
GKH The BOOL and BOOLEAN typedefs are now removed from the Hyper-V driver code.
Both BOOL and BOOLEAN have been replaced with bool. It seems kinda sloppy to have two different ones, but this might be part of a legitimate Win32 API difference.
[PATCH 178/641] Staging: hv: remove #defines from osd.c
GKH Remove the unneeded #defines from osd.c
This looks like version skew: the code has provisions for certain older API features, but since this is going into the 2.6.27 tree, there was no need for this if-old-else-new kind of stuff.
I don't fully understand this.
[PATCH 179/641] Staging: hv: remove MIN and MAX usages
GKH The kernel has the "correct" min() and max() functions, so use those.
Microsoft defined macros MIN() and MAX(): 
-#define MIN(a, b)       ((a) < (b)? (a): (b))
-#define MAX(a, b)       ((a) > (b)? (a): (b))
These are correct only if the arguments to these macros have no side effects — which is the case here — but it's considered bad form because it's easy to find a surprise. The right way to do this is with an inline function that employs proper side-effect semantics.
[PATCH 180/641] Staging: hv: remove PAGE_SIZE and PAGE_SHIFT and __builtin functions
GKH The kernel provides all of this, and actually gets it correct, so don't try to redefine these types of things.
One of the header files defined PAGE_SIZE and PAGE_SHIFT macros, and without knowing anything about how it's supposed to work, it looks really wrong to me: 

-#ifndef PAGE_SIZE
-#define PAGE_SIZE 0x1000
-#endif
-
-#ifndef PAGE_SHIFT
-#define PAGE_SHIFT 12
-#endif
This smells sloppy to me: let the operating system define these because the config/build system will insure that it's right. Getting virtual memory pagesize stuff wrong is catastrophic.
[PATCH 181/641] Staging: hv: remove STRUCT_PACKED and STRUCT_ALIGNED defines
GKH Use the correct __attribute__((packed)) one if it's really needed.
This just removes macros that can be applied to a struct to force alignment or tight packing: 
-#define STRUCT_PACKED		__attribute__((__packed__))
-#define STRUCT_ALIGNED(x)	__attribute__((__aligned__(x)))
This patch adjusts the code to apply the __attribute__ tag explicitly.
[PATCH 182/641] Staging: hv: remove UNUSED_VAR usage
GKH This isn't needed at all.
I'm not sure I agree with this assessment. Microsoft defined a macro that tagged the given variable as undefined, so it serves to tell both the compiler and the reader about this: 
#define UNUSED_VAR(v)		v  __attribute__((__unused__))

...

static void netvsc_set_multicast_list(UNUSED_VAR(struct net_device *net))
{
}
This kind of thing — tagging of attributes — is a best practice, and I don't know why it would be removed.
[PATCH 183/641] Staging: hv: remove FIELD_OFFSET usage
GKH This isn't needed, or even used, at all.
Microsoft included a macro FIELD_OFFSET() (which is provided on Windows in <WINNT.h>) that looks suspiciously like the ANSI C offsetof(), so even if it were used it would suggest that it should use the standard macro.
[PATCH 184/641] Staging: hv: remove TRUE, FALSE, and NULL usage
GKH Don't define things that are either already provided (like NULL), or you shouldn't use (like TRUE and FALSE).
I completely agree.
[PATCH 185/641] Staging: hv: osd: remove MemAlloc wrapper
GKH Use the "real" kmalloc call instead of a wrapper function.
Replace MemAlloc() with calls to kmalloc() directly.
[PATCH 186/641] Staging: hv: osd: remove MemAllocZeroed wrapper
GKH Use the "real" kzmalloc call instead of a wrapper function.
Replace MemAllocZeroed() with calls to kzmalloc() directly.
[PATCH 187/641] Staging: hv: osd: remove MemAllocAtomic wrapper
GKH Use the "real" kmalloc call instead of a wrapper function.
Replace use of MemAllocAtomic() with use of kzalloc(..., GFP_ATOMIC).
[PATCH 188/641] Staging: hv: osd: remove MemFree wrapper
GKH Use the "real" kfree call instead of a wrapper function.
[PATCH 189/641] Staging: hv: make Channel->InboundLock a real spinlock
GKH

Don't use the wrapper functions for this lock, make it a real lock so that we know what is going on.

I don't think we really want to be doing a irqsave for this code, but I left it alone to preserve the original codepath. It should be reviewed later.

[PATCH 190/641] Staging: hv: make RingInfo->RingLock a real spinlock
ditto the previous comment
[PATCH 191/641] Staging: hv: make Device->RequestLock a real spinlock
ditto the previous comment
[PATCH 192/641] Staging: hv: make netDevice->ReceivePacketListLock a real spinlock
ditto the previous comment
[PATCH 193/641] Staging: hv: make gVmbusConnection.ChannelMsgLock a real spinlock
ditto the previous comment
[PATCH 194/641] Staging: hv: make gVmbusConnection.ChannelLock a real spinlock
ditto the previous comment
[PATCH 195/641] Staging: hv: osd: remove spinlock wrapper functions
GKH Now that there are no users of the wrapper functions for spinlocks, remove them.
[PATCH 196/641] Staging: hv: osd: remove Sleep wrapper
GKH Use the "real" udelay call instead of a wrapper function.
Change Sleep(msecs) — a Win32 API function — with udelay(msecs), then dump the Sleep function.
[PATCH 197/641] Staging: hv: osd: remove MemoryFence wrapper
GKH Use the "real" mb call instead of a wrapper function.
Use mb() instead of the MemoryFence() wrapper.
[PATCH 198/641] Staging: hv: osd: remove LogMsg wrapper
GKH Use the "real" printk call instead of a wrapper function.
The printk() call is used for logging kernel message, and it's being used instead of the LogMsg() wrapper.
[PATCH 199/641] Staging: hv: osd: remove PrintBytes wrapper
GKH Use the "real" print_hex_dump_bytes call instead of a wrapper function.
[PATCH 200/641] Staging: hv: fix up printk warnings
GKH After LogMsg was converted to printk, lots of build warnings showed up as no one was checking the arguments to LogMsg. This patch fixes them all.
This is actually a side effect of sloppy code: the Linux kernel function printk is adorned with __attribute__((format(printf,1,2))), and the compiler knows how to automatically check the format strings with the associated parameters, and will report mistmatches.
But since LogMsg() didn't have the __attribute__, then none of this checking was done. This would have been trivial to add in the header: 
extern void LogMsg(const char *fmt, ...) __attribute__((format(printf,1,2)));
to have caught these long ago. And since the LogMsg wrapper has been replaced with the underyling printk, now GCC is catching it: hence this patch. It looks like around two dozen of them.
[PATCH 201/641] Staging: hv: osd: remove GetTickCount and GetTimestamp wrappers
GKH No one was even using them.
These are Win32 API functions that were turned into Linux equivalents, but since they were unused, they could be safely removed.
[PATCH 202/641] Staging: hv: Remove compatibility ifdefry
This seemed to remove the KERNEL_2_6_27 defines that were added previously. I don't get it.
[PATCH 203/641] Staging: hv: Transform some kzalloc calls to kcalloc
This is converting calls to kzalloc(n * size, flags) to kcalloc(n, size, flags), and it appears that this is going on with a lot of projects.
[PATCH 204/641] Staging: hv: force hyper-v drivers to be built as a module
GKH Right now they can not be built into the kernel due to global symbol name conflicts that this code is causing.
This change to the KConfig file forces the Hyper-V driver to build as a loadable module, rather than compiled-in driver. I don't know what global name conflicts are causing this, how hard it will be to fix, or whether this is sloppy or not.
[PATCH 205/641] Staging: hv: Use %ld instead of %d for a long ints
This may be part of sloppy coding: gcc is happy to complain about these mismatches if you crank the warning level up, which is something everybody should do.
[PATCH 206/641] Staging: hv: Remove C99 comments
GKH Remove C99 // comments with traditional /* */ comments
It's interesting that they aren't allowing // comments, especially since they are found in other parts of the kernel.
[PATCH 207/641] Staging: hv: StorVsc.c: fix print formatting
GKH There were a few places that used %lx when they should have used %x and a few places that used %d when they should have used %ld
[PATCH 208/641] Staging: hv: blkvsc_drv.c: fix print formatting
More DPRINT_DBG/INFO % formatting
[PATCH 209/641] Staging: hv: fix blkvsc_open() parameters
GKH blkvsc_open() had the wrong parameter list for struct block_device_operations
This may be an outright bug, though I'm not 100% sure.
[PATCH 210/641] Staging: hv: fix blkvsc_release() parameters
GKH blkvsc_release() had the wrong parameter list for struct block_device_operations
ditto
[PATCH 211/641] Staging: hv: fix blkvsc_ioctl() parameters
GKH blkvsc_ioctl() had the wrong parameter list for struct block_device_operations
ditto
[PATCH 212/641] Staging: hv: Remove X2V_LINUX check
GKH Remove preprocessor check for X2V_LINUX in osd.c
I have no idea what this is for, but it's probably cruft from some prior code that would have never compiled in the kernel.
[PATCH 213/641] Staging: hv: NetVsc.c: fix print formatting
more printf % mismatches
[PATCH 214/641] Staging: hv: RndisFilter.c: fix print formatting
Mostly innocuous: sizeof is a long, requires %ld.
[PATCH 215/641] Staging: hv: storvsc_drv.c: fix print formatting
Outright bug (though innocuous); enabling gcc warnings would have caught this.
[PATCH 216/641] Staging: hv: vmbus_drv.c: fix print formatting
More printf % mismatches. This one was an outright bug (though innocuous).
[PATCH 217/641] Staging: hv: check return value of bus_register()
The existing code was attempting to register the HyperV bus with the Linux Driver Model, but didn't check the return code. I don't know how realistic it is for this to fail, but this patch adds an error-handling check.
Gut feel: this was a bug.
[PATCH 218/641] Staging: hv: check return value of device_register()
ditto
[PATCH 219/641] Staging: hv: vmbus_drv.c: remove unused structs
GKH vmbus_ctl_table_hdr, vmbus_dev_ctl_table, vmbus_ctl_table, and vmus_root_ctl_table were never used. This removes them.
[PATCH 220/641] Staging: hv: Hv.c: remove unused physAddr
GKH physAddr was declared but never used in HvInit()
[PATCH 221/641] Staging: hv: remove VMBUS_CHANNEL_PACKET_PAGE_BUFFER typedef
Replaces use of the typedef VMBUS_CHANNEL_PACKET_PAGE_BUFFER with struct VMBUS_CHANNEL_PACKAGE_PAGE_BUFFER, then deletes the typedef.
[PATCH 222/641] Staging: hv: remove VMBUS_CHANNEL_PACKET_MULITPAGE_BUFFER typedef
ditto, though it looks like a typo was preserved: shouldn't MULITPAGE be MULTIPAGE?
[PATCH 223/641] Staging: hv: remove VMBUS_CONNECT_STATE typedef
ditto
[PATCH 224/641] Staging: hv: remove VMBUS_CONNECTION typedef
ditto
[PATCH 225/641] Staging: hv: remove VMBUS_MSGINFO typedef
ditto
[PATCH 226/641] Staging: hv: remove NETVSC_DEVICE typedef
ditto
[PATCH 227/641] Staging: hv: Remove WORKQUEUE typedef
Replaced the use of a generic HANDLE with struct workqueue_struct.
[PATCH 228/641] Staging: hv: Transform PDEVICE_OBJECT and DEVICE_OBJECT typedefs into their corresponding structs
GKH Remove typedef DEVICE_OBJECT and use a struct named hv_device instead.
Remove typedef PDEVICE_OBJECT which aliases a pointer and use struct hv_device instead.
[PATCH 229/641] Staging: hv: check return value of driver_for_each_device()
GKH The return value of driver_for_each_device() is now checked. A non-zero value simply generates a warning message, but it's better than not checking at all.
[PATCH 230/641] Staging: hv: comment out blkdev variable in blkvsc_ioctl
GKH This variable generated an unused variable warning due to other code in the fuction being commented out. This comments out the variable defination so that the code compiles without warnings.
It's common for one set of cleanups to create new warnings for variables that used to be used, but now are not. I assume that at some point in the future the line will be removed, not just commented out.
[PATCH 231/641] Staging: hv: remove WAITEVENT typedef
GKH Remove the WAITEVENT typedef and also replace HANDLE types that use the WaitEvent calls with struct osd_waitevent.
More migration to the direct underlying types, replacing the typedefs.
[PATCH 232/641] Staging: hv remove TIMER typedef
GKH Remove the TIMER typedef and also replace HANDLE types that use the timer calls.
ditto
[PATCH 233/641] Staging: hv: remove HANDLE typedef
The HANDLE was little more than a void*, a generic object handle: clearly the kernel team prefers to use real underlying types, which allows for better type checking. I think this is a great idea.
[PATCH 234/641] Staging: hv: remove more printk() warnings
GKH This should fix up the rest of the printk() warnings on an i386 build
[PATCH 235/641] Staging: hv: properly fix the printk() warnings
GKH This fixes the printk() warnings on all platforms now (x86-64 and i386).
[PATCH 236/641] Staging: hv: Remove typedef DRIVER_OBJECT and PDRIVER_OBJECT
GKH typedef DRIVER_OBJECT and PDRIVER_OBJECT are removed and their usages are replace by the use of struct hv_driver and struct hv_driver * respectively.
More migration from quasi-opaque types to literal struct types.
[PATCH 237/641] Staging: hv: Remove typedef NETVSC_PACKET and PNETVSC_PACKET
GKH typedef NETVSC_PACKET and PNETVSC_PACKET are removed and their usages are replace by the use of struct hv_netvsc_packet and struct hv_netvsc_packet * respectively.
ditto
[PATCH 238/641] Staging: hv: Remove typedef STORVSC_REQUEST and PSTORVSC_REQUEST
GKH typedef STORVSC_REQUEST and PSTORVSC_REQUEST are removed and their usages are replace by the use of struct hv_storvsc_request and struct hv_storvsc_request * respectively.
ditto
[PATCH 239/641] Staging: hv: fix sparse static warnings
GKH This fixes up all of the sparse warnings about static functions.
A number of functions are not used outside the modules they are defined in, so they are marked static. This is a best practice that I'd have liked Microsoft to have done.
[PATCH 240/641] Staging: hv: fix sparse function warnings
GKH This fixes up all of the sparse warnings about functions not being properly declared. Meaning, void functions need to say they are a void function, otherwise the compiler assumes it is an integer here.
Somebody had been doing kernel development without warnings cranked way up. Bad engineer.
[PATCH 241/641] Staging: hv: fix sparse NULL pointer warnings
GKH This fixes up all of the sparse warnings where NULL should be used instead of 0.
Hmmm, this seems petty to me: the C language defines an integral constant of zero as meaning "the null pointer" (including on platforms for which a NULL pointer is not all-bits-zero). There's no reason to change this other than personal style.
[PATCH 242/641] Staging: hv: rework use of workqueues in osd
GKH Change the usage of workqueues to be consistant with other parts of the kernel.
This undid an attempt to Win32-ify some Linux kernel mechanisms.
[PATCH 243/641] Staging: hv: remove WaitEventClose()
GKH All WaitEventClose() close did was call kfree(), so get rid of it and replace it with a call to kfree()
[PATCH 244/641] Staging: hv: remove wrapper functions for bit operations
GKH There were several Bit* functions that did nothing but call the kernel functions with the parameters reversed. Remove these and call the functions directly.
MSFT had defined a bunch of Win32-ish bit functions that just wrapped the Linux kernel versions, and unwinding them to use the Linux versions makes it easier for others to understand.
[PATCH 245/641] Staging: hv: remove wrapper functions for atomic operations
Ditto, for atomic operations
[PATCH 246/641] Staging: hv: remove wrapper function VirtualFree
Replace calls to VirtualFree(addr) to use vfree(addr).
[PATCH 247/641] Staging: hv: remove wrapper functions around kmap_
GKH Remove PageMapVirtualAddress() and PageUnmapVirtualAddress() which were wrappers around kmap_atomic() and kunmap_atomic()
[PATCH 248/641] Staging: hv: remove custom cpuid function
GKH Use the one that the kernel provides, it does it correctly.
[PATCH 249/641] Staging: hv: remove custom rdmsrl and wrmsrl functions
GKH Use the ones that the kernel provides, they do it correctly.
I don't know if the MSFT-provided functions — written in assembler — did the right thing or the wrong thing, but this seems like madness to do it twice when the existing code would do it just fine.
[PATCH 250/641] Staging: hv: osd: remove physical address wrapper functions
GKH Use the real functions the kernel provides, so that people can see what is actually going on in the code easier.
"Speak kernel with a Linux accent, not a Windows accent"
[PATCH 251/641] Staging: hv: osd: add osd_ prefix to global functions
GKH Put a "osd_" prefix on the osd.c functions in order for us to play nicer in the kernel namespace.
This might be what was referred to patch #204 (above): if Microsoft did their driver development without including every conceivable driver, it may be that they chose symbol names that conflicted with other modules. Adding a common prefix eliminates this clash.
[PATCH 252/641] Staging: hv: remove timer wrapper functions
GKH Use a real timer (there's only one in the code), no wrapper is needed, it just increases the complexity for no reason.
[PATCH 253/641] Staging: hv: remove duplicated osd.o inclusions
GKH Now that we have properly prefixed the osd.c functions, we don't need to include it in each of the modules. So only build it into the hv_vmbus module.
[PATCH 254/641] Staging: hv: Replace typedef SG_BUFFER_LIST by struct scatterlist
GKH typedef SG_BUFFER_LIST is removed and its uses are replaced by the use of struct scatterlist.
"SG" = scatter/gather I/O.
[PATCH 255/641] Staging: hv: blk dev depends on SCSI
GKH hv block driver uses scsi_*() interfaces so it should depend on SCSI.
If you tried to build the system with Hyper-V but without SCSI, it would fail; this adds the dependency so SCSI is added automatically.
[PATCH 256/641] Staging: hv: adjust Hyper-V Kconfig
GKH

Hyper-V sub-components' options should all depend on the base option.

The default of these sub-component options is also more reasonably set to that of the base option (since it makes little sense to enable the base option without the sub-component ones).

This looks like the kind of routine packaging that's done when something gets closer to "product" status.
[PATCH 257/641] Staging: hv: remove ReadMsr and WriteMsr functions from Hv.h
GKH They aren't needed as wrappers.
Ref: #249 above.
[PATCH 258/641] Staging: hv: cleanup coding style issues in Hv.h
Just shuffling around some whitespace for style, plus deleted some #include refs that were not needed.
[PATCH 259/641] Staging: hv: cleanup coding style issues in Channel.h
ditto
[PATCH 260/641] Staging: hv: cleanup coding style issues in VersionInfo.h
ditto
[PATCH 261/641] Staging: hv: cleanup coding style issues in ChannelInterface.h
ditto
[PATCH 262/641] Staging: hv: cleanup coding style issues in ChannelMgmt.h
ditto
[PATCH 263/641] Staging: hv: cleanup coding style issues in VmbusPrivate.h
ditto
[PATCH 264/641] Staging: hv: cleanup coding style issues in RingBuffer.h
ditto
[PATCH 265/641] Staging: hv: remove Sources.c
GKH It's a .c file including other .c files, ick.
Remove that mess now that the header files are unwound.
Ok, this is just yucky:
hv/Sources.c
-/*
- * Copyright (c) 2009, Microsoft Corporation.
- *
- * long copyright stuff deleted
- */
-
-#include "Vmbus.c"
-#include "Hv.c"
-#include "Connection.c"
-#include "Channel.c"
-#include "ChannelMgmt.c"
-#include "ChannelInterface.c"
-#include "RingBuffer.c"
It's being removed after refactoring.
[PATCH 266/641] Staging: hv: clean up NetVsc.h
GKH Cleans up coding style issues with NetVsc.h
Mostly moving whitespace around
[PATCH 267/641] Staging: hv: clean up RndisFilter.h
GKH Cleans up coding style issues with RndisFilter.h
ditto
[PATCH 268/641] Staging: hv: clean up typedefs in Hv.h
GKH This removes the typedefs from Hv.h, it's now clean from a codingstyle.pl standpoint.
Some whitespace shuffling, replaced use of some typedefs with the underlying structs.
[PATCH 269/641] Staging: hv: clean up typedefs in ChannelMgmt.h
GKH This removes the typedefs from ChannelMgmt.h, it's now clean from a codingstyle.pl standpoint.
[PATCH 270/641] Staging: hv: clean up vstorage.h
GKH Lots of coding style cleanups in vstorage.h
Looks mostly superficial.
[PATCH 271/641] Staging: hv: move osd.h
GKH This moves osd.h out of the include/ subdirectory.
No code changes are made here.
Moved "osd.h" from include/ to ./; drivers aren't supposed to have a separate include subdir.
[PATCH 272/641] Staging: hv: osd.h: codingstyle cleanups
GKH This fixes up the coding style issues in osd.h, with the exception of the typedefs, they will be removed later.
Mostly whitespace changes
[PATCH 273/641] Staging: hv: osd.h: remove GUID typedef
GKH GUID should not be a typedef. As proof of the problem of typedefs, look, we are passing 2 of these as a value in functions! Bah...
[PATCH 274/641] Staging: hv: osd.h: fix GUID reference problem
GKH As GUID was a typedef, it hid the fact that we were passing it a 2 variables in functions. This fixes this up by passing it as a pointer, as it should be.
It looks like this was a bug.
[PATCH 275/641] Staging: hv: osd.c: coding style fixes
GKH Codingstyle fixes for osd.c
Mostly whitespace changes
[PATCH 276/641] Staging: hv: remove include/HvTypes.h
GKH It isn't needed at all, was only being used for one typedef, which is now removed.
[PATCH 277/641] Staging: hv: remove include/HvHalApi.h
GKH It isn't needed at all, was only being used for one typedef, which is now removed.
[PATCH 278/641] Staging: hv: coding style cleanup of include/HvHcApi.h
GKH Coding style fixes for include/HvHcApi.h
Changing use of typedef names to use of struct names, plus whitespace changes
[PATCH 279/641] Staging: hv: coding style cleanup of include/HvVpApi.h
GKH

Coding style fixes for include/HvVpApi.h

All of the include/Hv*.h files should be merged eventually...

[PATCH 280/641] Staging: hv: move vmbus.h
GKH

This moves vmbus.h from the include/ subdirectory. It doesn't belong there.

No code changes happened here.

Drivers shouldn't generally use include/ subdirs.
[PATCH 281/641] Staging: hv: vmbus.h coding style cleanups
GKH Coding style fixes for vmbus.h
[PATCH 282/641] Staging: hv: move rndis.h
GKH

This moves rndis.h from the include/ subdirectory. It doesn't belong there.

No code changes happened here.

[PATCH 283/641] Staging: hv: rndis.h: remove pointless typedefs
GKH The typedefs for u32 are now removed.
[PATCH 284/641] Staging: hv: rndis.h: codingstyle fixes
GKH This fixes all of the coding style issues in rndis.h with the exception of the typedefs. That comes next.
[PATCH 285/641] Staging: hv: rndis.h: typedef removal, part 1
GKH This removes about half of the typedefs in rndis.h
Changing use of typedef names to the use of underlying struct names
[PATCH 286/641] Staging: hv: rndis.h: typedef removal, part 2
GKH

This removes the rest of the typedefs in rndis.h

The file is now checkpatch.pl clean.

Note, there are a lot of structures in this file that are not used anywhere. I don't know if we want to remove them, but I guess they don't take up any space so it's a nice documentation trail.

ditto
[PATCH 287/641] Staging: hv: coding style cleanups for HvPtApi.h
GKH Fix up the typedefs in there as well.
[PATCH 288/641] Staging: hv: coding style cleanups for HvSynicApi.h
GKH Don't fix the typedef issues, that will come next.
[PATCH 289/641] Staging: hv: typdef fixes for HvSynicApi.h
GKH Still some volatile mis-usages left to fix.
[PATCH 290/641] Staging: hv: remove volatile usage from HvSynicApi.h
GKH It's pretty pointless as no one is using this structure, but even so the use of volatile is so wrong here it's sad...
I'm not sure I agree with the "sad" part, though I don't know enough about it to be sure. The big sin with the volatile type qualifier is omitting it when it should be included — this can be catastrophic — but the penalty for including it when not needed is likely a very minor performance hit.
But I could be wrong.
[PATCH 291/641] Staging: hv: create hv_api.h
GKH

Merge the different include/Hv*Api.h files together into hv_api.h as they really don't justify separate files.

No code was changed here, only moving stuff around.

It looks like this was what he mentioned in #280 above
[PATCH 292/641] Staging: hv: coding style cleanups for HvStatus.h
GKH Ugh, what a mess, it's all better now.
Calling this a "mess" seems way overdramatic; it mainly changes the use of the HV_STATUS typedef to a direct u16 (unsigned 16-bit int), plus lots of whitespace changes.
[PATCH 293/641] Staging: hv: move HvStatus.h into hv_api.h
GKH

It doesn't need to be a standalone file anymore.

No code changed, only moving things around.

[PATCH 294/641] Staging: hv: coding style cleanups for VmbusChannelInterface.h
GKH typedefs still need to be fixed up.
Looks like mostly whitespace fixes.
[PATCH 295/641] Staging: hv: typedef removal from VmbusChannelInterface.h
GKH It's all clean now.
[PATCH 296/641] Staging: hv: coding style cleanups for VmbusApi.h
GKH typedefs still need to be fixed up.
[PATCH 297/641] Staging: hv: typedef removal for VmbusApi.h
GKH

The function pointers still have ugly names, but the structures are now cleaned up.

Note, a comment was added where the driver structure is pointing at a problem that needs to be fixed up later in the code.

Yah, I'd imagine that Hungarian notation wouldn't go over so well in the Linux kernel.
[PATCH 298/641] Staging: hv: codingsyle cleanups for ChannelMessages.h
GKH

Everything but the typedefs are taken care of.

Also a number of unused defines were removed.

[PATCH 299/641] Staging: hv: typedef removal for ChannelMessages.h
GKH ChannelMessages.h is now coding style clean.
[PATCH 300/641] Staging: hv: fix up some coding style issues in logging.h
GKH

It's now clean.

Well, from a coding style guide, not from a logic standpoint, the whole file needs to be tossed overboard and cheered on as the sharks tear it to individual bits.

Can't tell from the patch what he objects to.
[PATCH 301/641] Staging: hv: fix up coding style issues in NetVscApi.h
GKH Everything but the typedefs.
[PATCH 302/641] Staging: hv: fix up typedefs in NetVscApi.h
GKH It's now all clean from a coding style standpoint.
[PATCH 303/641] Staging: hv: move vstorage.h to hv dir
GKH

Move it out of the include subdir, it doesn't need to be there.

No code was changed in the move.

[PATCH 304/641] Staging: hv: fix remaining style issue in ChannelInterface.h
GKH It snuck in with the other typedef cleanups, sorry about that.
[PATCH 305/641] Staging: hv: fix typedefs in vstorage.h
GKH It's all clean now.
[PATCH 306/641] Staging: hv: fix coding style issues in VmbusPacketFormat.h
GKH

Heh, volatiles, like that was a good idea... Turns out they were not even used, wierd stuff.

All clean except for the typedefs.

This did lots of coding changes, including changing from the MSVC style of #pragma packed notation to the proper gcc __attribute__((packed)) notation. Looking at the structures in question, they look optimally packed anyway, but if the code depends on packing, it has to be done properly.
[PATCH 307/641] Staging: hv: remove typedefs from VmbusPacketFormat.h
GKH All spiffied up now, shines like a brass button on the bump of a barge's bilge.
Nothing but typedef-to-struct changes
[PATCH 308/641] Staging: hv: fix coding style issues in StorVscApi.h
GKH No typedef changes yet though.
I consider this patch to have fixed a bug: 
- #define STORVSC_RING_BUFFER_SIZE			10*PAGE_SIZE
- #define BLKVSC_RING_BUFFER_SIZE			20*PAGE_SIZE
+ #define STORVSC_RING_BUFFER_SIZE			(10*PAGE_SIZE)
+ #define BLKVSC_RING_BUFFER_SIZE			(20*PAGE_SIZE)
plus a few whitespace fixes.
[PATCH 309/641] Staging: hv: fix typedefs in StorVscApi.h
GKH It's all clean now.
[PATCH 310/641] Staging: hv: fix coding style issues in nvspprotocol.h
GKH No typedef changes yet though.
[PATCH 311/641] Staging: hv: fix typedefs in nvspprotocol.h
GKH It's all clean now.
[PATCH 312/641] Staging: hv: move nvspprotocol.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 313/641] Staging: hv: remove ChannelMessages.h
GKH

It's only ever used within the ChannelMgmt.h file, so pull it into there.

No code changes here, just file movements.

[PATCH 314/641] Staging: hv: move List.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 315/641] Staging: hv: move logging.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 316/641] Staging: hv: move NetVscApi.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 317/641] Staging: hv: move StorVscApi.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 318/641] Staging: hv: move VmbusApi.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 319/641] Staging: hv: move VmbusChannelInterface.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 320/641] Staging: hv: move VmbusPacketFormat.h
GKH

Move it out of the include subdirectory.

No code changes here, just file movements.

[PATCH 321/641] Staging: hv: coding style cleanups of BlkVsc.c
This patch makes it look like more things changed than actually did: it looks like a function moved, so this really confuses the diff-er. This is mostly whitespace changes.
[PATCH 322/641] Staging: hv: coding style cleanups of ChannelInterface.c
Typedef to struct changes only
[PATCH 323/641] Staging: hv: remove typedefs from ChannelMgmt.c
[PATCH 324/641] Staging: hv: remove typedefs from RndisFilter.c
[PATCH 325/641] Staging: hv: remove typedefs from StorVsc.c
[PATCH 326/641] Staging: hv: coding style cleanups for Connection.c
GKH Everything is clean except for some very long lines that need a bit more code rework to resolve. That will have to be done by someone that can test the code.
[PATCH 327/641] Staging: hv: coding style cleanups on Vmbus.c
GKH It's now much nicer and cleaner.
[PATCH 328/641] Staging: hv: code reduction from Vmbus.c
GKH Reorder some functions to get rid of all of the forward declarations.
This kind of thing always makes the diff go crazy, but doesn't actually involve any functional changes.
[PATCH 329/641] Staging: hv: fix coding style issues in Hv.c
GKH

Lots of cleanups.

Note, the use of volatile still needs to be resolved, and possibly the #ifdef could be done a bit "better".

This is the business end of the driver: it's the interface that crosses the line from Linux to the hypervisor, and this involves all kinds of scary assembler gating.
This makes it all the more important to make the code Über clean and easy to read, not lines of assembler more than 200 characters wide! 
-	__asm__ __volatile__ ("call *%8" : "=d"(hvStatusHi), "=a"(hvStatusLo) : "d" (controlHi), "a" (controlLo), "b" (inputAddressHi), "c" (inputAddressLo), "D"(outputAddressHi), "S"(outputAddressLo), "m" (hypercallPage));
+	__asm__ __volatile__ ("call *%8" : "=d"(hvStatusHi),
+			      "=a"(hvStatusLo) : "d" (controlHi),
+			      "a" (controlLo), "b" (inputAddressHi),
+			      "c" (inputAddressLo), "D"(outputAddressHi),
+			      "S"(outputAddressLo), "m" (hypercallPage));
This is not just "whitespace reformatting". Sheesh. I like readable code, especially when it's trivial to achieve.
The volatile stuff rears its head again, and I don't have a good handle on how it should be done. But I do know that erring in the side of volatile makes code safer. But on the other hand, it can also be a crutch that substitutes volatile for meticulously understanding how your own code works.
[PATCH 330/641] Staging: hv: coding style cleanups for netvsc_drv.c
GKH

There are still some horrible long lines in here, which some simple code reworking will make smaller and easier to understand.

Also note the FIXME in struct netvsc_driver_context...

[PATCH 331/641] Staging: hv: reorganize netvsc_drv.c
GKH Saves space by getting rid of the forward declarations.
Moving functions around really scares diff.
[PATCH 332/641] Staging: hv: coding style cleanups for ChannelMgmt.c
GKH There are still some too-long lines here, hopefully they will get resolved later.
[PATCH 333/641] Staging: hv: reorg ChannelMgmt a bit
GKH This gets rid of the unneeded typedef and the forward declarations, saving a bit of code file size.
[PATCH 334/641] Staging: hv: TODO: add some more items
GKH Add List.h and RingBuffer.h removal items.
The two added items:
TODO
+	- remove List.h usage to use in-kernel list.h instead
+	- remove RingBuffer.c to us in-kernel ringbuffer functions instead.
Curiously, set of prior patches by Bill Pemberton earlier in the season had done exactly this (for List.h), but I guess it didn't get accepted.
Update: Bill's patches were accepted in items #351..354 below.
[PATCH 335/641] Staging: hv: coding style cleanups for StorVsc.c
GKH Some one owes me a lot of beer, or a nice bottle of rum for all of this crud cleanup...
I dunno if mostly whitespace-reformatting really counts as "crud" - this looks really routine.
[PATCH 336/641] Staging: hv: reorg StorVsc.c
GKH This gets rid of all of the forward declarations.
[PATCH 337/641] Staging: hv: coding style fixes for blkvsc_drv.c
GKH There are still some very long lines, someone needs to unwind the logic there to resolve that.
I hate long lines: it usually means overly indented loop logic.
[PATCH 338/641] Staging: hv: coding style cleanup for Channel.c
GKH All clean now.
[PATCH 339/641] Staging: hv: warn the world of a bug in the release function
GKH All device release functions need to do something, if not, it's a bug. By merely providing an "empty" release function, it gets the kernel to shut up, but that's not solving the problem at all. Stick a big fat WARN_ON(1); in there to get people's attention.
This is probably a resource leak, or worse; it really does need to be fixed, and even in the hard-to-believe case that a device release does not actually have anything to do, a BIG FAT COMMENT needs to describe why this is the case.
[PATCH 340/641] Staging: hv: coding style cleanups for vmbus_drv.c
GKH Almost clean.
Whitespace
[PATCH 341/641] Staging: hv: coding style cleanup for storvsc_drv.c
GKH Where's the hazard pay for cleaning up this mess...
Um, how hazardous is whitespace?
[PATCH 342/641] Staging: hv: coding style cleanup for RndisFilter.c
GKH It's much better now
[PATCH 343/641] Staging: hv: coding style cleanup for NetVsc.c
[PATCH 344/641] Staging: hv: rename struct NETVSC_DEVICE
GKH The Linux kernel doesn't have all caps structures, we don't like to shout at our programmers, it makes them grumpy. Instead, we like to sooth them with small, rounded letters, which puts them in a nice, compliant mood, and makes them more productive and happier, allowing them more fufilling lives overall.
I think somebody got him that bottle of rum :-)
[PATCH 345/641] Staging: hv: remove function pointer typedefs from VmbusApi.h
GKH function pointer typedefs are allowed in the kernel, but only if they make sense, which they really do not here, as they are not passed around with any kind of frequency. So just spell them all out, it makes the code smaller and easier to understand overall.
This is more a personal preference thing: there's something to be said for giving a type a name, but I understand the history here. But it's not a slam dunk.
[PATCH 346/641] Staging: hv: remove function pointer typedefs from NetVscApi.h
ditto
[PATCH 347/641] Staging: hv: remove function pointer typedefs from StorVscApi.h
ditto
[PATCH 348/641] Staging: hv: remove PFN_CHANNEL_CALLBACK
GKH Come on people, it doesn't get simpler than this, why have a typedef for something so tiny...
... because a typedef encapsulates type information, making it much easier to centrally change the type when needed. Admittedly this is a bigger deal in Win32 than it is on Linux, mainly because the function pointer also conveys a calling-convention qualifier (__cdecl, __stdcall, etc.), which doesn't apply to Linux, but this is not foolish.
[PATCH 349/641] Staging: hv: remove function pointer typedefs from vmbus.h
GKH It's amazing the hoops that people go through to make code work when they don't opensource the whole thing. Passing these types of function pointers around for no good reason is a mess, this needs to be unwound as everything is now in the open.
Ok, now Greg is outright ignorant here: this has nothing to do with "opensource".
It's one thing to say that Linux does things a certain way, and that new drivers ought to do it that way — which is a fair point I mostly agree with — but the code that was changed was positively fully in the spirit of how Win32 development is done.
Linux has a style, Win32 has a style, and neither one is right or wrong. It's OK to say "This is Linux, you'll do it our way", but you don't get to call the other guy wrong because it's not your style.
[PATCH 350/641] Staging: hv: Add Haiyang's email to the TODO file
GKH Add Haiyang's email to the TODO file.
Haiyang Zhang is a Microsoft developer.
[PATCH 351/641] Staging: hv: remove use of internal list routines in NetVsc
GKH The hv driver has its own linked list routines. This removes them from NetVsc and uses the kernels routines instead.
Ah: this was referenced in item #334 above. Always use existing mechanisms rather than rolling your own, because this reuses code that's been tested, plus is part of the vocabulary used by everybody else who works on the code.
[PATCH 352/641] Staging: hv: remove more usages of internal list routines
GKH The hv driver has its own linked list routines. This removes them from more places in hv.
[PATCH 353/641] Staging: hv: remove use of internal list routines in RndisFilter
GKH The hv driver has its own linked list routines. This removes them from more places in hv.
[PATCH 354/641] Staging: hv: Remove List.h
GKH List.h is no longer used and can be removed.
[PATCH 355/641] Staging: hv: update the TODO file
GKH Remove a few items that have already been resolved. There are only a few checkpatch issues, they need to be resolved by larger code logic changes that are not "simple" changes.
[PATCH 355/641] Staging: hv: update the TODO file
This was a "patch" from Hank @ Microsoft thanking everybody who worked on this.

Published: 2009/09/20 (blogged)