linux-kernel - Re: [PATCH 02/14] cxl/mem: Map memory device registers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Feb 2021 10:31:51 -0800
From:   Ben Widawsky <ben.widawsky@...el.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     linux-cxl@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-nvdimm@...ts.01.org,
        linux-pci@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>,
        Chris Browy <cbrowy@...ry-design.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Jon Masters <jcm@...masters.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Vishal Verma <vishal.l.verma@...el.com>,
        daniel.lll@...baba-inc.com,
        "John Groves (jgroves)" <jgroves@...ron.com>,
        "Kelley, Sean V" <sean.v.kelley@...el.com>
Subject: Re: [PATCH 02/14] cxl/mem: Map memory device registers

On 21-02-02 18:04:41, Christoph Hellwig wrote:
> Any reason not to merge a bunch of patches?  Both this one and
> the previous one are rather useless on their own, making review
> harder than necessary.
> 

As this is an initial driver, there's obviously no functional/regression testing
value in separating the patches.

This was the way we brought up the driver and how we verified functionality
incrementally. I see value in both capturing that in the history, as well as
making review a bit easier (which apparently failed for you).

> > + * cxl_mem_create() - Create a new &struct cxl_mem.
> > + * @pdev: The pci device associated with the new &struct cxl_mem.
> > + * @reg_lo: Lower 32b of the register locator
> > + * @reg_hi: Upper 32b of the register locator.
> > + *
> > + * Return: The new &struct cxl_mem on success, NULL on failure.
> > + *
> > + * Map the BAR for a CXL memory device. This BAR has the memory device's
> > + * registers for the device as specified in CXL specification.
> > + */
> 
> A lot of text with almost no value over just reading the function.
> What's that fetish with kerneldoc comments for trivial static functions?
> 
> > +		reg_type =
> > +			(reg_lo >> CXL_REGLOC_RBI_SHIFT) & CXL_REGLOC_RBI_MASK;
> 
> OTOH this screams for a helper that would make the code a lot more
> self documenting.
> 

I agree, I'll change this.

> > +		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > +			rc = 0;
> > +			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
> > +			if (!cxlm)
> > +				rc = -ENODEV;
> > +			break;
> 
> And given that we're going to grow more types eventually, why not start
> out with a switch here?  Also why return the structure when nothing
> uses it?

 We've (Intel) already started working on the libnvdimm integration which does
 change this around a bit. In order to go with what's best tested though, I've
 chosen to use this as is for merge. Many different people not just in Intel
 have tested these codepaths. The resulting code moves the check on register
 type out of this function entirely.

 If you'd like me to make it a switch, I can, but it's going to be extracted
 later anyway.