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

[Dan Williams]

On Wed, Nov 11, 2020 at 1:42 PM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
>
> On 11/11/20 9:17 AM, Dan Williams wrote:
> > On Tue, Nov 10, 2020 at 11:12 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> 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.

I agree with that sentiment, but walk it back through the requirement
I mentioned above... *if* we want a top-level CXL option (default n)
that goes and enables many CXL sub-options the default for those
sub-options is something that needs to be listed in the Kconfig. 'm'
is more flexible than 'y', so if a user wants CXL at all, and doesn't
care about how, I'd prefer it's 'm' rather than 'y'.

I have had to go submit distro config fixes when Kconfig defaulted to
'y' when 'm' was available, and the reasoning for why it was 'y' was
"oh, that was the Kconfig default when I flipped this other option".


> >>> +// 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@xxxxxxxxxxxxxxx would be slightly better,
> but some kind of contact info would be great. Otherwise just delete that line.

True, if the goal is to allow random end users to email support
questions about this module I'd rather not put my email there.
Instead, if it's someone that has kernel development questions then
they should be able to use MAINTAINERS for that contact.