various vmbus review comments

various vmbus review comments

[Greg KH]

I just took a quick look at the vmbus code, and have the following
comments:
	- why is count_hv_channel() even a function?
	- your .h files need to be consolidated and renamed.  You will
	  need a single hyperv.h file for include/linux/ that will
	  contain some of what the vmbus*.h files have in it, but not
	  all.  Please merge things together to have:
	  	- include/linux/hyperv.h
		   What is needed to build the drivers that attach to
		   the bus
		- drivers/staging/hv/hyperv.h
		   The local .h file will have the vmbus*.h remaining
		   stuff that is only needed by the hv_vmbus.ko module
		   to be build.
	- the instances of hv_driver structures need to be static and
	  not programatically defined, like all other USB and PCI
	  drivers are handled.
	- module reference counting.  Are you sure you got it all right
	  for the individual modules that attach to the bus?  I don't
	  see any reference counting happening, is that correct?
	- fix the sparse warnings.
	- fix the use of volatile in the ring buffer code.  It should
	  not be needed and if you are relying on it, then the code is
	  wrong.
	- fix the namespace on the ringbuffer code to show that it
	  really is only for the hyperv bus code internally.

That should be enough for at least one more set of patches for you all
to work on :)

thanks,

greg k-h

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, May 03, 2011 4:47 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: various vmbus review comments
> 
> I just took a quick look at the vmbus code, and have the following
> comments:
> 	- why is count_hv_channel() even a function?
> 	- your .h files need to be consolidated and renamed.  You will
> 	  need a single hyperv.h file for include/linux/ that will
> 	  contain some of what the vmbus*.h files have in it, but not
> 	  all.  Please merge things together to have:
> 	  	- include/linux/hyperv.h
> 		   What is needed to build the drivers that attach to
> 		   the bus
> 		- drivers/staging/hv/hyperv.h
> 		   The local .h file will have the vmbus*.h remaining
> 		   stuff that is only needed by the hv_vmbus.ko module
> 		   to be build.
> 	- the instances of hv_driver structures need to be static and
> 	  not programatically defined, like all other USB and PCI
> 	  drivers are handled.
> 	- module reference counting.  Are you sure you got it all right
> 	  for the individual modules that attach to the bus?  I don't
> 	  see any reference counting happening, is that correct?
> 	- fix the sparse warnings.
> 	- fix the use of volatile in the ring buffer code.  It should
> 	  not be needed and if you are relying on it, then the code is
> 	  wrong.
> 	- fix the namespace on the ringbuffer code to show that it
> 	  really is only for the hyperv bus code internally.
> 
> That should be enough for at least one more set of patches for you all
> to work on :)

Thanks for taking the time to review this code. I will start work on addressing
your comments right away. As we had discussed on this list, my goal is to deal
with the vmbus related issues first. You had also suggested that I should ask for 
a community review once you had had applied my last set of patches. Now that
you have applied all my patches, should I formally ask for this review?

Regards,

K. Y

Re: various vmbus review comments

[Greg KH]

On Tue, May 03, 2011 at 09:00:01PM +0000, KY Srinivasan wrote:
> You had also suggested that I should ask for a community review once
> you had had applied my last set of patches. Now that you have applied
> all my patches, should I formally ask for this review?

I said something like "after my review comments are all handled".  It
wouldn't make much sense for more review when these comments aren't
handled yet, right (note, some of these were made previously, so I
wasn't the first to say them.)

How about when I am happy, then we ask everyone else?  I have a feeling
that will take a while anyway :)

thanks,

greg k-h

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, May 03, 2011 6:04 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Tue, May 03, 2011 at 09:00:01PM +0000, KY Srinivasan wrote:
> > You had also suggested that I should ask for a community review once
> > you had had applied my last set of patches. Now that you have applied
> > all my patches, should I formally ask for this review?
> 
> I said something like "after my review comments are all handled".  It
> wouldn't make much sense for more review when these comments aren't
> handled yet, right (note, some of these were made previously, so I
> wasn't the first to say them.)
> 
> How about when I am happy, then we ask everyone else?  I have a feeling
> that will take a while anyway :)

Fair enough; I will work on your comments first.

Regards,

K. Y

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, May 03, 2011 4:47 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: various vmbus review comments
> 
> I just took a quick look at the vmbus code, and have the following
> comments:
> 	- why is count_hv_channel() even a function?

I will fix this.

> 	- your .h files need to be consolidated and renamed.  You will
> 	  need a single hyperv.h file for include/linux/ that will
> 	  contain some of what the vmbus*.h files have in it, but not
> 	  all.  Please merge things together to have:
> 	  	- include/linux/hyperv.h
> 		   What is needed to build the drivers that attach to
> 		   the bus
> 		- drivers/staging/hv/hyperv.h
> 		   The local .h file will have the vmbus*.h remaining
> 		   stuff that is only needed by the hv_vmbus.ko module
> 		   to be build.

