RE: [PATCH 01/20] dlb2: add skeleton for DLB 2.0 driver

From: "Eads, Gage" <gage.eads@intel.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"arnd@arndb.de" <arnd@arndb.de>,
	"Karlsson, Magnus" <magnus.karlsson@intel.com>,
	"Topel, Bjorn" <bjorn.topel@intel.com>
Subject: RE: [PATCH 01/20] dlb2: add skeleton for DLB 2.0 driver
Date: Fri, 17 Jul 2020 18:19:14 +0000	[thread overview]
Message-ID: <SN6PR11MB25742456A7DA3C99F88FA77FF67C0@SN6PR11MB2574.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200712155631.GB186665@kroah.com>

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Sunday, July 12, 2020 10:57 AM
> To: Eads, Gage <gage.eads@intel.com>
> Cc: linux-kernel@vger.kernel.org; arnd@arndb.de; Karlsson, Magnus
> <magnus.karlsson@intel.com>; Topel, Bjorn <bjorn.topel@intel.com>
> Subject: Re: [PATCH 01/20] dlb2: add skeleton for DLB 2.0 driver
> 
> On Sun, Jul 12, 2020 at 08:43:12AM -0500, Gage Eads wrote:
> > +config INTEL_DLB2
> > +       tristate "Intel(R) Dynamic Load Balancer 2.0 Driver"
> > +       depends on 64BIT && PCI && X86
> 
> Why just that platform?  What about CONFIG_TEST for everything else?

This device will only appear on an x86 platform. CONFIG_COMPILE_TEST won't work, since the driver uses the x86-only function iosubmit_cmds512().

> 
> > +       help
> > +         This driver supports the Intel(R) Dynamic Load Balancer 2.0 (DLB 2.0)
> > +         device.
> 
> Are you sure you need the (R) in Kconfig texts everywhere?

The second is probably overkill. Just the first one is required.

> 
> And a bit more info here would be nice, as no one knows if they have this or
> not, right?

Intel hasn't yet announced more information that I can include here. For now, "lspci -d 8086:2710" will tell the user if this device is present.

> 
> > --- /dev/null
> > +++ b/drivers/misc/dlb2/dlb2_hw_types.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> 
> Why dual licensed?  I thought that Intel told me they were not going to do
> that anymore for any kernel code going forward as it was just such a pain and
> never actually helped anything.  Has that changed?
> 

The driver is mostly GPLv2-only, but a subset constitutes a "hardware access library" that is almost completely OS-independent. "almost" because it has calls to non-GPL symbols like kmalloc() and kfree(). This dual-licensed portion can be ported to other environments that need the more permissive BSD license.

For the broader policy question, Intel's open source team will get back to you on this.

> 
> > + * Copyright(c) 2016-2020 Intel Corporation */
> > +
> > +#ifndef __DLB2_HW_TYPES_H
> > +#define __DLB2_HW_TYPES_H
> > +
> > +#define DLB2_MAX_NUM_VDEVS 16
> > +#define DLB2_MAX_NUM_DOMAINS 32
> > +#define DLB2_MAX_NUM_LDB_QUEUES 32 /* LDB == load-balanced */
> #define
> > +DLB2_MAX_NUM_DIR_QUEUES 64 /* DIR == directed */ #define
> > +DLB2_MAX_NUM_LDB_PORTS 64 #define DLB2_MAX_NUM_DIR_PORTS
> > +DLB2_MAX_NUM_DIR_QUEUES #define DLB2_MAX_NUM_LDB_CREDITS
> 8192 #define
> > +DLB2_MAX_NUM_DIR_CREDITS 2048 #define
> DLB2_MAX_NUM_HIST_LIST_ENTRIES
> > +2048 #define DLB2_MAX_NUM_AQED_ENTRIES 2048 #define
> > +DLB2_MAX_NUM_QIDS_PER_LDB_CQ 8 #define
> > +DLB2_MAX_NUM_SEQUENCE_NUMBER_GROUPS 2 #define
> > +DLB2_MAX_NUM_SEQUENCE_NUMBER_MODES 5 #define
> DLB2_QID_PRIORITIES 8
> > +#define DLB2_NUM_ARB_WEIGHTS 8 #define DLB2_MAX_WEIGHT 255
> #define
> > +DLB2_NUM_COS_DOMAINS 4 #define DLB2_MAX_CQ_COMP_CHECK_LOOPS
> 409600
> > +#define DLB2_MAX_QID_EMPTY_CHECK_LOOPS (32 * 64 * 1024 * (800 / 30))
> > +#define DLB2_HZ 800000000
> 
> No tabs?  How easy is that to read?  :(

I'll improve this (and a few other instances in later patches) in V2.

Thanks,
Gage

> 
> greg k-h