Re: [PATCH v1 1/9] drivers: hv: dxgkrnl: Driver initialization and creation of dxgadapter

From: Greg KH <gregkh@linuxfoundation.org>
To: Iouri Tarassov <iourit@linux.microsoft.com>
Cc: kys@microsoft.com, haiyangz@microsoft.com,
	sthemmin@microsoft.com, wei.liu@kernel.org,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
	spronovo@microsoft.com
Subject: Re: [PATCH v1 1/9] drivers: hv: dxgkrnl: Driver initialization and creation of dxgadapter
Date: Fri, 14 Jan 2022 06:40:53 +0100	[thread overview]
Message-ID: <YeEM5asW5XtqD0Pi@kroah.com> (raw)
In-Reply-To: <19e524e6-d0fc-20a7-03c6-c1094681a2a6@linux.microsoft.com>

On Thu, Jan 13, 2022 at 04:08:07PM -0800, Iouri Tarassov wrote:
> 
> On 1/12/2022 11:46 PM, Greg KH wrote:
> > On Wed, Jan 12, 2022 at 11:55:06AM -0800, Iouri Tarassov wrote:
> > > +	dev_dbg(dxgglobaldev, "%s: %x:%x %p %pUb\n",
> > > +		    __func__, adapter->luid.b, adapter->luid.a, hdev->channel,
> > > +		    &hdev->channel->offermsg.offer.if_instance);
> > 
> > When I see something like "global device pointer", that is a HUGE red
> > flag.
> > 
> > No driver should ever have anything that is static to the driver like
> > this, it should always be per-device.  Please use the correct device
> > model here, which does not include a global pointer, but rather unique
> > ones that are given to you by the driver core.  That way you are never
> > tied to only "one device per system" as that is a constraint that you
> > will have to fix eventually, might as well do it all correctly the first
> > time as it is not any extra effort to do so
> Hi Greg,
> 
> dxgglobaldev is a pointer to the global driver data. By design there is a
> single hyper-v VM bus and a single corresponding /dev/dxg device.

That's fine, but use the pointer that you create based on your bus
device, not some static pointer please.

> Virtual GPU adapters are present on the VM bus. /dev/dxg device is used
> to enumerate all virtual GPUs, which are accessible only via IOCTLs
> to /dev/dxg. dxgglobaldev has a list of all vGPU adapters and
> other global driver state. This follows the design on Windows where a single
> global object in dxgkrnl.sys driver is used to enumerate and access all
> GPU devices. This is also how the public D3DKMT interface to dxgkrnl is
> structured.

First off, remember this isn't Windows, let's not make the same mistakes
they have made there please :)

Secondly, this isn't the problem, the issue is that you have a
non-dynamic device here, which is not how Linux drivers should ever
work.  It's fine to have a "mux" device like this, but create it
properly, based on the device that the driver core gives you as you must
respect that lifetime, not the lifetime of a static pointer embedded in
a module.

thanks,

greg k-h