I have currently consolidated all the header files as follows:

1) hyperv.h - this will have all the vmbus related definitions
needed to build drivers that attach to the bus (as you have suggested).

2) hyperv_storage.h - this has all the definitions needed to build storage
drivers for Hyper-V. Storage drivers will include hyperv.h and 
hyperv_storage.h.

3) hyperv_net.h - this has all the definitions needed to build the network
driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h.

4) hyperv_utils.h - this has all the definitions needed to build the util driver.
The util driver would include hyperv.h and hyperv_utils.h.

All these four header files could potentially be under include/linux.
I have also created a private header file that has the definitions to 
build the vmbus driver - hyperv_vmbus.h.

Let me know if this is what you had in mind.


> 	- the instances of hv_driver structures need to be static and
> 	  not programatically defined, like all other USB and PCI
> 	  drivers are handled.
> 	- module reference counting.  Are you sure you got it all right
> 	  for the individual modules that attach to the bus?  I don't
> 	  see any reference counting happening, is that correct?

For this round, I want to concentrate on just the vmbus driver. So,
module reference counting is I don't think an issue for the vmbus driver
given that the driver is not unlodable. Once I am done with the vmbus driver
I will address the module reference counting issues for other drivers. 
I will also address your comment on static initialization hv_driver instances
as part of other driver cleanup.

> 	- fix the sparse warnings.

Will do. Just out of curiosity I ran sparse and was relieved to see only a handful
warnings!

> 	- fix the use of volatile in the ring buffer code.  It should
> 	  not be needed and if you are relying on it, then the code is
> 	  wrong.
> 	- fix the namespace on the ringbuffer code to show that it
> 	  really is only for the hyperv bus code internally.

I will fix these.

Regards,

K. Y

Re: various vmbus review comments

[Greg KH]

On Wed, May 04, 2011 at 04:20:11PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, May 03, 2011 4:47 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > Subject: various vmbus review comments
> > 
> > I just took a quick look at the vmbus code, and have the following
> > comments:
> > 	- why is count_hv_channel() even a function?
> 
> I will fix this.
> 
> > 	- your .h files need to be consolidated and renamed.  You will
> > 	  need a single hyperv.h file for include/linux/ that will
> > 	  contain some of what the vmbus*.h files have in it, but not
> > 	  all.  Please merge things together to have:
> > 	  	- include/linux/hyperv.h
> > 		   What is needed to build the drivers that attach to
> > 		   the bus
> > 		- drivers/staging/hv/hyperv.h
> > 		   The local .h file will have the vmbus*.h remaining
> > 		   stuff that is only needed by the hv_vmbus.ko module
> > 		   to be build.
> 
> I have currently consolidated all the header files as follows:
> 
> 1) hyperv.h - this will have all the vmbus related definitions
> needed to build drivers that attach to the bus (as you have suggested).

Great.

> 2) hyperv_storage.h - this has all the definitions needed to build storage
> drivers for Hyper-V. Storage drivers will include hyperv.h and 
> hyperv_storage.h.
> 
> 3) hyperv_net.h - this has all the definitions needed to build the network
> driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h.
> 
> 4) hyperv_utils.h - this has all the definitions needed to build the util driver.
> The util driver would include hyperv.h and hyperv_utils.h.

No for all 3 of these.  Why would a simple driver need a .h file at all?
Just include the structures within the .c file, making it nice and
self-contained.  There's no need to clutter things up, and individual
driver .h files should _never_ be in include/linux.

> All these four header files could potentially be under include/linux.
> I have also created a private header file that has the definitions to 
> build the vmbus driver - hyperv_vmbus.h.

Sure, you can call it that, but as it will be local to the hyperv bus
directory, the name isn't all that important.

> Let me know if this is what you had in mind.
> 
> 
> > 	- the instances of hv_driver structures need to be static and
> > 	  not programatically defined, like all other USB and PCI
> > 	  drivers are handled.
> > 	- module reference counting.  Are you sure you got it all right
> > 	  for the individual modules that attach to the bus?  I don't
> > 	  see any reference counting happening, is that correct?
> 
> For this round, I want to concentrate on just the vmbus driver. So,
> module reference counting is I don't think an issue for the vmbus driver
> given that the driver is not unlodable. Once I am done with the vmbus driver
> I will address the module reference counting issues for other drivers. 

