From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4E685C433DB for ; Wed, 3 Feb 2021 21:24:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0534864E4F for ; Wed, 3 Feb 2021 21:24:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229934AbhBCVYS (ORCPT ); Wed, 3 Feb 2021 16:24:18 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60136 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231448AbhBCVYN (ORCPT ); Wed, 3 Feb 2021 16:24:13 -0500 Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B2B65C0613D6 for ; Wed, 3 Feb 2021 13:23:32 -0800 (PST) Received: by mail-ej1-x630.google.com with SMTP id y9so1391370ejp.10 for ; Wed, 03 Feb 2021 13:23:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VT23w92CT9zXGoB2xTF4DjGzC0MXKQMCiaO+54MJ7Ao=; b=kxpyxImNyl0PD+X1iGOdvaWlfhCG1TIKsHTUd3YIbtmQGOQN1d0MCUUHfIff4tHdHo 4rkeo72QT5v52E7o4MSw6G5C9PFnkqsaxGiD2t6Z3g/nUDvrDKbVkRo0D9oUGce8xMd1 D9WDuYQE1t98gpIh3hjVGZlF/tNPEAhtns2hDwk/fUXSf38hcGas8HbkZcDAaRURLGKV 3RiAoKUv3nJWfMx/5VeciMGYvZKE2HYlJAAUd9sAwpDOwfKSbc1R0yNBFxbZNzr0+sYo /MDlatcZ7OcQ7mRVmkYp7bnDyKHd824Gp2Hb+fsrxrOX/ujxvWArT4kJDlSVwPhsHWbv WhOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VT23w92CT9zXGoB2xTF4DjGzC0MXKQMCiaO+54MJ7Ao=; b=IUBOKcyS7qCS3JfpJX0TBYQo9N0ylGgAhLtX7cu0Eeq/WKSy126w/ZWD2KwI+KtNtS GEeh4BXmT8rw/X0GVKc42+FdlmM8n5E26wuPWMM/jKS6GaNX6YoXFE6XGhiVzDeUpyWt vNRfQxr7AOhyfEKB7+jOj9N5qR3ZgLBnnV0uuoPn1zgs5Z04uYUMMndy57wpjPLuX259 1t/X2BLiGX3OmgESWzkvAhUuB+mmZE9bI0y+awYhlEOD05Ug0krPUJD/qhHCw9ruBgeW i54awz1qun5x4do/4oQ+B/r4py+cihMKrWVE9cIigParycan0KmFvCoaL5H8WWaxsnBv G6Lw== X-Gm-Message-State: AOAM531yuLZMLfws2BaNN1a428Ibkx/sj1FtjTLBI5F1HgE/FMMPm4Fb ZCJsHhw5Xl6382LUEDluuD4d23mN3A9+1VF6ZeTa4Q== X-Google-Smtp-Source: ABdhPJycsTuuO9WFxOwJr0uveWLDYX613Mw9CD4bctsoU/uYDmnrN/Izr6YTrrIN45yfo7Sw2Zt5lHunHYMWXjSffEI= X-Received: by 2002:a17:906:d8a1:: with SMTP id qc1mr5313759ejb.523.1612387411333; Wed, 03 Feb 2021 13:23:31 -0800 (PST) MIME-Version: 1.0 References: <20210130002438.1872527-1-ben.widawsky@intel.com> <20210130002438.1872527-4-ben.widawsky@intel.com> <20210202181016.GD3708021@infradead.org> <20210202182418.3wyxnm6rqeoeclu2@intel.com> <20210203171534.GB4104698@infradead.org> <20210203172342.fpn5vm4xj2xwh6fq@intel.com> In-Reply-To: <20210203172342.fpn5vm4xj2xwh6fq@intel.com> From: Dan Williams Date: Wed, 3 Feb 2021 13:23:31 -0800 Message-ID: Subject: Re: [PATCH 03/14] cxl/mem: Find device capabilities To: Ben Widawsky Cc: Christoph Hellwig , linux-cxl@vger.kernel.org, Linux ACPI , Linux Kernel Mailing List , linux-nvdimm , Linux PCI , Bjorn Helgaas , Chris Browy , Ira Weiny , Jon Masters , Jonathan Cameron , Rafael Wysocki , Randy Dunlap , Vishal Verma , daniel.lll@alibaba-inc.com, "John Groves (jgroves)" , "Kelley, Sean V" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Feb 3, 2021 at 9:23 AM Ben Widawsky wrote: > > On 21-02-03 17:15:34, Christoph Hellwig wrote: > > 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); > > > > Right, so you wrote the buggy version. Should be. > readl(cxlm->mbox_regs + CXLDEV_MB_CAPS_OFFSET); > > Admittedly, "MB" for mailbox isn't super obvious. I think you've convinced me to > rename these register definitions > s/MB/MBOX. > > I'd prefer to keep the helpers for now as I do find them helpful, and so far > nobody else who has touched the code has complained. If you feel strongly, I > will change it. After seeing the options, I think I'd prefer to not have to worry what extra magic is happening with cxl_read_mbox_reg32() cxl_read_mbox_reg32(cxlm, CXLDEV_MB_CAPS_OFFSET); readl(cxlm->mbox_regs + CXLDEV_MB_CAPS_OFFSET); The latter is both shorter and more idiomatic. > > > > > > + /* 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. > > Why not both? > > I ran into this in fact going from version 0.7 to 1.0 of the spec. I did call > out the spec version to address this, but you're right. Section names can change > too in theory. > > /* > * CXL 2.0 8.2.8.4.3 > * Mailbox Capabilities Register > */ > > Too much? That seems like too many lines. /* CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register */ ...this looks ok.