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;
MIME-Version: 1.0
References: <20210506223654.1310516-1-ira.weiny@intel.com> <20210506223654.1310516-2-ira.weiny@intel.com>
In-Reply-To: <20210506223654.1310516-2-ira.weiny@intel.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Wed, 19 May 2021 17:50:44 -0700
Message-ID: <CAPcyv4iV-Xq7eGBHavETiKW=kfucxDnua3a4j6MKtswz=n4gRA@mail.gmail.com>
Subject: Re: [PATCH 1/4] cxl/mem: Fully decode device capability header
To:     "Weiny, Ira" <ira.weiny@intel.com>
Cc:     Ben Widawsky <ben.widawsky@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        linux-cxl@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, May 6, 2021 at 3:37 PM <ira.weiny@intel.com> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> Previously only the capability ID and offset were decoded.
>
> Create a version MASK and decode the additional version and length
> fields of the header.
>

I'm not seeing a justification for why Linux would want this patch?

> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/cxl/core.c | 15 ++++++++++++---
>  drivers/cxl/cxl.h  |  1 +
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index b3c3532b53f7..21553386e218 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -501,12 +501,21 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>
>         for (cap = 1; cap <= cap_count; cap++) {
>                 void __iomem *register_block;
> -               u32 offset;
> +               u32 hdr, offset, __maybe_unused length;

What's the point of reading the length and not using it? If this is
used in a future patch then wait until then to add it.

>                 u16 cap_id;
> +               u8 version;
> +
> +               hdr = readl(base + cap * 0x10);
> +
> +               cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK, hdr);
> +               version = FIELD_GET(CXLDEV_CAP_HDR_VERSION_MASK, hdr);
> +               if (version != 1)
> +                       dev_err(dev, "Vendor cap ID: %x incorrect version (0x%x)\n",
> +                               cap_id, version);

It's not an error. Any future version needs to be backwards
compatible. All this is doing is ensuring that when hardware is
updated old kernels will start spamming the log.

>
> -               cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
> -                                  readl(base + cap * 0x10));
>                 offset = readl(base + cap * 0x10 + 0x4);
> +               length = readl(base + cap * 0x10 + 0x8);
> +
>                 register_block = base + offset;
>
>                 switch (cap_id) {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0211f44c95a2..9b315c069557 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -15,6 +15,7 @@
>  #define   CXLDEV_CAP_ARRAY_COUNT_MASK GENMASK_ULL(47, 32)
>  /* CXL 2.0 8.2.8.2 CXL Device Capability Header Register */
>  #define CXLDEV_CAP_HDR_CAP_ID_MASK GENMASK(15, 0)
> +#define CXLDEV_CAP_HDR_VERSION_MASK GENMASK(23, 16)
>  /* CXL 2.0 8.2.8.2.1 CXL Device Capabilities */
>  #define CXLDEV_CAP_CAP_ID_DEVICE_STATUS 0x1
>  #define CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX 0x2
> --
> 2.28.0.rc0.12.gb6a658bd00c9
>