No, I am referring to the module reference counting of the bus drivers
that register with the vmbus core.  You aren't doing that at all, and
you probably need to make sure that this isn't needed.  That is
concentrating on the vmbus driver.

> I will also address your comment on static initialization hv_driver instances
> as part of other driver cleanup.

No, please do this now as it will show how to properly interact with the
vmbus core code in the correct manner.  Hopefully that will be correct,
but I have a feeling that it will show you some places in the API that
need to be changed...

thanks,

greg k-h

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, May 04, 2011 12:32 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Wed, May 04, 2011 at 04:20:11PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, May 03, 2011 4:47 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > Subject: various vmbus review comments
> > >
> > > I just took a quick look at the vmbus code, and have the following
> >
> > I have currently consolidated all the header files as follows:
> >
> > 1) hyperv.h - this will have all the vmbus related definitions
> > needed to build drivers that attach to the bus (as you have suggested).
> 
> Great.
> 
> > 2) hyperv_storage.h - this has all the definitions needed to build storage
> > drivers for Hyper-V. Storage drivers will include hyperv.h and
> > hyperv_storage.h.
> >
> > 3) hyperv_net.h - this has all the definitions needed to build the network
> > driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h.
> >
> > 4) hyperv_utils.h - this has all the definitions needed to build the util driver.
> > The util driver would include hyperv.h and hyperv_utils.h.
> 
> No for all 3 of these.  Why would a simple driver need a .h file at all?
> Just include the structures within the .c file, making it nice and
> self-contained.  There's no need to clutter things up, and individual
> driver .h files should _never_ be in include/linux.

Agreed - we don't need to clutter up  include/linux directory with individual
driver.h files. However, including all the definitions in the .c file would also be
ugly. For instance there are multiple *.c files for the storage drivers and we don't
want to replicate the contents of  hyperv_storage.h in each of these .c files.
The same is true for the netvsc driver - there are multiple .c files where we would need
to replicate the contents of hyperv_net.h. 

How about I keep these driver specific header files; but these could be local to the 
directory where these drivers land.
 
> >
> > > 	- the instances of hv_driver structures need to be static and
> > > 	  not programatically defined, like all other USB and PCI
> > > 	  drivers are handled.
> > > 	- module reference counting.  Are you sure you got it all right
> > > 	  for the individual modules that attach to the bus?  I don't
> > > 	  see any reference counting happening, is that correct?
> >
> > For this round, I want to concentrate on just the vmbus driver. So,
> > module reference counting is I don't think an issue for the vmbus driver
> > given that the driver is not unlodable. Once I am done with the vmbus driver
> > I will address the module reference counting issues for other drivers.
> 
> No, I am referring to the module reference counting of the bus drivers
> that register with the vmbus core.  You aren't doing that at all, and
> you probably need to make sure that this isn't needed.  That is
> concentrating on the vmbus driver.

Ok; I will look into this.

> 
> > I will also address your comment on static initialization hv_driver instances
> > as part of other driver cleanup.
> 
> No, please do this now as it will show how to properly interact with the
> vmbus core code in the correct manner.  Hopefully that will be correct,
> but I have a feeling that it will show you some places in the API that
> need to be changed...

Will do.

Regards,

K. Y

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, May 04, 2011 12:32 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Wed, May 04, 2011 at 04:20:11PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, May 03, 2011 4:47 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > Subject: various vmbus review comments
> > >
> > > I just took a quick look at the vmbus code, and have the following
> >
> > I have currently consolidated all the header files as follows:
> >
> > 1) hyperv.h - this will have all the vmbus related definitions
> > needed to build drivers that attach to the bus (as you have suggested).
> 
> Great.
> 
> > 2) hyperv_storage.h - this has all the definitions needed to build storage
> > drivers for Hyper-V. Storage drivers will include hyperv.h and
> > hyperv_storage.h.
> >
> > 3) hyperv_net.h - this has all the definitions needed to build the network
> > driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h.
> >
> > 4) hyperv_utils.h - this has all the definitions needed to build the util driver.
> > The util driver would include hyperv.h and hyperv_utils.h.
> 
> No for all 3 of these.  Why would a simple driver need a .h file at all?
> Just include the structures within the .c file, making it nice and
> self-contained.  There's no need to clutter things up, and individual
> driver .h files should _never_ be in include/linux.

Agreed - we don't need to clutter up  include/linux directory with individual
driver.h files. However, including all the definitions in the .c file would also be
ugly. For instance there are multiple *.c files for the storage drivers and we don't
want to replicate the contents of  hyperv_storage.h in each of these .c files.
The same is true for the netvsc driver - there are multiple .c files where we would need
to replicate the contents of hyperv_net.h. 

How about I keep these driver specific header files; but these could be local to the 
directory where these drivers land.
 
