lkml.org 
[lkml]   [2011]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: Hyper-V vmbus driver
    Date


    > -----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



    \
     
     \ /
      Last update: 2011-04-25 04:17    [W:0.028 / U:1.556 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site