Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver
On Wed, Feb 1, 2023 at 10:34 AM Saurabh Singh Sengar wrote: > > On Wed, Feb 01, 2023 at 08:51:46AM -0600, Rob Herring wrote: > > On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote: > > > On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote: > > > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar > > > > wrote: > > > > > > > > > > This set of patches expands the VMBus driver to include device tree > > > > > support. > > > > > > > > > > The first two patches enable compilation of Hyper-V APIs in a non-ACPI > > > > > build. > > > > > > > > > > The third patch converts the VMBus driver from acpi to more generic > > > > > platform driver. > > > > > > > > > > Further to add device tree documentation for VMBus, it needs to club > > > > > with > > > > > other virtualization driver's documentation. For this rename the > > > > > virtio > > > > > folder to more generic hypervisor, so that all the hypervisor based > > > > > devices can co-exist in a single place in device tree documentation. > > > > > The > > > > > fourth patch does this renaming. > > > > > > > > > > The fifth patch introduces the device tree documentation for VMBus. > > > > > > > > > > The sixth patch adds device tree support to the VMBus driver. > > > > > Currently > > > > > this is tested only for x86 and it may not work for other archs. > > > > > > > > I can read all the patches and see *what* they do. You don't really > > > > need to list that here. I'm still wondering *why*. That is what the > > > > cover letter and commit messages should answer. Why do you need DT > > > > support? How does this even work on x86? FDT is only enabled for > > > > CE4100 platform. > > > > > > HI Rob, > > > > > > Thanks for your comments. > > > We are working on a solution where kernel is booted without ACPI tables > > > to keep > > > the overall system's memory footprints slim and possibly faster boot time. > > > We have tested this by enabling CONFIG_OF for x86. > > > > It's CONFIG_OF_EARLY_FLATTREE which you would need and that's not user > > selectable. At a minimum, you need some kconfig changes. Where are > > those? > > You are right we have define a new config flag in Kconfig, and selected > CONFIG_OF > and CONFIG_OF_EARLY_FLATTREE. We are working on upstreaming that patch as well > however that will be a separate patch series. Fair enough, but that should come first IMO. Really I just want to see a complete picture. That can be a reference to a git branch(es) or other patch series. But again, what I want to see in particular is the actual DT and validation run on it. > > Also see my comment on v1 about running DT validation on your dtb. I'm > > sure running it would point out other issues. Such as the root level > > comaptible string(s) need to be documented. You need cpu nodes, > > interrupt controller, timers, etc. Those all have to be documented. > > I will be changing the parent node to soc node as suggested by Krzysztof > in other thread. Another issue yes, but orthogonal to my comments. > > soc { > #address-cells = <2>; > #size-cells = <2>; You are missing 'ranges' here. Without it, addresses aren't translatable. You are also missing 'compatible = "simple-bus";'. This happens to work on x86 because of legacy reasons, but we don't want new cases added. > > vmbus@ff000 { > #address-cells = <2>; > #size-cells = <1>; > compatible = "Microsoft,vmbus"; 'Microsoft' is not a vendor prefix. > ranges = <0x00 0x00 0x0f 0xf000 0x1000>; > }; > }; > > This will be sufficient. All these comments are unnecessary because the tools will now check these things and we shouldn't have to. Rob ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/6] Device tree support for Hyper-V VMBus driver
On Tue, Jan 31, 2023 at 06:04:49PM -0800, Saurabh Singh Sengar wrote: > On Tue, Jan 31, 2023 at 02:27:51PM -0600, Rob Herring wrote: > > On Tue, Jan 31, 2023 at 12:10 PM Saurabh Sengar > > wrote: > > > > > > This set of patches expands the VMBus driver to include device tree > > > support. > > > > > > The first two patches enable compilation of Hyper-V APIs in a non-ACPI > > > build. > > > > > > The third patch converts the VMBus driver from acpi to more generic > > > platform driver. > > > > > > Further to add device tree documentation for VMBus, it needs to club with > > > other virtualization driver's documentation. For this rename the virtio > > > folder to more generic hypervisor, so that all the hypervisor based > > > devices can co-exist in a single place in device tree documentation. The > > > fourth patch does this renaming. > > > > > > The fifth patch introduces the device tree documentation for VMBus. > > > > > > The sixth patch adds device tree support to the VMBus driver. Currently > > > this is tested only for x86 and it may not work for other archs. > > > > I can read all the patches and see *what* they do. You don't really > > need to list that here. I'm still wondering *why*. That is what the > > cover letter and commit messages should answer. Why do you need DT > > support? How does this even work on x86? FDT is only enabled for > > CE4100 platform. > > HI Rob, > > Thanks for your comments. > We are working on a solution where kernel is booted without ACPI tables to > keep > the overall system's memory footprints slim and possibly faster boot time. > We have tested this by enabling CONFIG_OF for x86. It's CONFIG_OF_EARLY_FLATTREE which you would need and that's not user selectable. At a minimum, you need some kconfig changes. Where are those? Also see my comment on v1 about running DT validation on your dtb. I'm sure running it would point out other issues. Such as the root level comaptible string(s) need to be documented. You need cpu nodes, interrupt controller, timers, etc. Those all have to be documented. Rob ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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
Hyper-V vmbus driver
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. 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 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