> >
> > > 	- the instances of hv_driver structures need to be static and
> > > 	  not programatically defined, like all other USB and PCI
> > > 	  drivers are handled.
> > > 	- module reference counting.  Are you sure you got it all right
> > > 	  for the individual modules that attach to the bus?  I don't
> > > 	  see any reference counting happening, is that correct?
> >
> > For this round, I want to concentrate on just the vmbus driver. So,
> > module reference counting is I don't think an issue for the vmbus driver
> > given that the driver is not unlodable. Once I am done with the vmbus driver
> > I will address the module reference counting issues for other drivers.
> 
> No, I am referring to the module reference counting of the bus drivers
> that register with the vmbus core.  You aren't doing that at all, and
> you probably need to make sure that this isn't needed.  That is
> concentrating on the vmbus driver.

Ok; I will look into this.

> 
> > I will also address your comment on static initialization hv_driver instances
> > as part of other driver cleanup.
> 
> No, please do this now as it will show how to properly interact with the
> vmbus core code in the correct manner.  Hopefully that will be correct,
> but I have a feeling that it will show you some places in the API that
> need to be changed...

Will do.

Regards,

K. Y

Re: various vmbus review comments

[Greg KH]

On Wed, May 04, 2011 at 04:58:39PM +0000, KY Srinivasan wrote:
> > > 2) hyperv_storage.h - this has all the definitions needed to build storage
> > > drivers for Hyper-V. Storage drivers will include hyperv.h and
> > > hyperv_storage.h.
> > >
> > > 3) hyperv_net.h - this has all the definitions needed to build the network
> > > driver for Hyper-V. The netvsc driver will include hyperv.h and hyperv_net.h.
> > >
> > > 4) hyperv_utils.h - this has all the definitions needed to build the util driver.
> > > The util driver would include hyperv.h and hyperv_utils.h.
> > 
> > No for all 3 of these.  Why would a simple driver need a .h file at all?
> > Just include the structures within the .c file, making it nice and
> > self-contained.  There's no need to clutter things up, and individual
> > driver .h files should _never_ be in include/linux.
> 
> Agreed - we don't need to clutter up  include/linux directory with individual
> driver.h files. However, including all the definitions in the .c file would also be
> ugly. For instance there are multiple *.c files for the storage drivers and we don't
> want to replicate the contents of  hyperv_storage.h in each of these .c files.
> The same is true for the netvsc driver - there are multiple .c files where we would need
> to replicate the contents of hyperv_net.h. 
> 
> How about I keep these driver specific header files; but these could be local to the 
> directory where these drivers land.

Yes, that is fine.  I still find it odd that you need multiple files for
the network driver.  We have much more complex and longer drivers for
common cards these days than this one, all in one file.  But that's for
you and the network developers to argue over :)

thanks,

greg k-h

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, May 04, 2011 12:32 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Wed, May 04, 2011 at 04:20:11PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, May 03, 2011 4:47 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > 	- module reference counting.  Are you sure you got it all right
> > > 	  for the individual modules that attach to the bus?  I don't
> > > 	  see any reference counting happening, is that correct?
> >
> > For this round, I want to concentrate on just the vmbus driver. So,
> > module reference counting is I don't think an issue for the vmbus driver
> > given that the driver is not unlodable. Once I am done with the vmbus driver
> > I will address the module reference counting issues for other drivers.
> 
> No, I am referring to the module reference counting of the bus drivers
> that register with the vmbus core.  You aren't doing that at all, and
> you probably need to make sure that this isn't needed.  That is
> concentrating on the vmbus driver.

I audited the block and the net drivers. As part of their exit routine,
they invoke vmbus_child_driver_unregister() after properly cleaning
up all the devices they are managing. Do you still see an issue with
regards to module reference counting.

> 
> > I will also address your comment on static initialization hv_driver instances
> > as part of other driver cleanup.
> 
> No, please do this now as it will show how to properly interact with the
> vmbus core code in the correct manner.  Hopefully that will be correct,
> but I have a feeling that it will show you some places in the API that
> need to be changed...

As opposed to run-time initialization of fields such as probe, etc; I have 
initialized them statically. For instance, in the blkvsc driver:

/* The one and only one */
static  struct storvsc_driver blkvsc_drv = {
        .base.probe =  blkvsc_probe,
        .base.remove =  blkvsc_remove,
        .base.shutdown = blkvsc_shutdown,
};

Is this what you had in mind.

Regards,

K. Y 

Re: various vmbus review comments

[Greg KH]

