RE: Hyper-V vmbus driver
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Saturday, April 23, 2011 11:21 AM To: Greg KH Cc: KY Srinivasan; de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V vmbus driver On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. Thanks Greg. The latest re-send of my hv patches are against the tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git that I picked up on April 22, 2011. I hope there won't be any issues this time around. But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. Are you referring to the fact that in the vmbus_match function, the current code binds the device specific driver to the corresponding hv_device structure? See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. I am pretty sure, I can get rid of this. The way this code was originally structured, in the vmbus_match() function, you needed to get at the device specific driver pointer so that we could do the binding between the hv_device and the correspond device specific driver. The earlier code depended on the structure layout to map a pointer to the hv_driver to the corresponding device specific driver (net, block etc.) To get rid of this layout dependency, I introduced an addition field (priv) in the hv_driver. There is, I suspect sufficient state available to: (a) Not require the vmbus_match() function to do the binding. (b) And to get at the device specific driver structure from the generic driver structure without having to have an explicit mapping maintained in the hv_driver structure. Before, I go ahead and make these changes, Greg, can you confirm if I have captured your concerns correctly. You should be able to set that structure constant, like all other busses. Right now you can not which shows a design issue. I am a little confused here. While I agree with you that perhaps we could get rid the priv element in the hv_driver structure, what else would you want done here. So, take a look at that and let me know what you think. Once I hear from you, I will work on getting rid of the priv pointer from hv_driver structure as well as the code that currently does the binding in vmbus_match. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Hyper-V vmbus driver
On Sun, Apr 24, 2011 at 04:18:24PM +, KY Srinivasan wrote: On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. Thanks Greg. The latest re-send of my hv patches are against the tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git that I picked up on April 22, 2011. I hope there won't be any issues this time around. Me too :) But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. Are you referring to the fact that in the vmbus_match function, the current code binds the device specific driver to the corresponding hv_device structure? Yes, that's the problem (well, kind of the problem.) You seem to be doing things a bit odd and that's due to the old way the code was written. First off, don't embed a struct bus_type in another structure, that's not needed at all. Why is that done? Anyway... In your vmbus_match function, you should be matching to see if your device matches the driver that is passed to you. You do this by looking at some type of id. For the vmbus you should do this by looking at the GUID, right? And it looks like you do do this, so that's fine. And then your vmbus_probe() function calls the driver probe function, with the device it is to bind to. BUT, you need to have your probe function pass in the correct device type (i.e. struct hv_device, NOT struct device.) That way, your hv_driver will have a type all its own, with probe functions that look nothing like the probe functions that 'struct driver' has in it. Look at 'struct pci_driver' for an example of this. Don't try to overload the probe/remove/suspend/etc functions of your hv_driver by using the base 'struct device_driver' callbacks, that's putting knowledge of the driver core into the individual hv drivers, where it's not needed at all. And, by doing that, you should be able to drop your private pointer in the hv_driver function completly, right? That shouldn't be needed at all. See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. I am pretty sure, I can get rid of this. The way this code was originally structured, in the vmbus_match() function, you needed to get at the device specific driver pointer so that we could do the binding between the hv_device and the correspond device specific driver. The earlier code depended on the structure layout to map a pointer to the hv_driver to the corresponding device specific driver (net, block etc.) To get rid of this layout dependency, I introduced an addition field (priv) in the hv_driver. There is, I suspect sufficient state available to: (a) Not require the vmbus_match() function to do the binding. No, you still want that, see above. (b) And to get at the device specific driver structure from the generic driver structure without having to have an explicit mapping maintained in the hv_driver structure. Kind of, see above for more details. If you want a good example, again, look at the PCI core code, it's pretty simple in this area (hint, don't look at the USB code, it does much more complex things than you want, due to things that the USB bus imposes on devices, that's never a good example to look at.) Hope this helps. Please let me know if it doesn't :) thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V vmbus driver
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Sunday, April 24, 2011 8:14 PM To: KY Srinivasan Cc: Greg KH; de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V vmbus driver On Sun, Apr 24, 2011 at 04:18:24PM +, KY Srinivasan wrote: On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. Thanks Greg. The latest re-send of my hv patches are against the tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git that I picked up on April 22, 2011. I hope there won't be any issues this time around. Me too :) Just curious; when are you planning to drain the hv patch queue next. But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. Are you referring to the fact that in the vmbus_match function, the current code binds the device specific driver to the corresponding hv_device structure? Yes, that's the problem (well, kind of the problem.) You seem to be doing things a bit odd and that's due to the old way the code was written. First off, don't embed a struct bus_type in another structure, that's not needed at all. Why is that done? Anyway... Currently, struct bus_type is embedded in struct hv_bus that has very minimal additional state. I will clean this up. In your vmbus_match function, you should be matching to see if your device matches the driver that is passed to you. You do this by looking at some type of id. For the vmbus you should do this by looking at the GUID, right? And it looks like you do do this, so that's fine. And then your vmbus_probe() function calls the driver probe function, with the device it is to bind to. BUT, you need to have your probe function pass in the correct device type (i.e. struct hv_device, NOT struct device.) I will clean this up. That way, your hv_driver will have a type all its own, with probe functions that look nothing like the probe functions that 'struct driver' has in it. Look at 'struct pci_driver' for an example of this. Don't try to overload the probe/remove/suspend/etc functions of your hv_driver by using the base 'struct device_driver' callbacks, that's putting knowledge of the driver core into the individual hv drivers, where it's not needed at all. And, by doing that, you should be able to drop your private pointer in the hv_driver function completly, right? That shouldn't be needed at all. After sending you the mail this afternoon, I worked on patches that do exactly that. I did this with the current model where probe/remove/ etc. get a pointer to struct device. Within a specific driver you can always map a struct device pointer to the class specific device driver. I will keep that code; I will however do what you are suggesting here and make probe/remove etc. take a pointer to struct hv_device. See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. I am pretty sure, I can get rid of this. The way this code was originally structured, in the vmbus_match() function, you needed to get at the device specific driver pointer so that we could do the binding between the hv_device and the correspond device specific driver. The earlier code depended on the structure layout to map a pointer to the hv_driver to the corresponding device specific driver (net, block etc.) To get rid of this layout dependency, I introduced an addition field (priv) in the hv_driver. There is, I suspect sufficient state available to: (a) Not require the vmbus_match() function to do the binding. No, you still want that, see above. The current code has the following assignment after a match is found: device_ctx-drv = drv-priv; What I meant was that I would get rid of this assignment (binding) since I can get that information quite easily in the class specific (net, block, etc.) where it is needed. (b) And to get at the device specific driver structure from the generic driver structure without having to have an explicit mapping maintained in the hv_driver structure. Kind of, see above for more details. If you want a good example, again, look at the PCI core code, it's
Re: Hyper-V vmbus driver
On Mon, Apr 25, 2011 at 02:15:47AM +, KY Srinivasan wrote: On Sun, Apr 24, 2011 at 04:18:24PM +, KY Srinivasan wrote: On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. Thanks Greg. The latest re-send of my hv patches are against the tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git that I picked up on April 22, 2011. I hope there won't be any issues this time around. Me too :) Just curious; when are you planning to drain the hv patch queue next. When I get a chance to get to it :) But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. Are you referring to the fact that in the vmbus_match function, the current code binds the device specific driver to the corresponding hv_device structure? Yes, that's the problem (well, kind of the problem.) You seem to be doing things a bit odd and that's due to the old way the code was written. First off, don't embed a struct bus_type in another structure, that's not needed at all. Why is that done? Anyway... Currently, struct bus_type is embedded in struct hv_bus that has very minimal additional state. I will clean this up. Thanks. In your vmbus_match function, you should be matching to see if your device matches the driver that is passed to you. You do this by looking at some type of id. For the vmbus you should do this by looking at the GUID, right? And it looks like you do do this, so that's fine. And then your vmbus_probe() function calls the driver probe function, with the device it is to bind to. BUT, you need to have your probe function pass in the correct device type (i.e. struct hv_device, NOT struct device.) I will clean this up. Thanks. That way, your hv_driver will have a type all its own, with probe functions that look nothing like the probe functions that 'struct driver' has in it. Look at 'struct pci_driver' for an example of this. Don't try to overload the probe/remove/suspend/etc functions of your hv_driver by using the base 'struct device_driver' callbacks, that's putting knowledge of the driver core into the individual hv drivers, where it's not needed at all. And, by doing that, you should be able to drop your private pointer in the hv_driver function completly, right? That shouldn't be needed at all. After sending you the mail this afternoon, I worked on patches that do exactly that. I did this with the current model where probe/remove/ etc. get a pointer to struct device. Within a specific driver you can always map a struct device pointer to the class specific device driver. I will keep that code; I will however do what you are suggesting here and make probe/remove etc. take a pointer to struct hv_device. Great. See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. I am pretty sure, I can get rid of this. The way this code was originally structured, in the vmbus_match() function, you needed to get at the device specific driver pointer so that we could do the binding between the hv_device and the correspond device specific driver. The earlier code depended on the structure layout to map a pointer to the hv_driver to the corresponding device specific driver (net, block etc.) To get rid of this layout dependency, I introduced an addition field (priv) in the hv_driver. There is, I suspect sufficient state available to: (a) Not require the vmbus_match() function to do the binding. No, you still want that, see above. The current code has the following assignment after a match is found: device_ctx-drv = drv-priv; What I meant was that I would get rid of this assignment (binding) since I can get that information quite easily in the class specific (net, block, etc.) where it is needed. Yes, that is good as it is not needed. It's also a flaw in that you would not allow multiple devices attached to the same driver, but as you can't run this bus that way, it was never noticed. (b) And to get at the device specific driver structure from the generic driver structure without having to have an explicit mapping
Re: Hyper-V vmbus driver
On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: With that patch-set, I think I have addressed all architectural issues that I am aware of. I was wondering if you would have the time to let me know what else would have to be addressed in the vmbus driver, before it could be considered ready for exiting staging. As always your help is greatly appreciated. Anyway, yes, I discussed this with Hank last week at the LF Collab summit. I'll look at the vmbus code later this week when I catch up on all of my other work (stable, usb, tty, staging, etc.) that has piled up during my 2 week absence, and get back to you with what I feel is still needed to be done, if anything. Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. You should be able to set that structure constant, like all other busses. Right now you can not which shows a design issue. So, take a look at that and let me know what you think. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Hyper-V vmbus driver
On Mon, Apr 11, 2011 at 06:46:24PM +, KY Srinivasan wrote: Greg, Recently, you applied a patch-set from me that cleaned a bunch of architectural issues in the vmbus driver. With that patch-set, I think I have addressed all architectural issues that I am aware of. I was wondering if you would have the time to let me know what else would have to be addressed in the vmbus driver, before it could be considered ready for exiting staging. As always your help is greatly appreciated. Hm, interesting word wrapping there, might I consider a real email client one of these days? :) Anyway, yes, I discussed this with Hank last week at the LF Collab summit. I'll look at the vmbus code later this week when I catch up on all of my other work (stable, usb, tty, staging, etc.) that has piled up during my 2 week absence, and get back to you with what I feel is still needed to be done, if anything. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
