RE: Hyper-V vmbus driver

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxx]
> Sent: Sunday, April 24, 2011 8:14 PM
> To: KY Srinivasan
> Cc: Greg KH; devel@xxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> virtualization@xxxxxxxxxxxxxx
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/