On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> > No, I am referring to the module reference counting of the bus drivers
> > that register with the vmbus core.  You aren't doing that at all, and
> > you probably need to make sure that this isn't needed.  That is
> > concentrating on the vmbus driver.
> 
> I audited the block and the net drivers. As part of their exit routine,
> they invoke vmbus_child_driver_unregister() after properly cleaning
> up all the devices they are managing. Do you still see an issue with
> regards to module reference counting.

I will look again, the next time I review the vmbus code.

> > > I will also address your comment on static initialization hv_driver instances
> > > as part of other driver cleanup.
> > 
> > No, please do this now as it will show how to properly interact with the
> > vmbus core code in the correct manner.  Hopefully that will be correct,
> > but I have a feeling that it will show you some places in the API that
> > need to be changed...
> 
> As opposed to run-time initialization of fields such as probe, etc; I have 
> initialized them statically. For instance, in the blkvsc driver:
> 
> /* The one and only one */
> static  struct storvsc_driver blkvsc_drv = {
>         .base.probe =  blkvsc_probe,
>         .base.remove =  blkvsc_remove,
>         .base.shutdown = blkvsc_shutdown,
> };
> 
> Is this what you had in mind.

Close.  The format is correct.

But what's with that ".base." crud?  That shows that something is wrong
as no USB or PCI or any other bus driver has to mess with a ".base"
subpointer in a driver structure.

See, I told you that when you converted to use this format the problems
would pop out at you :)

Please fix that.

thanks,

greg k-h

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, May 06, 2011 11:00 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> > > No, I am referring to the module reference counting of the bus drivers
> > > that register with the vmbus core.  You aren't doing that at all, and
> > > you probably need to make sure that this isn't needed.  That is
> > > concentrating on the vmbus driver.
> >
> > I audited the block and the net drivers. As part of their exit routine,
> > they invoke vmbus_child_driver_unregister() after properly cleaning
> > up all the devices they are managing. Do you still see an issue with
> > regards to module reference counting.
> 
> I will look again, the next time I review the vmbus code.

Thank you.

> 
> > > > I will also address your comment on static initialization hv_driver instances
> > > > as part of other driver cleanup.
> > >
> > > No, please do this now as it will show how to properly interact with the
> > > vmbus core code in the correct manner.  Hopefully that will be correct,
> > > but I have a feeling that it will show you some places in the API that
> > > need to be changed...
> >
> > As opposed to run-time initialization of fields such as probe, etc; I have
> > initialized them statically. For instance, in the blkvsc driver:
> >
> > /* The one and only one */
> > static  struct storvsc_driver blkvsc_drv = {
> >         .base.probe =  blkvsc_probe,
> >         .base.remove =  blkvsc_remove,
> >         .base.shutdown = blkvsc_shutdown,
> > };
> >
> > Is this what you had in mind.
> 
> Close.  The format is correct.
> 
> But what's with that ".base." crud?  That shows that something is wrong
> as no USB or PCI or any other bus driver has to mess with a ".base"
> subpointer in a driver structure.
> 
> See, I told you that when you converted to use this format the problems
> would pop out at you :)
> 
> Please fix that.

I will. I am incrementally cleaning up the drivers, I will cleanup the ".base" 
crud as part of additional cleanup that I will do.

Regards,

K. Y

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, May 03, 2011 4:47 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: various vmbus review comments
> 
> I just took a quick look at the vmbus code, and have the following
> comments:
> 	- why is count_hv_channel() even a function?


Done; I got rid of this function

> 	- your .h files need to be consolidated and renamed.  You will
> 	  need a single hyperv.h file for include/linux/ that will
> 	  contain some of what the vmbus*.h files have in it, but not
> 	  all.  Please merge things together to have:
> 	  	- include/linux/hyperv.h
> 		   What is needed to build the drivers that attach to
> 		   the bus
> 		- drivers/staging/hv/hyperv.h
> 		   The local .h file will have the vmbus*.h remaining
> 		   stuff that is only needed by the hv_vmbus.ko module
> 		   to be build.

Done; as I had informed you in an earlier mail, in addition to the two header files
you have mentioned, I have also created driver specific header files for block and
network drivers.

> 	- the instances of hv_driver structures need to be static and
> 	  not programatically defined, like all other USB and PCI
> 	  drivers are handled.

Done. You had expressed some concern that this would expose some issue
with the core vmbus driver (which is what I want to concentrate on this 
go around). I have done this for both the block driver and the mouse driver
and I am pretty sure I can do the same with the network driver. I have not 
currently done this for the network driver, since the number of patches I have 
to submit is already very large.

> 	- module reference counting.  Are you sure you got it all right
> 	  for the individual modules that attach to the bus?  I don't
> 	  see any reference counting happening, is that correct?

