Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Feb 2021 17:15:34 +0000
From:   Christoph Hellwig <hch@infradead.org>
To:     Ben Widawsky <ben.widawsky@intel.com>
Cc:     Christoph Hellwig <hch@infradead.org>, linux-cxl@vger.kernel.org,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-nvdimm@lists.01.org, linux-pci@vger.kernel.org,
        Bjorn Helgaas <helgaas@kernel.org>,
        Chris Browy <cbrowy@avery-design.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Jon Masters <jcm@jonmasters.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Rafael Wysocki <rafael.j.wysocki@intel.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        Vishal Verma <vishal.l.verma@intel.com>,
        daniel.lll@alibaba-inc.com,
        "John Groves (jgroves)" <jgroves@micron.com>,
        "Kelley, Sean V" <sean.v.kelley@intel.com>
Subject: Re: [PATCH 03/14] cxl/mem: Find device capabilities
Message-ID: <20210203171534.GB4104698@infradead.org>
References: <20210130002438.1872527-1-ben.widawsky@intel.com>
 <20210130002438.1872527-4-ben.widawsky@intel.com>
 <20210202181016.GD3708021@infradead.org>
 <20210202182418.3wyxnm6rqeoeclu2@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210202182418.3wyxnm6rqeoeclu2@intel.com>
Precedence: bulk

On Tue, Feb 02, 2021 at 10:24:18AM -0800, Ben Widawsky wrote:
> > > +	/* Cap 4000h - CXL_CAP_CAP_ID_MEMDEV */
> > > +	struct {
> > > +		void __iomem *regs;
> > > +	} mem;
> > 
> > This style looks massively obsfucated.  For one the comments look like
> > absolute gibberish, but also what is the point of all these anonymous
> > structures?
> 
> They're not anonymous, and their names are for the below register functions. The
> comments are connected spec reference 'Cap XXXXh' to definitions in cxl.h. I can
> articulate that if it helps.

But why no simply a

	void __iomem *mem_regs;

field vs the extra struct?

> The register space for CXL devices is a bit weird since it's all subdivided
> under 1 BAR for now. To clearly distinguish over the different subregions, these
> helpers exist. It's really easy to mess this up as the developer and I actually
> would disagree that it makes debugging quite a bit easier. It also gets more
> convoluted when you add the other 2 BARs which also each have their own
> subregions.
> 
> For example. if my mailbox function does:
> cxl_read_status_reg32(cxlm, CXLDEV_MB_CAPS_OFFSET);
> 
> instead of:
> cxl_read_mbox_reg32(cxlm, CXLDEV_MB_CAPS_OFFSET);
> 
> It's easier to spot than:
> readl(cxlm->regs + cxlm->status_offset + CXLDEV_MB_CAPS_OFFSET)

Well, what I think would be the most obvious is:

readl(cxlm->status_regs + CXLDEV_MB_CAPS_OFFSET);

> > > +	/* 8.2.8.4.3 */
> > 
> > ????
> > 
> 
> I had been trying to be consistent with 'CXL2.0 - ' in front of all spec
> reference. I obviously missed this one.

FYI, I generally find section names much easier to find than section
numbers.  Especially as the numbers change very frequently, some times
even for very minor updates to the spec.  E.g. in NVMe the numbers might
even change from NVMe 1.X to NVMe 1.Xa because an errata had to add
a clarification as its own section.