From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B69E9215198 for ; Wed, 9 Apr 2025 23:10:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.11 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744240238; cv=none; b=d79HrbbkqX7PXuy849jMKk8r6pIO8FJNTgFMpvmFPXuJ4rFK2eoCpriIeTicSIyIp9gIvW19xmJ9A7wzqnEUzEXEt9fZyeAxXP5k8AarAAh4kPKgEjTimgexVo7cvY9LP5jaxrFTy/29aqx3japZ5cSVq3/g0ogSPn3UYf2S7nY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744240238; c=relaxed/simple; bh=u/n1aGmfJVgYwSs7QCF5pzObc3G0d4j4oIh8RMHqRTY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=RpMSTJgbxn9rtfu5R9XByufsj0/QVBVQ0QiE0qwZaAEcUpr3FicJLLRQjgTXQ/2YmoOC7HyMhomLyQDa1fHnOI04goY5UMK5nWzYSnl3ITpy8uWcZZHjy9MvltKY/coAERxzF4chh7UUfbfT5MPViKFrD5AzOxSrMcJaim78lrg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=d+Di66rD; arc=none smtp.client-ip=198.175.65.11 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="d+Di66rD" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1744240236; x=1775776236; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=u/n1aGmfJVgYwSs7QCF5pzObc3G0d4j4oIh8RMHqRTY=; b=d+Di66rDH+LBa+VFZhNi5YXr28ocvNrxX1pSImaJYKvonp4pDEISr+Sg 78ITTJ033tys0NTOtWe/4ArMenimi2Z7K0nxpme8SXmW1s+mMd09zj8CE lSbaiijGjESj8RbJzq+0IlJ0aeSN5nAmzeYEDtWTp6HOrDFD0veCIRc5m YEMBE8o3j6Z8807zVGI8F5jOZ0DpweRxaGX1Fep2SvvF7o52gvbIjH5pn fgxj9iuPM29XkLbpG45MzgBkCPQB7aLemWCkkVl42lp8uKBjDmnWGb95f uJ5wq8Bu0A/9+dMCvMEKPfOwxhglYJGmyCFGuFym7arfAnJlzxxCU5Lfg g==; X-CSE-ConnectionGUID: 8HkqNvZIRPejyeBprY4XRQ== X-CSE-MsgGUID: 1LyQw2QLTwWb6I/tbgVrfg== X-IronPort-AV: E=McAfee;i="6700,10204,11399"; a="55916493" X-IronPort-AV: E=Sophos;i="6.15,201,1739865600"; d="scan'208";a="55916493" Received: from fmviesa001.fm.intel.com ([10.60.135.141]) by orvoesa103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Apr 2025 16:10:35 -0700 X-CSE-ConnectionGUID: 3Z8dVSIgQOmShvUYqrpciw== X-CSE-MsgGUID: EX3LycqjRUaB+7xsOGbZYA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.15,201,1739865600"; d="scan'208";a="159690303" Received: from ldmartin-desk2.corp.intel.com (HELO [10.125.111.236]) ([10.125.111.236]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Apr 2025 16:10:34 -0700 Message-ID: Date: Wed, 9 Apr 2025 16:10:30 -0700 Precedence: bulk X-Mailing-List: linux-cxl@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [NDCTL PATCH v4 3/3] cxl/test: Add test for cxl features device To: Alison Schofield Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev References: <20250218230116.2689627-1-dave.jiang@intel.com> <20250218230116.2689627-4-dave.jiang@intel.com> Content-Language: en-US From: Dave Jiang In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 4/9/25 1:40 PM, Alison Schofield wrote: > On Tue, Feb 18, 2025 at 03:59:56PM -0700, Dave Jiang wrote: >> Add a unit test to verify the features ioctl commands. Test support added >> for locating a features device, retrieve and verify the supported features >> commands, retrieve specific feature command data, retrieve test feature >> data, and write and verify test feature data. >> > > Let's revisit the naming - > > If the script is cxl-feature.sh then would the C program make sense as > feature-control.c or ??? I don't have strong opinions on this. I can change it to feature-control.c. DJ > >> Signed-off-by: Dave Jiang >> --- >> v4: >> - Adjust for kernel changes of input/out data structures >> - Setup test script to error out if not -ENODEV >> - Remove kernel 6.15 check >> --- >> test/cxl-features.sh | 31 ++++ >> test/fwctl.c | 383 +++++++++++++++++++++++++++++++++++++++++++ >> test/meson.build | 45 +++++ >> 3 files changed, 459 insertions(+) >> create mode 100755 test/cxl-features.sh >> create mode 100644 test/fwctl.c >> >> diff --git a/test/cxl-features.sh b/test/cxl-features.sh > > snip > >> diff --git a/test/fwctl.c b/test/fwctl.c >> new file mode 100644 >> index 000000000000..ca39e30f6dca >> --- /dev/null >> +++ b/test/fwctl.c >> @@ -0,0 +1,383 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (C) 2024-2025 Intel Corporation. All rights reserved. >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > > Not clear bitmap.h is needed? > >> + >> +static const char provider[] = "cxl_test"; >> + >> +UUID_DEFINE(test_uuid, >> + 0xff, 0xff, 0xff, 0xff, >> + 0xff, 0xff, >> + 0xff, 0xff, >> + 0xff, 0xff, >> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff >> +); >> + >> +#define CXL_MBOX_OPCODE_GET_SUPPORTED_FEATURES 0x0500 >> +#define CXL_MBOX_OPCODE_GET_FEATURE 0x0501 >> +#define CXL_MBOX_OPCODE_SET_FEATURE 0x0502 >> + >> +#define GET_FEAT_SIZE 4 >> +#define SET_FEAT_SIZE 4 >> +#define EFFECTS_MASK (BIT(0) | BIT(9)) >> + >> +#define MAX_TEST_FEATURES 1 >> +#define DEFAULT_TEST_DATA 0xdeadbeef >> +#define DEFAULT_TEST_DATA2 0xabcdabcd >> + >> +struct test_feature { >> + uuid_t uuid; >> + size_t get_size; >> + size_t set_size; >> +}; >> + >> +static int send_command(int fd, struct fwctl_rpc *rpc, struct fwctl_rpc_cxl_out *out) >> +{ >> + if (ioctl(fd, FWCTL_RPC, rpc) == -1) { >> + fprintf(stderr, "RPC ioctl error: %s\n", strerror(errno)); >> + return -errno; >> + } >> + >> + if (out->retval) { >> + fprintf(stderr, "operation returned failure: %d\n", out->retval); >> + return -ENXIO; >> + } >> + >> + return 0; >> +} > > Above the send_command() is factored out and reused. How about doing similar with > the ioctl setup - ie a setup_and_send_command() that setups up the ioctl and calls > send_command(). That removes redundancy in *get_feature, *set_feature, *get_supported. > > >> + >> +static int cxl_fwctl_rpc_get_test_feature(int fd, struct test_feature *feat_ctx, >> + const uint32_t expected_data) >> +{ >> + struct cxl_mbox_get_feat_in *feat_in; >> + struct fwctl_rpc_cxl_out *out; >> + struct fwctl_rpc rpc = {0}; >> + struct fwctl_rpc_cxl *in; >> + size_t out_size, in_size; >> + uint32_t val; >> + void *data; >> + int rc; >> + >> + in_size = sizeof(*in) + sizeof(*feat_in); >> + rc = posix_memalign((void **)&in, 16, in_size); >> + if (rc) >> + return -ENOMEM; >> + memset(in, 0, in_size); > > How about de-duplicating the repeated posix_memalign() + memset() pattern into > one helper func like alloc_aligned_memory() - including the memset on success. > > >> + feat_in = &in->get_feat_in; >> + >> + uuid_copy(feat_in->uuid, feat_ctx->uuid); >> + feat_in->count = feat_ctx->get_size; >> + >> + out_size = sizeof(*out) + feat_ctx->get_size; >> + rc = posix_memalign((void **)&out, 16, out_size); >> + if (rc) >> + goto free_in; >> + memset(out, 0, out_size); >> + >> + in->opcode = CXL_MBOX_OPCODE_GET_FEATURE; >> + in->op_size = sizeof(*feat_in); >> + >> + rpc.size = sizeof(rpc); >> + rpc.scope = FWCTL_RPC_CONFIGURATION; >> + rpc.in_len = in_size; >> + rpc.out_len = out_size; >> + rpc.in = (uint64_t)(uint64_t *)in; >> + rpc.out = (uint64_t)(uint64_t *)out; >> + >> + rc = send_command(fd, &rpc, out); >> + if (rc) >> + goto free_all; >> + >> + data = out->payload; >> + val = le32toh(*(__le32 *)data); >> + if (memcmp(&val, &expected_data, sizeof(val)) != 0) { >> + rc = -ENXIO; >> + goto free_all; >> + } >> + >> +free_all: >> + free(out); >> +free_in: >> + free(in); >> + return rc; >> +} >> + > snip > >> +static int test_fwctl_features(struct cxl_memdev *memdev) >> +{ >> + struct test_feature feat_ctx; >> + unsigned int major, minor; >> + int fd, rc; >> + char path[256]; >> + >> + major = cxl_memdev_get_fwctl_major(memdev); >> + minor = cxl_memdev_get_fwctl_minor(memdev); >> + >> + if (!major && !minor) >> + return -ENODEV; >> + >> + sprintf(path, "/dev/char/%d:%d", major, minor); >> + >> + fd = open(path, O_RDONLY, 0644); >> + if (!fd) { >> + fprintf(stderr, "Failed to open: %d\n", -errno); >> + return -errno; >> + } > > Needs to be "if (fd < 0)" as open() returns -1 on failure. > > snip