[PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg at kroah.com]
> Sent: Monday, February 28, 2011 9:53 PM
> To: KY Srinivasan
> Cc: gregkh at suse.de; linux-kernel at vger.kernel.org;
> devel at linuxdriverproject.org; virtualization at 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