I have already exchanged an email with you on this. To summarize, it
does not look like there is a problem

> 	- fix the sparse warnings.
Mostly done; most of the errors are in the base kernel coming out of
the macro page_to_pfn()

> 	- fix the use of volatile in the ring buffer code.  It should
> 	  not be needed and if you are relying on it, then the code is
> 	  wrong.

You are right; all accesses were already serialized with a spin lock and the 
Volatile attribute was unnecessary.

> 	- fix the namespace on the ringbuffer code to show that it
> 	  really is only for the hyperv bus code internally.

Done.

> 
> That should be enough for at least one more set of patches for you all
> to work on :)

Greg,

I have had so much fun cleaning up these drivers that I lost track of the patch count.
I have addressed all the issues you have raised in addition to some other cleanup
that I was doing since about a week. As I look at the patch-set, I have little over 
200 patches. If it is ok with you, I would like to send them as a single set. Let me know 
what you prefer. I need to circulate these patches internally before I can send them out. 
I should be able to send these out early next week.


Regards,

K. Y

Re: various vmbus review comments

[Greg KH]

On Mon, May 09, 2011 at 01:46:56AM +0000, KY Srinivasan wrote:
> > 	- the instances of hv_driver structures need to be static and
> > 	  not programatically defined, like all other USB and PCI
> > 	  drivers are handled.
> 
> Done. You had expressed some concern that this would expose some issue
> with the core vmbus driver (which is what I want to concentrate on this 
> go around). I have done this for both the block driver and the mouse driver
> and I am pretty sure I can do the same with the network driver. I have not 
> currently done this for the network driver, since the number of patches I have 
> to submit is already very large.

Ok, but it shouldn't be a major change to the code, right?

> > 	- module reference counting.  Are you sure you got it all right
> > 	  for the individual modules that attach to the bus?  I don't
> > 	  see any reference counting happening, is that correct?
> 
> I have already exchanged an email with you on this. To summarize, it
> does not look like there is a problem
> 
> > 	- fix the sparse warnings.
> Mostly done; most of the errors are in the base kernel coming out of
> the macro page_to_pfn()
> 
> > 	- fix the use of volatile in the ring buffer code.  It should
> > 	  not be needed and if you are relying on it, then the code is
> > 	  wrong.
> 
> You are right; all accesses were already serialized with a spin lock and the 
> Volatile attribute was unnecessary.
> 
> > 	- fix the namespace on the ringbuffer code to show that it
> > 	  really is only for the hyperv bus code internally.
> 
> Done.
> 
> > 
> > That should be enough for at least one more set of patches for you all
> > to work on :)
> 
> Greg,
> 
> I have had so much fun cleaning up these drivers that I lost track of the patch count.
> I have addressed all the issues you have raised in addition to some other cleanup
> that I was doing since about a week. As I look at the patch-set, I have little over 
> 200 patches. If it is ok with you, I would like to send them as a single set. Let me know 
> what you prefer. I need to circulate these patches internally before I can send them out. 
> I should be able to send these out early next week.

A single set is fine, if that's what you want to do, I can handle that
amount as long as they all build all along the way and don't break
anything.

thanks,

greg k-h

Re: various vmbus review comments

[Greg KH]

On Mon, May 09, 2011 at 01:46:56AM +0000, KY Srinivasan wrote:
> > 	- the instances of hv_driver structures need to be static and
> > 	  not programatically defined, like all other USB and PCI
> > 	  drivers are handled.
> 
> Done. You had expressed some concern that this would expose some issue
> with the core vmbus driver (which is what I want to concentrate on this 
> go around). I have done this for both the block driver and the mouse driver
> and I am pretty sure I can do the same with the network driver. I have not 
> currently done this for the network driver, since the number of patches I have 
> to submit is already very large.

Ok, but it shouldn't be a major change to the code, right?

> > 	- module reference counting.  Are you sure you got it all right
> > 	  for the individual modules that attach to the bus?  I don't
> > 	  see any reference counting happening, is that correct?
> 
> I have already exchanged an email with you on this. To summarize, it
> does not look like there is a problem
> 
> > 	- fix the sparse warnings.
> Mostly done; most of the errors are in the base kernel coming out of
> the macro page_to_pfn()
> 
> > 	- fix the use of volatile in the ring buffer code.  It should
> > 	  not be needed and if you are relying on it, then the code is
> > 	  wrong.
> 
> You are right; all accesses were already serialized with a spin lock and the 
> Volatile attribute was unnecessary.
> 
> > 	- fix the namespace on the ringbuffer code to show that it
> > 	  really is only for the hyperv bus code internally.
> 
> Done.
> 
> > 
> > That should be enough for at least one more set of patches for you all
> > to work on :)
> 
> Greg,
> 
> I have had so much fun cleaning up these drivers that I lost track of the patch count.
> I have addressed all the issues you have raised in addition to some other cleanup
> that I was doing since about a week. As I look at the patch-set, I have little over 
> 200 patches. If it is ok with you, I would like to send them as a single set. Let me know 
> what you prefer. I need to circulate these patches internally before I can send them out. 
> I should be able to send these out early next week.

