Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Jul 2023 17:39:38 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Terry Bowman <terry.bowman@amd.com>, <alison.schofield@intel.com>,
        <vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
        <bwidawsk@kernel.org>, <dan.j.williams@intel.com>,
        <dave.jiang@intel.com>, <Jonathan.Cameron@huawei.com>,
        <linux-cxl@vger.kernel.org>
CC:     <terry.bowman@amd.com>, <rrichter@amd.com>,
        <linux-kernel@vger.kernel.org>, <bhelgaas@google.com>
Subject: RE: [PATCH v8 03/14] cxl/hdm: Use stored Component Register mappings
 to map HDM decoder capability
Message-ID: <64a36a4a39351_8e178294c5@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230630231635.3132638-1-terry.bowman@amd.com>
 <20230630231635.3132638-4-terry.bowman@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230630231635.3132638-4-terry.bowman@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?A2UBmJYJWNBeju5ybp7Xab3KU7UGiYzgB3BHyjnDbHW1c6MIqQjbsCL0Uf2n?=
 =?us-ascii?Q?vbYfQmg5CxYMAiDqipBkjIKV/3UWISW4h3AxWmpxkR9tYMkmceRwx1mzRYCw?=
 =?us-ascii?Q?PUvLC5UStjSqVSHLFfkHm7Z9ikOXN+qObJcWOTgGBv9cyAgSFmL5T6jWxz4G?=
 =?us-ascii?Q?eDth3RML1aLXsxwNKsaEeh9pySHxJxiz2gl3o+VqXOQ2sYfghBHT5ncgNDTu?=
 =?us-ascii?Q?pSRP8PIAL9JKc9QQkJExSgIeUQ9qd+IbO8c7I6Oxew8VbnFPp/pd26w350fc?=
 =?us-ascii?Q?tp6v3d1BoL3tD56HQ406vELjhkpgINEV8P2OKmO5vq/7OCnmaNbbUGW+gBM4?=
 =?us-ascii?Q?BtOmEKGNgrIhp2mxB6rzSVFr5yl8E4teb9LZKd50MoZ0XLuHepWuqYp6EAnW?=
 =?us-ascii?Q?1rKCHw4n9yW7fpIm9a1xlmhU8tENCPHMiAK0YMfzMs9QDuCFsr7eqfu4GQpt?=
 =?us-ascii?Q?JoCY67LCjtYQm1ttEoWjb1TvynCj6uTPk1YSOQhpsOmP0qDGFpzc4bUobPOv?=
 =?us-ascii?Q?mmyN/R3WWNuT5LyQ/pgOC0HJlI297FzrzLYjjXeeKbLS1MoAT/zKg5IRCXb1?=
 =?us-ascii?Q?5YFoc9vt0v1l/dzGey/S7O9zpro2Q+xtqLbZk1+MXieiByTrEcMVT2tqTHvB?=
 =?us-ascii?Q?zgsAtfNAppo/tN8Tv5gCjf84+td2zYNYZQxadqtH+WVwa+APTqQdnfHI6IHZ?=
 =?us-ascii?Q?Rw4wJqSRjuCiu8mevrztfKKKJNzstLIVD465VVg/9UaszB6daP8hPVJpL92F?=
 =?us-ascii?Q?nuiA9n5kSKJqVsKabXnmL9UvPvmrBYz6fy6B34GNSdAS086G0Zna1RQOlj35?=
 =?us-ascii?Q?r3lemdQXuFZIhPYS2GAhZ9GuATXY9l1eVv5iEbwdByd3PxC5MjzIj/P7aBBd?=
 =?us-ascii?Q?136zNl8YfnmjKFl7oUq0LoGeaYMRccmVq3P8LyLAgWDrVb6KIw/KkRmpCaX9?=
 =?us-ascii?Q?rtPEqqDZcSIBFypKXXjifyUp5u1S4J9f9vluVTyCatdoSokfHtE+fQfynKyB?=
 =?us-ascii?Q?Rh4kqONySeKGO+LswhOFwf6dTVt199PlY/1J6ELvcfXQJffh8f85aohgimDp?=
 =?us-ascii?Q?m42BkPmStCjLb9oK/zfRGXXGgxBxd9tjiN01oEoazyM3VJFQYOfgkYq62TK9?=
 =?us-ascii?Q?HoLoIZTbn/03bJ7f5vB6NhMKl/EdpOLVIp2OuthExhRccBW89rhnaBKQAocS?=
 =?us-ascii?Q?s0K+WthQ8UzTIKgFMye3YTXm3wrEFn9nItMcBQ1YFBMy4z1cB4lsu1XTjvxX?=
 =?us-ascii?Q?gKHgI7wwE/c0g1l6RU8aZids3mGhGy0bXQGKGUgpp++aMRy1Gwo8q53E7eXI?=
 =?us-ascii?Q?J8+tveiiEPtVqsz3nJ0kFvmmOCpIFYB+jGH/q9zOuPjjSlHV+zpi8dUuw+BY?=
 =?us-ascii?Q?k8ISCDwPfij5hWeaT04qUWk6kfGeubfdOmfLDH1nio47JrRrpPs1yaeioTqN?=
 =?us-ascii?Q?La+iOUQfDGDYxY/NcI36Nu7mCBv/lZtbpU6ucBxuMEJ+lFfC1M0FtaBv7Nkk?=
 =?us-ascii?Q?5R8fSbjIID04+yXxaw7EHgRAUjpKIZWoYANJRsKSQKK47dO1raL7L5H0MZVX?=
 =?us-ascii?Q?NP/xTEeTWpd3JcxOn+xkdYcjCo5mcaJQT83QafcgnDsfjPqdvnZe48N1GUyc?=
 =?us-ascii?Q?Tg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 57895abd-1fe8-4069-a439-08db7c272887
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2023 00:39:43.1106
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nlj9cIjRGegY4Xu6C0et25p0n2L2gjhBqfFgjHQKmgHLo0kXvUfV9qslafFbcwc2UvLU7pLILeR4aMTgUoRUblVZKbQiYqFJ7iSmIwRt9/Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5970
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> Now, that the Component Register mappings are stored, use them to
> enable and map the HDM decoder capabilities. The Component Registers
> do not need to be probed again for this, remove probing code.
> 
> The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> Endpoint's component register mappings are located in the cxlds and
> else in the port's structure. Provide a helper function
> cxl_port_get_comp_map() to locate the mappings depending on the
> component's type.

