Re: [RFC PATCH 3/9] cxl/mem: Add a driver for the type-3 mailbox

From: Randy Dunlap <rdunlap@infradead.org>
To: Dan Williams <dan.j.williams@intel.com>,
	Christoph Hellwig <hch@infradead.org>
Cc: Ben Widawsky <ben.widawsky@intel.com>,
	linux-cxl@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux PCI <linux-pci@vger.kernel.org>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	Ira Weiny <ira.weiny@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	"Kelley, Sean V" <sean.v.kelley@intel.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [RFC PATCH 3/9] cxl/mem: Add a driver for the type-3 mailbox
Date: Wed, 11 Nov 2020 13:41:47 -0800	[thread overview]
Message-ID: <4a8b5a64-7ba0-a275-744f-6642f98e2213@infradead.org> (raw)
In-Reply-To: <CAPcyv4iA_hNc=xdcbR-eb57W9o4br1BognSr5Sj4pAO3uMm69g@mail.gmail.com>

On 11/11/20 9:17 AM, Dan Williams wrote:
> On Tue, Nov 10, 2020 at 11:12 PM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Tue, Nov 10, 2020 at 09:43:50PM -0800, Ben Widawsky wrote:
>>> +config CXL_MEM
>>> +        tristate "CXL.mem Device Support"
>>> +        depends on PCI && CXL_BUS_PROVIDER != n
>>
>> depend on PCI && CXL_BUS_PROVIDER
>>
>>> +        default m if CXL_BUS_PROVIDER
>>
>> Please don't set weird defaults for new code.  Especially not default
>> to module crap like this.
> 
> This goes back to what people like Dave C. asked for LIBNVDIMM / DAX,
> a way to blanket turn on a subsystem without needing to go hunt down
> individual configs. All of CXL is "default n", but if someone turns on
> a piece of it they get all of it by default. The user can then opt-out
> on pieces after that first opt-in. If there's a better way to turn on
> suggested configs I'm open to switch to that style. As for the
> "default m" I was worried that it would be "default y" without the
> specificity, but I did not test that... will check. There have been
> times when I wished that distros defaulted bleeding edge new enabling
> to 'm' and putting that default in the Kconfig maybe saves me from
> needing to file individual config changes to distros after the fact.

What we as developers put into mainline kernel Kconfig files has nothing
to do with what distros use in their distro config files.
Or at least it shouldn't.  Maybe your experience has been different.

>>
>>> +// Copyright(c) 2020 Intel Corporation. All rights reserved.
>>
>> Please don't use '//' for anything but the SPDX header.
> 
> Ok, I find // following by /* */ a bit ugly, but I don't care enough to fight.
> 

Hm, it's not in coding-style AFAICT but Linus has OK-ed C99 style comments:
http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

>>> +MODULE_AUTHOR("Intel Corporation");
>>
>> A module author is not a company.
> 
> At least I don't have a copyright assignment clause, I don't agree
> with the vanity of listing multiple people here especially when
> MAINTAINERS has the contact info, and I don't want to maintain a list
> as people do drive-by contributions and we need to figure out at what
> level of contribution mandates a new MODULE_AUTHOR line. Now, that
> said I would be ok to duplicate the MAINTAINERS as MODULE_AUTHOR
> lines, but I otherwise expect MAINTAINERS is the central source for
> module contact info.

Sure, MAINTAINERS is fine, but the MODULE_AUTHOR() above provides
no useful information.
Even saying (made up) linux-devel@linux.intel.com would be slightly better,
but some kind of contact info would be great. Otherwise just delete that line.

-- 
~Randy