A single set is fine, if that's what you want to do, I can handle that
amount as long as they all build all along the way and don't break
anything.

thanks,

greg k-h

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Sunday, May 08, 2011 11:05 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Mon, May 09, 2011 at 01:46:56AM +0000, KY Srinivasan wrote:
> > > 	- the instances of hv_driver structures need to be static and
> > > 	  not programatically defined, like all other USB and PCI
> > > 	  drivers are handled.
> >
> > Done. You had expressed some concern that this would expose some issue
> > with the core vmbus driver (which is what I want to concentrate on this
> > go around). I have done this for both the block driver and the mouse driver
> > and I am pretty sure I can do the same with the network driver. I have not
> > currently done this for the network driver, since the number of patches I have
> > to submit is already very large.
> 
> Ok, but it shouldn't be a major change to the code, right?

Right, it is not. I will submit code for the net driver after I am done with this patch-set.

> 
> > > 	- module reference counting.  Are you sure you got it all right
> > > 	  for the individual modules that attach to the bus?  I don't
> > > 	  see any reference counting happening, is that correct?
> >
> > I have already exchanged an email with you on this. To summarize, it
> > does not look like there is a problem
> >
> > > 	- fix the sparse warnings.
> > Mostly done; most of the errors are in the base kernel coming out of
> > the macro page_to_pfn()
> >
> > > 	- fix the use of volatile in the ring buffer code.  It should
> > > 	  not be needed and if you are relying on it, then the code is
> > > 	  wrong.
> >
> > You are right; all accesses were already serialized with a spin lock and the
> > Volatile attribute was unnecessary.
> >
> > > 	- fix the namespace on the ringbuffer code to show that it
> > > 	  really is only for the hyperv bus code internally.
> >
> > Done.
> >
> > >
> > > That should be enough for at least one more set of patches for you all
> > > to work on :)
> >
> > Greg,
> >
> > I have had so much fun cleaning up these drivers that I lost track of the patch
> count.
> > I have addressed all the issues you have raised in addition to some other
> cleanup
> > that I was doing since about a week. As I look at the patch-set, I have little over
> > 200 patches. If it is ok with you, I would like to send them as a single set. Let me
> know
> > what you prefer. I need to circulate these patches internally before I can send
> them out.
> > I should be able to send these out early next week.
> 
> A single set is fine, if that's what you want to do, I can handle that
> amount as long as they all build all along the way and don't break
> anything.

Thanks Greg. 

Regards,

K. Y

Re: various vmbus review comments

[Christoph Hellwig]

On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> I audited the block and the net drivers. As part of their exit routine,
> they invoke vmbus_child_driver_unregister() after properly cleaning
> up all the devices they are managing. Do you still see an issue with
> regards to module reference counting.

Which is not the correct thing to do as explained in my last round
of reviews.  Take a look at the PCI code - the functional driver only
does a foo_untegister_driver (which maps almost directly to
driver_unregister), which then causes the device core to unbind the
devices.  The function driver must never call device_unregister
directly as the device continues to exist even if no driver is bound to
it.

Re: various vmbus review comments

[Christoph Hellwig]

On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> I audited the block and the net drivers. As part of their exit routine,
> they invoke vmbus_child_driver_unregister() after properly cleaning
> up all the devices they are managing. Do you still see an issue with
> regards to module reference counting.

Which is not the correct thing to do as explained in my last round
of reviews.  Take a look at the PCI code - the functional driver only
does a foo_untegister_driver (which maps almost directly to
driver_unregister), which then causes the device core to unbind the
devices.  The function driver must never call device_unregister
directly as the device continues to exist even if no driver is bound to
it.

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Monday, May 09, 2011 10:34 AM
> To: KY Srinivasan
> Cc: Greg KH; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> > I audited the block and the net drivers. As part of their exit routine,
> > they invoke vmbus_child_driver_unregister() after properly cleaning
> > up all the devices they are managing. Do you still see an issue with
> > regards to module reference counting.
> 
> Which is not the correct thing to do as explained in my last round
> of reviews.  Take a look at the PCI code - the functional driver only
> does a foo_untegister_driver (which maps almost directly to
> driver_unregister), which then causes the device core to unbind the
> devices.  The function driver must never call device_unregister
> directly as the device continues to exist even if no driver is bound to
> it.

