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
> -----Original Message----- > From: Greg KH [mailto:greg at kroah.com] > Sent: Tuesday, May 03, 2011 4:47 PM > To: KY Srinivasan > Cc: gregkh at suse.de; linux-kernel at vger.kernel.org; > devel at linuxdriverproject.org; virtualization at 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
> -----Original Message----- > From: Greg KH [mailto:greg at kroah.com] > Sent: Tuesday, May 03, 2011 4:47 PM > To: KY Srinivasan > Cc: gregkh at suse.de; linux-kernel at vger.kernel.org; > devel at linuxdriverproject.org; virtualization at 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
> -----Original Message----- > From: Greg KH [mailto:greg at kroah.com] > Sent: Tuesday, May 03, 2011 4:47 PM > To: KY Srinivasan > Cc: gregkh at suse.de; linux-kernel at vger.kernel.org; > devel at linuxdriverproject.org; virtualization at 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
> -----Original Message----- > From: Christoph Hellwig [mailto:hch at infradead.org] > Sent: Monday, May 09, 2011 10:34 AM > To: KY Srinivasan > Cc: Greg KH; gregkh at suse.de; linux-kernel at vger.kernel.org; > devel at linuxdriverproject.org; virtualization at 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
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.
> -----Original Message----- > From: Christoph Hellwig [mailto:hch at infradead.org] > Sent: Tuesday, May 10, 2011 1:24 AM > To: KY Srinivasan > Cc: Christoph Hellwig; Greg KH; gregkh at suse.de; linux-kernel at vger.kernel.org; > devel at linuxdriverproject.org; virtualization at 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