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:   Tue, 2 Feb 2021 18:10:16 +0000
From:   Christoph Hellwig <hch@infradead.org>
To:     Ben Widawsky <ben.widawsky@intel.com>
Cc:     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>,
        Christoph Hellwig <hch@infradead.org>,
        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: <20210202181016.GD3708021@infradead.org>
References: <20210130002438.1872527-1-ben.widawsky@intel.com>
 <20210130002438.1872527-4-ben.widawsky@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210130002438.1872527-4-ben.widawsky@intel.com>
Precedence: bulk

On Fri, Jan 29, 2021 at 04:24:27PM -0800, Ben Widawsky wrote:
>  #ifndef __CXL_H__
>  #define __CXL_H__
>  
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +
> +#define CXL_SET_FIELD(value, field)                                            \
> +	({                                                                     \
> +		WARN_ON(!FIELD_FIT(field##_MASK, value));                      \
> +		FIELD_PREP(field##_MASK, value);                               \
> +	})
> +
> +#define CXL_GET_FIELD(word, field) FIELD_GET(field##_MASK, word)

This looks like some massive obsfucation.  What is the intent
here?

> +	/* Cap 0001h - CXL_CAP_CAP_ID_DEVICE_STATUS */
> +	struct {
> +		void __iomem *regs;
> +	} status;
> +
> +	/* Cap 0002h - CXL_CAP_CAP_ID_PRIMARY_MAILBOX */
> +	struct {
> +		void __iomem *regs;
> +		size_t payload_size;
> +	} mbox;
> +
> +	/* 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?

> +#define cxl_reg(type)                                                          \
> +	static inline void cxl_write_##type##_reg32(struct cxl_mem *cxlm,      \
> +						    u32 reg, u32 value)        \
> +	{                                                                      \
> +		void __iomem *reg_addr = cxlm->type.regs;                      \
> +		writel(value, reg_addr + reg);                                 \
> +	}                                                                      \
> +	static inline void cxl_write_##type##_reg64(struct cxl_mem *cxlm,      \
> +						    u32 reg, u64 value)        \
> +	{                                                                      \
> +		void __iomem *reg_addr = cxlm->type.regs;                      \
> +		writeq(value, reg_addr + reg);                                 \
> +	}                                                                      \

What is the value add of all this obsfucation over the trivial
calls to the write*/read* functions, possible with a locally
declarate "void __iomem *" variable in the callers like in all
normall drivers?  Except for making the life of the poor soul trying
to debug this code some time in the future really hard, of course.

> +	/* 8.2.8.4.3 */

????