Re: Hyper-V vmbus driver

Re: Hyper-V vmbus driver

[Greg KH]

On Mon, Apr 11, 2011 at 06:46:24PM +0000, 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

RE: Hyper-V vmbus driver

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Monday, April 11, 2011 3:07 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org
> Subject: Re: Hyper-V vmbus driver
> 
> On Mon, Apr 11, 2011 at 06:46:24PM +0000, 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. 

Regards,

K. Y
> 
> thanks,
> 
> greg k-h

Re: Hyper-V vmbus driver

[Greg KH]

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

RE: Hyper-V vmbus driver

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Saturday, April 23, 2011 11:21 AM
> To: Greg KH
> Cc: KY Srinivasan; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@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

Re: Hyper-V vmbus driver

[Greg KH]

On Sun, Apr 24, 2011 at 04:18:24PM +0000, 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

RE: Hyper-V vmbus driver

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Sunday, April 24, 2011 8:14 PM
> To: KY Srinivasan
> Cc: Greg KH; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org;
> virtualization@lists.osdl.org
> Subject: Re: Hyper-V vmbus driver
> 
> On Sun, Apr 24, 2011 at 04:18:24PM +0000, 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
> 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 for this detailed mail Greg. As I am writing this email, I have pretty much 
completed the code for much of what we have discussed here. These are on top
of my patches that are yet to be applied (the ones that I sent on April 22). Since some of 
these changes also affect netvsc code and Haiyang had sent some patches to deal with 
forward declarations in the netvsc code, I have locally applied haiyang's patches. I will send you 
these patches soon.

Regards,

K. Y 
> 
> thanks,
> 
> greg k-h

Re: Hyper-V vmbus driver

[Greg KH]

On Mon, Apr 25, 2011 at 02:15:47AM +0000, KY Srinivasan wrote:
> > On Sun, Apr 24, 2011 at 04:18:24PM +0000, 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
> > >        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 for this detailed mail Greg. As I am writing this email, I have
> pretty much completed the code for much of what we have discussed
> here. These are on top of my patches that are yet to be applied (the
> ones that I sent on April 22). Since some of these changes also affect
> netvsc code and Haiyang had sent some patches to deal with forward
> declarations in the netvsc code, I have locally applied haiyang's
> patches. I will send you these patches soon.

Great.

thanks,

greg k-h