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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Wednesday, March 02, 2011 12:41 AM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; 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@xxxxxxxxx]
> > > Sent: Monday, February 28, 2011 9:53 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux