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

From: Greg KH
Date: Fri Jan 14 2022 - 00:40:59 EST


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