This causes a regression that cxl_test tripped over. It's likely
something you could reproduce without cxl_test just be reloading the
cxl_port driver. Root cause below, but here is what I see on a test run:

# meson test -C build --suite cxl
ninja: no work to do.
ninja: Entering directory `/root/git/ndctl/build'
[113/113] Linking target ndctl/ndctl
1/6 ndctl:cxl / cxl-topology.sh             OK              12.78s
2/6 ndctl:cxl / cxl-region-sysfs.sh         OK               7.72s
3/6 ndctl:cxl / cxl-labels.sh               FAIL             1.53s   (exit status 129 or signal 1 SIGHUP)
>>> LD_LIBRARY_PATH=/root/git/ndctl/build/cxl/lib:/root/git/ndctl/build/ndctl/lib:/root/git/ndctl/build/daxctl/lib NDCTL=/root/git/ndctl/build/ndctl/ndctl DAXCTL=/root/git/ndctl/build/daxctl/daxctl MALLOC_PERTURB_=67 TEST_PATH=/root/git/ndctl/build/test DATA_PATH=/root/git/ndctl/test /bin/bash /root/git/ndctl/test/cxl-labels.sh

4/6 ndctl:cxl / cxl-create-region.sh        OK              17.05s
5/6 ndctl:cxl / cxl-xor-region.sh           OK               5.18s
6/6 ndctl:cxl / cxl-security.sh             OK               4.68s

Ok:                 5   
Expected Fail:      0   
Fail:               1   
Unexpected Pass:    0   
Skipped:            0   
Timeout:            0   

Full log written to /root/git/ndctl/build/meson-logs/testlog.txt

It's not that cxl-labels.sh causes the error, it is loading and
unloading the port driver trips over the problem below:

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c | 64 +++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 4449b34a80cc..b0f59e63e0d2 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->interleave_mask |= GENMASK(14, 12);
>  }
>  
> -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> -				struct cxl_component_regs *regs)
> -{
> -	struct cxl_register_map map = {
> -		.dev = &port->dev,
> -		.resource = port->component_reg_phys,
> -		.base = crb,
> -		.max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> -	};
> -
> -	cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> -	if (!map.component_map.hdm_decoder.valid) {
> -		dev_dbg(&port->dev, "HDM decoder registers not implemented\n");
> -		/* unique error code to indicate no HDM decoder capability */
> -		return -ENODEV;
> -	}
> -
> -	return cxl_map_component_regs(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
> -}
> -
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_hdm *cxlhdm;
> @@ -145,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  	return true;
>  }
>  
> +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> +{
> +	/*
> +	 * HDM capability applies to Endpoints, USPs and VH Host
> +	 * Bridges. The Endpoint's component register mappings are
> +	 * located in the cxlds.
> +	 */
> +	if (is_cxl_endpoint(port)) {
> +		struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> +
> +		return &memdev->cxlds->comp_map;

...but why? The issue here is that the @dev argument in that map is the
memdev parent PCI device. However, in this context the @dev for devm
operations wants to be &port->dev.

> +	}
> +
> +	return &port->comp_map;

...so this is fine, and folding in the following resolves the test
failure.

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index b0f59e63e0d2..6f111f487795 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -125,22 +125,6 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
        return true;
 }
 
-static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
-{
-       /*
-        * HDM capability applies to Endpoints, USPs and VH Host
-        * Bridges. The Endpoint's component register mappings are
-        * located in the cxlds.
-        */
-       if (is_cxl_endpoint(port)) {
-               struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
-
-               return &memdev->cxlds->comp_map;
-       }
-
-       return &port->comp_map;
-}
-
 /**
  * devm_cxl_setup_hdm - map HDM decoder component registers
  * @port: cxl_port to map
@@ -160,8 +144,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
        cxlhdm->port = port;
        dev_set_drvdata(dev, cxlhdm);
 
-       comp_map = cxl_port_get_comp_map(port);
-
+       comp_map = &port->comp_map;
        if (comp_map->resource == CXL_RESOURCE_NONE) {
                if (info && info->mem_enabled) {
                        cxlhdm->decoder_count = info->ranges;


Am I missing why the cxlds->comp_map needs to be reused?

Note that I am out and may not be able to respond further until next
week.