Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code

From: Greg KH <gregkh@linuxfoundation.org>
To: Iouri Tarassov <iourit@linux.microsoft.com>
Cc: Sasha Levin <sashal@kernel.org>,
	kys@microsoft.com, haiyangz@microsoft.com,
	sthemmin@microsoft.com, wei.liu@kernel.org, iourit@microsoft.com,
	linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Subject: Re: [PATCH 1/4] drivers: hv: dxgkrnl: core code
Date: Fri, 28 Aug 2020 08:01:27 +0200	[thread overview]
Message-ID: <20200828060127.GA56396@kroah.com> (raw)
In-Reply-To: <58011269-e910-b3e4-2a3c-552b08c90574@linux.microsoft.com>

On Thu, Aug 27, 2020 at 04:29:59PM -0700, Iouri Tarassov wrote:
> On 8/14/2020 5:55 AM, Greg KH wrote:
> > On Fri, Aug 14, 2020 at 08:38:53AM -0400, Sasha Levin wrote:
> > > Add support for a Hyper-V based vGPU implementation that exposes the
> > > DirectX API to Linux userspace.
> > > > Signed-off-by: Sasha Levin <sashal@kernel.org>
> > 
> > Better, but what is this mess:
> > 
> > > +#define ISERROR(ret)					(ret < 0)
> 
> The VM bus messages return the NTSTATUS error code from the host. NTSTATUS
> is integer and the positive values indicate success.
> The success error code needs to be returned by IOCTLs to the DxCore
> applications. The ISERROR() macro is used in places where the NTSTATUS
> success code could be returned from a function. "if (ret)" cannot be used. I
> will add a comment to the macro in the next patch.

No, please just "open code" this, there is no need for a macro that is
actually more characters than the original test.

> > > +#define DXGKDEBUG 1
> > > +/* #define USEPRINTK 1 */
> > > +
> > > +#ifndef DXGKDEBUG
> > > +#define TRACE_DEBUG(...)
> > > +#define TRACE_DEFINE(...)
> > > +#define TRACE_FUNC_ENTER(...)
> > > +#define TRACE_FUNC_EXIT(...)
> > 
> > No, please do not to custom "trace" printk messages, that is what ftrace
> > is for, no individual driver should ever need to do that.
> > 
> > Just use the normal dev_*() calls for error messages and the like, do
> > not try to come up with a custom tracing framework for one tiny
> > individual driver.  If every driver in kernel did that, we would have a
> > nightmare...
> I understand the concern. This will be fixed later when we learn and pick
> the final tracing technology for the driver.

When is "later"?  We don't want to review something that you do not
think is ready to be merged, do it now so we don't trip over things that
you know you want to fix up.

> The current implementation
> allows the hardware vendors to quickly identify issues without learning
> about ftrace. They just need to enable the WSL debug console and mount
> debugfs.

Hardware vendors who know Linux already know about ftrace, don't make
people have to read up and learn about custom ways to debug
just-your-tiny-individual-driver.  Instead follow the customs and ways
the _WHOLE_ kernel works, that is just polite, don't you think?

thanks,

greg k-h