Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp719863pxb; Tue, 2 Feb 2021 16:37:44 -0800 (PST) X-Google-Smtp-Source: ABdhPJx9zKlpTCxx/XOE3FuZUA0YyJhf8Ned9Sx2JbDfIBOUg+3r/9bMj1Fw9+yilbMP2fPM3m5n X-Received: by 2002:a17:906:2353:: with SMTP id m19mr590501eja.13.1612312664789; Tue, 02 Feb 2021 16:37:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1612312664; cv=none; d=google.com; s=arc-20160816; b=NWLYjCBfqfa6H2QS/Hiack9ZGMhSsCR5lgZBJ2DGRaF+C0rOUJ+uzPUsJHpwQ77NQ4 GCHv1xa0NWRJoQwYnF/k1OktBxaMLBBDwsumdOwCMIym5yqtTEMXMEC7GV3OpI/s/7B9 4XvXMBSpkCPInQH1jg9PgYYG3M5+IXBA6SHRIpFTpDN6c3m84J4JXBEkGjhN3+LS8V2s /HHKKXfsehrH5kOkc+NKM5kW4jVwA/K6RwPiQmw7jQoqQjD1o9aFhskW6TcjMohjcWgB eHD+izgA5la322MsMihaNg1SG/a/3frKSEv4Qd5WIBLUFCu/1UeRCKr+YkdLg5wiDs4D 5eKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:ironport-sdr :ironport-sdr; bh=1V7v4chYdjmJUidrubfNbSFuIFY5fnFKbNZPC1EYKPQ=; b=PHkWvnPfJ2blhydw8zDm9qa9mn52QTnnU4MCbs9/lM5e196/xqd9049qppIpKC/ZE4 RzNbKDurLhNo1Gx1ZV3+BPplDrKgp830zKHL1uTDmXsOD5Ru/8Ng4wusboGu395ndM8P IlRNCBSJtJk1k+aNuO0Y+mSVT3CdhbHEsocp5cYvTMg/Vn7lLd3gDKLy2rerViMOEY6M MRoiciJyvmur+54Bt87JE6Wn5z4LkOsxTQqgXKVHQVAZBbBZefhJ04oSrztj3UlabGUU 4xgvX8WJo5QhANVrkvuN7YMzCcsWTaLGRXNol513HOfqEreAmY9sFDLScJFCJE2RU7TW C2pQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id rl11si305968ejb.688.2021.02.02.16.37.20; Tue, 02 Feb 2021 16:37:44 -0800 (PST) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238647AbhBBS1n (ORCPT + 99 others); Tue, 2 Feb 2021 13:27:43 -0500 Received: from mga11.intel.com ([192.55.52.93]:64452 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238289AbhBBSZE (ORCPT ); Tue, 2 Feb 2021 13:25:04 -0500 IronPort-SDR: e23cfKNk14nG+y+GcyHumJmOm8EhyYG6TNHiBXm9to+MOhS9nO8oLkKqcfi0UubNcoZ/kkYyzC goFY6vFohgZw== X-IronPort-AV: E=McAfee;i="6000,8403,9883"; a="177397682" X-IronPort-AV: E=Sophos;i="5.79,396,1602572400"; d="scan'208";a="177397682" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Feb 2021 10:24:21 -0800 IronPort-SDR: rlVOr+F59Dm93VedZPbkRLa3d7C9TiDXpbtmq3k0WVsbXYLMphar3MgWZ1t5VZzywr2UW91/TO FbLUWuM8GdVQ== X-IronPort-AV: E=Sophos;i="5.79,396,1602572400"; d="scan'208";a="391610338" Received: from joship1x-mobl1.amr.corp.intel.com (HELO intel.com) ([10.252.131.202]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Feb 2021 10:24:20 -0800 Date: Tue, 2 Feb 2021 10:24:18 -0800 From: Ben Widawsky To: Christoph Hellwig 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 , Chris Browy , Dan Williams , Ira Weiny , Jon Masters , Jonathan Cameron , Rafael Wysocki , Randy Dunlap , Vishal Verma , daniel.lll@alibaba-inc.com, "John Groves (jgroves)" , "Kelley, Sean V" Subject: Re: [PATCH 03/14] cxl/mem: Find device capabilities Message-ID: <20210202182418.3wyxnm6rqeoeclu2@intel.com> References: <20210130002438.1872527-1-ben.widawsky@intel.com> <20210130002438.1872527-4-ben.widawsky@intel.com> <20210202181016.GD3708021@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210202181016.GD3708021@infradead.org> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 21-02-02 18:10:16, Christoph Hellwig wrote: > On Fri, Jan 29, 2021 at 04:24:27PM -0800, Ben Widawsky wrote: > > #ifndef __CXL_H__ > > #define __CXL_H__ > > > > +#include > > +#include > > +#include > > + > > +#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? > I will drop these. I don't recall why I did this to be honest. > > + /* 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? 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. > > > +#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. > 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) > > + /* 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.