I will address this. Greg had a concern about module reference counting
and looking at the current code, it did not appear to be an issue. The
change you are suggesting will not affect the vmbus core which is what I want
to focus on. I will however, fix this issue in the current round of patches I will
send out this week.

Regards,

K. Y

Re: various vmbus review comments

[Christoph Hellwig]

On Mon, May 09, 2011 at 02:56:52PM +0000, KY Srinivasan wrote:
> I will address this. Greg had a concern about module reference counting
> and looking at the current code, it did not appear to be an issue. The
> change you are suggesting will not affect the vmbus core which is what I want
> to focus on. I will however, fix this issue in the current round of patches I will
> send out this week.

It very clearly affects the interface between the core and the
functional drivers.  Trying to submit the core without making sure the
interface is exports works properly is not an overly good idea.

Re: various vmbus review comments

[Christoph Hellwig]

On Mon, May 09, 2011 at 02:56:52PM +0000, KY Srinivasan wrote:
> I will address this. Greg had a concern about module reference counting
> and looking at the current code, it did not appear to be an issue. The
> change you are suggesting will not affect the vmbus core which is what I want
> to focus on. I will however, fix this issue in the current round of patches I will
> send out this week.

It very clearly affects the interface between the core and the
functional drivers.  Trying to submit the core without making sure the
interface is exports works properly is not an overly good idea.

RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Tuesday, May 10, 2011 1:24 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; Greg KH; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
> 
> On Mon, May 09, 2011 at 02:56:52PM +0000, KY Srinivasan wrote:
> > I will address this. Greg had a concern about module reference counting
> > and looking at the current code, it did not appear to be an issue. The
> > change you are suggesting will not affect the vmbus core which is what I want
> > to focus on. I will however, fix this issue in the current round of patches I will
> > send out this week.
> 
> It very clearly affects the interface between the core and the
> functional drivers.  Trying to submit the core without making sure the
> interface is exports works properly is not an overly good idea.

I must be missing something here. As I look at the block driver (and
this is indicative of other drivers as well); the exit routine -
blkvsc_drv_exit, first iterates through all the devices it manages
and invokes device_unregister() on each of the devices and then 
invokes vmbus_child_driver_unregister() which is just a wrapper on
driver_unregister(). So, if I understand you correctly, you want the devices to
persist even if there is no driver bound to them. So, if I eliminated the code
that iterates over the devices and unregisters them, that should fix the problem
and I can do this without changing the vmbus core interfaces.

Regards,

K. Y

Re: various vmbus review comments

[Christoph Hellwig]

> I must be missing something here. As I look at the block driver (and
> this is indicative of other drivers as well); the exit routine -
> blkvsc_drv_exit, first iterates through all the devices it manages
> and invokes device_unregister() on each of the devices and then 
> invokes vmbus_child_driver_unregister() which is just a wrapper on
> driver_unregister(). So, if I understand you correctly, you want the devices to
> persist even if there is no driver bound to them. So, if I eliminated the code
> that iterates over the devices and unregisters them, that should fix the problem
> and I can do this without changing the vmbus core interfaces.

If that actually works go for it.  But there's no way to know that
without actually implementing and testing it.

Re: various vmbus review comments

[Greg KH]

On Tue, May 10, 2011 at 01:00:26PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Christoph Hellwig [mailto:hch@infradead.org]
> > Sent: Tuesday, May 10, 2011 1:24 AM
> > To: KY Srinivasan
> > Cc: Christoph Hellwig; Greg KH; gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > Subject: Re: various vmbus review comments
> > 
> > On Mon, May 09, 2011 at 02:56:52PM +0000, KY Srinivasan wrote:
> > > I will address this. Greg had a concern about module reference counting
> > > and looking at the current code, it did not appear to be an issue. The
> > > change you are suggesting will not affect the vmbus core which is what I want
> > > to focus on. I will however, fix this issue in the current round of patches I will
> > > send out this week.
> > 
> > It very clearly affects the interface between the core and the
> > functional drivers.  Trying to submit the core without making sure the
> > interface is exports works properly is not an overly good idea.
> 
> I must be missing something here. As I look at the block driver (and
> this is indicative of other drivers as well); the exit routine -
> blkvsc_drv_exit, first iterates through all the devices it manages
> and invokes device_unregister() on each of the devices and then 
> invokes vmbus_child_driver_unregister() which is just a wrapper on
> driver_unregister(). So, if I understand you correctly, you want the devices to
> persist even if there is no driver bound to them.

That's how the Linux driver model should be used, so yes, that is the
correct thing to do.

thanks,

greg k-h