RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 4:22 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 09:16:29PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 1:10 AM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 02:50:00AM +, KY Srinivasan wrote: struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. There is not a struct driver_context in the code that I see today, or am I missing something? That's my objection here, please don't use that name, it's not valid for a subsystem to use, even for a tiny bit. Look at the file vmbus.h you will see struct driver_context. This has been there for as long as I have seen this code. Ok, I am rightly corrected, I totally missed that, you are right. Feel free to resend after addressing the other issues. I'll fix up the hv_mouse driver, you don't have to worry about that one if you don't want to, just ignore it please. Greg, I am working on a patch-set that hopefully will address all the concerns that were raised. As part of this effort, I will also deal with the mouse driver. I should have these patches out next week. Thanks for your patience here. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 1:10 AM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 02:50:00AM +, KY Srinivasan wrote: struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. There is not a struct driver_context in the code that I see today, or am I missing something? That's my objection here, please don't use that name, it's not valid for a subsystem to use, even for a tiny bit. Look at the file vmbus.h you will see struct driver_context. This has been there for as long as I have seen this code. Think of this as the surviving data structure after the hv_driver state is consolidated into (the existing) driver_context data structure. I did this in the spirit of doing one thing at a time. If I am going to be picking a more appropriate name for the consolidated data structure; I might as well pick the final name that we want this unified driver abstraction to be called. Your final name is fine, it's the intermediate one I'm objecting to. Ok. How about 'struct hv_driver_context' instead? I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. Based on your comments on intermediate names, would you recommend that as part of consolidating the driver abstractions, I also rename this combined state. Probably, if I understand what you are referring to. Please post code so that I really know what you are doing :) thanks, greg k-h Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
On Thu, Mar 03, 2011 at 09:16:29PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 1:10 AM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 02:50:00AM +, KY Srinivasan wrote: struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. There is not a struct driver_context in the code that I see today, or am I missing something? That's my objection here, please don't use that name, it's not valid for a subsystem to use, even for a tiny bit. Look at the file vmbus.h you will see struct driver_context. This has been there for as long as I have seen this code. Ok, I am rightly corrected, I totally missed that, you are right. Feel free to resend after addressing the other issues. I'll fix up the hv_mouse driver, you don't have to worry about that one if you don't want to, just ignore it please. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, March 02, 2011 12:41 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Wed, Mar 02, 2011 at 01:43:13AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. Think of this as the surviving data structure after the hv_driver state is consolidated into (the existing) driver_context data structure. I did this in the spirit of doing one thing at a time. If I am going to be picking a more appropriate name for the consolidated data structure; I might as well pick the final name that we want this unified driver abstraction to be called. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. Based on your comments on intermediate names, would you recommend that as part of consolidating the driver abstractions, I also rename this combined state. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
On Thu, Mar 03, 2011 at 02:50:00AM +, KY Srinivasan wrote: struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. There is not a struct driver_context in the code that I see today, or am I missing something? That's my objection here, please don't use that name, it's not valid for a subsystem to use, even for a tiny bit. Think of this as the surviving data structure after the hv_driver state is consolidated into (the existing) driver_context data structure. I did this in the spirit of doing one thing at a time. If I am going to be picking a more appropriate name for the consolidated data structure; I might as well pick the final name that we want this unified driver abstraction to be called. Your final name is fine, it's the intermediate one I'm objecting to. How about 'struct hv_driver_context' instead? I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. Based on your comments on intermediate names, would you recommend that as part of consolidating the driver abstractions, I also rename this combined state. Probably, if I understand what you are referring to. Please post code so that I really know what you are doing :) thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. Greg, would it be better if I just had one patch for dealing with all device related issues and a Separate patch for dealing all driver related issues? Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
On Wed, Mar 02, 2011 at 01:43:13AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. Greg, would it be better if I just had one patch for dealing with all device related issues and a Separate patch for dealing all driver related issues? Again, only do one thing per patch. So yes, of course they should be separate. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
