RE: various vmbus review comments

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@xxxxxxxxx]
> Sent: Tuesday, May 03, 2011 4:47 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/