Message-ID: <YiJSh4WRmVdWvUc8@kroah.com>
Date:   Fri, 4 Mar 2022 18:55:19 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Wei Liu <wei.liu@...nel.org>
Cc:     Iouri Tarassov <iourit@...ux.microsoft.com>, kys@...rosoft.com,
        haiyangz@...rosoft.com, sthemmin@...rosoft.com,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
        spronovo@...rosoft.com, spronovo@...ux.microsoft.com
Subject: Re: [PATCH v3 02/30] drivers: hv: dxgkrnl: Driver initialization and
 loading

On Fri, Mar 04, 2022 at 04:04:10PM +0000, Wei Liu wrote:
> > > A PCI device device is created for each virtual compute device.
> > > Therefore, there should be a global list of objects and a mutex to
> > > synchronize access to the list.
> > 
> > Woah, what?  You create a fake PCI device for each virtual device?  If
> > so, great, then you are now a PCI bus and create the PCI devices
> > properly so that the PCI core can handle and manage them and then
> > assign them to your driver.  You should NEVER have a global list of
> > these devices, as that is what the driver model should be managing.
> > Not you!
> > 
> 
> No, there is no fake PCI device. The device object is still coming from
> the PCI core driver. There is code to match against PCI vendor ID and
> device ID, and follow the usual way of managing PCI device.

So it is a real PCI device?  Why the confusion?

> Iouri understands device specific state should be encapsulated in the
> private data field in their respective device. And I believe the code
> can perhaps be rewritten to better conform to Linux kernel's model.

It has to follow the Linux kernel model, to think otherwise is quite
odd.

> > > IO space is shared by all compute devices, so its parameters should
> > > be global.
> > 
> > Huh?  If that's the case then you have bigger problems.  Use the aux
> > bus for devices that share io space.  That is what it was created for,
> > do not ignore the functionality that Linux already provides you by
> > trying to go around it and writing your own code.  Use the frameworks
> > we have already debugged and support.  This is why your Linux driver
> > should be at least 1/3 smaller than drivers for other operating
> > systems.
> > 
> 
> To be fair, auxiliary bus was only added in 5.11, while this series was
> written long before that. Unfortunately one only has so much time to
> follow Linux kernel development closely. I admit this is the first time
> I hear about it. :-)

5.11 was over a year ago.  We do not take "old" drivers into the kernel
tree, and if this series has not been touched for over a year, um, you
all have bigger problems here and are just wasting our time :(

> > Then fix this.  Make your compute devices store the needed information
> > when they are created.  Again, we have loads of examples in the kernel,
> > this is nothing new.
> > 
> 
> At this point, I think Iouri and I have settled on more encapsulation is
> needed. Yet there is something I don't know how to square yet. That is,
> devices (either from vmbus or pci) don't form a clear hierarchy.

That is because devices don't care where they are in the larger kernel
tree of devices, they are independant.  To try to claim there is a
needed hierarchy is quite odd and will cause lots of problems when that
heirachy changes by the underlying system.

> If
> there isn't a linked list or some sort to organize them it would be
> difficult to cross-reference.

You should never be touching another device from another one.  There are
a few rare exceptions but they are rare and you should not use them at
all.  And if you do have to do so, you better get the reference counting
logic correct.

good luck!

greg k-h

Powered by blists - more mailing lists