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

From: Dan Williams <dan.j.williams@intel.com>
To: 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 09:17:37 -0800	[thread overview]
Message-ID: <CAPcyv4iA_hNc=xdcbR-eb57W9o4br1BognSr5Sj4pAO3uMm69g@mail.gmail.com> (raw)
In-Reply-To: <20201111071231.GC7829@infradead.org>

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.

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

>
> > +
> > +             pci_read_config_word(pdev, pos + PCI_DVSEC_VENDOR_OFFSET, &vendor);
> > +             pci_read_config_word(pdev, pos + PCI_DVSEC_ID_OFFSET, &id);
> > +             if (vendor == PCI_DVSEC_VENDOR_CXL && dvsec == id)
> > +                     return pos;
> > +
> > +             pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC);
>
> Overly long lines again.

I thought 100 is the new 80 these days?

> > +static void cxl_mem_remove(struct pci_dev *pdev)
> > +{
> > +}
>
> No need for the empty remove callback.

True, will fix.

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