lkml.org 
[lkml]   [2011]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
Date


> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, March 02, 2011 12:41 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 4/6] Staging: hv: Unify the hyperv driver abstractions
>
> On Wed, Mar 02, 2011 at 01:43:13AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Monday, February 28, 2011 9:53 PM
> > > 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 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.

Greg, the temporary name happens to be the name currently in use in the
code - this is not the name I introduced. Think of this as the surviving
data structure after the hv_driver state is consolidated into
(the existing) driver_context data structure. I did this in the spirit of
doing one thing at a time. If I am going to be
picking a more appropriate name for the consolidated data structure; I
might as well pick the final name that we want this unified driver abstraction
to be called.


>
> > > 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.

Based on your comments on intermediate names, would you recommend that
as part of consolidating the driver abstractions, I also rename this combined
state.

Regards,

K. Y


\
 
 \ /
  Last update: 2011-03-03 03:53    [W:0.097 / U:32.864 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site