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

[Greg KH]

On Fri, Jul 17, 2020 at 06:19:14PM +0000, Eads, Gage wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Sunday, July 12, 2020 10:57 AM
> > To: Eads, Gage <gage.eads@xxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; Karlsson, Magnus
> > <magnus.karlsson@xxxxxxxxx>; Topel, Bjorn <bjorn.topel@xxxxxxxxx>
> > 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().

Please wrap your lines correctly...

Anyway, there is no config option for that function that you can trigger
off of?

> > > +       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.

Really?  I would just drop it.  Unless you get a signed-off-by from a
lawyer saying it 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.

That's fine, but we can't take a 1 sentance help text, that means
nothing.

> > > --- /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.

Then put that "OS independant" part as a separate file, with a separate
license.  You all know how to do this properly, don't mix this stuff up.

But even then, I would drop such a library as that's not going to make a
good Linux driver, we do not like, or need, such things in the kernel.

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

Wonderful, when will that happen?

thanks,

greg k-h