[ndctl,09/15] cxl/region: Make ways an integer argument

Message ID	166777845733.1238089.4849744927692588680.stgit@dwillia2-xfh.jf.intel.com
State	Superseded
Headers	show Return-Path: <linux-cxl-owner@kernel.org> Subject: [ndctl PATCH 09/15] cxl/region: Make ways an integer argument From: Dan Williams <dan.j.williams@intel.com> To: vishal.l.verma@intel.com Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev Date: Sun, 06 Nov 2022 15:47:37 -0800 Message-ID: <166777845733.1238089.4849744927692588680.stgit@dwillia2-xfh.jf.intel.com> In-Reply-To: <166777840496.1238089.5601286140872803173.stgit@dwillia2-xfh.jf.intel.com> References: <166777840496.1238089.5601286140872803173.stgit@dwillia2-xfh.jf.intel.com> User-Agent: StGit/0.18-3-g996c MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Precedence: bulk
Series	cxl-cli test and usability updates \| expand [ndctl,00/15] cxl-cli test and usability updates [ndctl,01/15] ndctl/test: Move firmware-update.sh to the 'descructive' set [ndctl,02/15] ndctl/test: Add kernel backtrace detection to some dax tests [ndctl,03/15] ndctl/clang-format: Move minimum version to 6 [ndctl,04/15] ndctl/clang-format: Fix space after for_each macros [ndctl,05/15] cxl/list: Always attempt to collect child objects [ndctl,06/15] cxl/list: Skip emitting pmem_size when it is zero [ndctl,07/15] cxl/filter: Return json-c topology [ndctl,08/15] cxl/list: Record cxl objects in json objects [ndctl,09/15] cxl/region: Make ways an integer argument [ndctl,10/15] cxl/region: Make granularity an integer argument [ndctl,11/15] cxl/region: Use cxl_filter_walk() to gather create-region targets [ndctl,12/15] cxl/region: Trim region size by max available extent [ndctl,13/15] cxl/region: Default to memdev mode for create with no arguments [ndctl,14/15] cxl/test: Extend cxl-topology.sh for a single root-port host-bridge [ndctl,15/15] cxl/test: Test single-port host-bridge region creation

Message ID

166777845733.1238089.4849744927692588680.stgit@dwillia2-xfh.jf.intel.com

State

Superseded

Headers

Subject: [ndctl PATCH 09/15] cxl/region: Make ways an integer argument
From: Dan Williams <dan.j.williams@intel.com>
To: vishal.l.verma@intel.com
Cc: linux-cxl@vger.kernel.org, nvdimm@lists.linux.dev
Date: Sun, 06 Nov 2022 15:47:37 -0800
Message-ID: 
 <166777845733.1238089.4849744927692588680.stgit@dwillia2-xfh.jf.intel.com>
In-Reply-To: 
 <166777840496.1238089.5601286140872803173.stgit@dwillia2-xfh.jf.intel.com>
References: 
 <166777840496.1238089.5601286140872803173.stgit@dwillia2-xfh.jf.intel.com>
User-Agent: StGit/0.18-3-g996c
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Series

cxl-cli test and usability updates | expand

Commit Message

Dan Williams Nov. 6, 2022, 11:47 p.m. UTC

Since --ways does not take a unit value like --size, just make it an
integer argument directly and skip the hand coded conversion.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 cxl/region.c |   41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

Comments

Alison Schofield Nov. 7, 2022, 10:43 p.m. UTC | #1

On Sun, Nov 06, 2022 at 03:47:37PM -0800, Dan Williams wrote:
> Since --ways does not take a unit value like --size, just make it an
> integer argument directly and skip the hand coded conversion.

Just curious why not unsigned int for this and granularity?


> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |   41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 334fcc291de7..494da5139c05 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -21,21 +21,23 @@
>  static struct region_params {
>  	const char *bus;
>  	const char *size;
> -	const char *ways;
>  	const char *granularity;
>  	const char *type;
>  	const char *root_decoder;
>  	const char *region;
> +	int ways;
>  	bool memdevs;
>  	bool force;
>  	bool human;
>  	bool debug;
> -} param;
> +} param = {
> +	.ways = INT_MAX,
> +};
>  
>  struct parsed_params {
>  	u64 size;
>  	u64 ep_min_size;
> -	unsigned int ways;
> +	int ways;
>  	unsigned int granularity;
>  	const char **targets;
>  	int num_targets;
> @@ -63,9 +65,8 @@ OPT_BOOLEAN(0, "debug", &param.debug, "turn on debug")
>  OPT_STRING('s', "size", &param.size, \
>  	   "size in bytes or with a K/M/G etc. suffix", \
>  	   "total size desired for the resulting region."), \
> -OPT_STRING('w', "ways", &param.ways, \
> -	   "number of interleave ways", \
> -	   "number of memdevs participating in the regions interleave set"), \
> +OPT_INTEGER('w', "ways", &param.ways, \
> +	    "number of memdevs participating in the regions interleave set"), \
>  OPT_STRING('g', "granularity", \
>  	   &param.granularity, "interleave granularity", \
>  	   "granularity of the interleave set"), \
> @@ -126,15 +127,11 @@ static int parse_create_options(int argc, const char **argv,
>  		}
>  	}
>  
> -	if (param.ways) {
> -		unsigned long ways = strtoul(param.ways, NULL, 0);
> -
> -		if (ways == ULONG_MAX || (int)ways <= 0) {
> -			log_err(&rl, "Invalid interleave ways: %s\n",
> -				param.ways);
> -			return -EINVAL;
> -		}
> -		p->ways = ways;
> +	if (param.ways <= 0) {
> +		log_err(&rl, "Invalid interleave ways: %d\n", param.ways);
> +		return -EINVAL;
> +	} else if (param.ways < INT_MAX) {
> +		p->ways = param.ways;
>  	} else if (argc) {
>  		p->ways = argc;
>  	} else {
> @@ -155,13 +152,13 @@ static int parse_create_options(int argc, const char **argv,
>  	}
>  
>  
> -	if (argc > (int)p->ways) {
> +	if (argc > p->ways) {
>  		for (i = p->ways; i < argc; i++)
>  			log_err(&rl, "extra argument: %s\n", p->targets[i]);
>  		return -EINVAL;
>  	}
>  
> -	if (argc < (int)p->ways) {
> +	if (argc < p->ways) {
>  		log_err(&rl,
>  			"too few target arguments (%d) for interleave ways (%u)\n",
>  			argc, p->ways);
> @@ -253,7 +250,7 @@ static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
>  
>  static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
>  {
> -	unsigned int i, matched = 0;
> +	int i, matched = 0;
>  
>  	for (i = 0; i < p->ways; i++) {
>  		struct cxl_memdev *memdev;
> @@ -393,7 +390,8 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
>  					    struct parsed_params *p)
>  {
>  	const char *devname = cxl_region_get_devname(region);
> -	unsigned int granularity, ways;
> +	unsigned int granularity;
> +	int ways;
>  
>  	/* Default granularity will be the root decoder's granularity */
>  	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
> @@ -408,7 +406,7 @@ static int cxl_region_determine_granularity(struct cxl_region *region,
>  		return granularity;
>  
>  	ways = cxl_decoder_get_interleave_ways(p->root_decoder);
> -	if (ways == 0 || ways == UINT_MAX) {
> +	if (ways == 0 || ways == -1) {
>  		log_err(&rl, "%s: unable to determine root decoder ways\n",
>  			devname);
>  		return -ENXIO;
> @@ -436,12 +434,11 @@ static int create_region(struct cxl_ctx *ctx, int *count,
>  {
>  	unsigned long flags = UTIL_JSON_TARGETS;
>  	struct json_object *jregion;
> -	unsigned int i, granularity;
>  	struct cxl_region *region;
> +	int i, rc, granularity;
>  	u64 size, max_extent;
>  	const char *devname;
>  	uuid_t uuid;
> -	int rc;
>  
>  	rc = create_region_validate_config(ctx, p);
>  	if (rc)
>

Dan Williams Nov. 7, 2022, 11:50 p.m. UTC | #2

Alison Schofield wrote:
> On Sun, Nov 06, 2022 at 03:47:37PM -0800, Dan Williams wrote:
> > Since --ways does not take a unit value like --size, just make it an
> > integer argument directly and skip the hand coded conversion.
> 
> Just curious why not unsigned int for this and granularity?

Mainly to avoid signed vs unsigned integer comparison with a typical
'int i' iterator variable.

Verma, Vishal L Nov. 8, 2022, 7:36 p.m. UTC | #3

On Sun, 2022-11-06 at 15:47 -0800, Dan Williams wrote:
> Since --ways does not take a unit value like --size, just make it an
> integer argument directly and skip the hand coded conversion.

Ah I had gone back and forth between using an int vs. unsigned. I had
gone with unsigned because the kernel treats it as an unsigned too
behind the sysfs ABI. But I guess in practice it doesn't matter, since
the values are clamped to [256, 16K]. Certainly using an int here makes
things cleaner!

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  cxl/region.c |   41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/cxl/region.c b/cxl/region.c
> index 334fcc291de7..494da5139c05 100644
> --- a/cxl/region.c
> +++ b/cxl/region.c
> @@ -21,21 +21,23 @@
>  static struct region_params {
>         const char *bus;
>         const char *size;
> -       const char *ways;
>         const char *granularity;
>         const char *type;
>         const char *root_decoder;
>         const char *region;
> +       int ways;
>         bool memdevs;
>         bool force;
>         bool human;
>         bool debug;
> -} param;
> +} param = {
> +       .ways = INT_MAX,
> +};
>  
>  struct parsed_params {
>         u64 size;
>         u64 ep_min_size;
> -       unsigned int ways;
> +       int ways;
>         unsigned int granularity;
>         const char **targets;
>         int num_targets;
> @@ -63,9 +65,8 @@ OPT_BOOLEAN(0, "debug", &param.debug, "turn on
> debug")
>  OPT_STRING('s', "size", &param.size, \
>            "size in bytes or with a K/M/G etc. suffix", \
>            "total size desired for the resulting region."), \
> -OPT_STRING('w', "ways", &param.ways, \
> -          "number of interleave ways", \
> -          "number of memdevs participating in the regions interleave
> set"), \
> +OPT_INTEGER('w', "ways", &param.ways, \
> +           "number of memdevs participating in the regions
> interleave set"), \
>  OPT_STRING('g', "granularity", \
>            &param.granularity, "interleave granularity", \
>            "granularity of the interleave set"), \
> @@ -126,15 +127,11 @@ static int parse_create_options(int argc, const
> char **argv,
>                 }
>         }
>  
> -       if (param.ways) {
> -               unsigned long ways = strtoul(param.ways, NULL, 0);
> -
> -               if (ways == ULONG_MAX || (int)ways <= 0) {
> -                       log_err(&rl, "Invalid interleave ways: %s\n",
> -                               param.ways);
> -                       return -EINVAL;
> -               }
> -               p->ways = ways;
> +       if (param.ways <= 0) {
> +               log_err(&rl, "Invalid interleave ways: %d\n",
> param.ways);
> +               return -EINVAL;
> +       } else if (param.ways < INT_MAX) {
> +               p->ways = param.ways;
>         } else if (argc) {
>                 p->ways = argc;
>         } else {
> @@ -155,13 +152,13 @@ static int parse_create_options(int argc, const
> char **argv,
>         }
>  
>  
> -       if (argc > (int)p->ways) {
> +       if (argc > p->ways) {
>                 for (i = p->ways; i < argc; i++)
>                         log_err(&rl, "extra argument: %s\n", p-
> >targets[i]);
>                 return -EINVAL;
>         }
>  
> -       if (argc < (int)p->ways) {
> +       if (argc < p->ways) {
>                 log_err(&rl,
>                         "too few target arguments (%d) for interleave
> ways (%u)\n",
>                         argc, p->ways);
> @@ -253,7 +250,7 @@ static bool validate_memdev(struct cxl_memdev
> *memdev, const char *target,
>  
>  static int validate_config_memdevs(struct cxl_ctx *ctx, struct
> parsed_params *p)
>  {
> -       unsigned int i, matched = 0;
> +       int i, matched = 0;
>  
>         for (i = 0; i < p->ways; i++) {
>                 struct cxl_memdev *memdev;
> @@ -393,7 +390,8 @@ static int
> cxl_region_determine_granularity(struct cxl_region *region,
>                                             struct parsed_params *p)
>  {
>         const char *devname = cxl_region_get_devname(region);
> -       unsigned int granularity, ways;
> +       unsigned int granularity;
> +       int ways;
>  
>         /* Default granularity will be the root decoder's granularity
> */
>         granularity = cxl_decoder_get_interleave_granularity(p-
> >root_decoder);
> @@ -408,7 +406,7 @@ static int
> cxl_region_determine_granularity(struct cxl_region *region,
>                 return granularity;
>  
>         ways = cxl_decoder_get_interleave_ways(p->root_decoder);
> -       if (ways == 0 || ways == UINT_MAX) {
> +       if (ways == 0 || ways == -1) {
>                 log_err(&rl, "%s: unable to determine root decoder
> ways\n",
>                         devname);
>                 return -ENXIO;
> @@ -436,12 +434,11 @@ static int create_region(struct cxl_ctx *ctx,
> int *count,
>  {
>         unsigned long flags = UTIL_JSON_TARGETS;
>         struct json_object *jregion;
> -       unsigned int i, granularity;
>         struct cxl_region *region;
> +       int i, rc, granularity;
>         u64 size, max_extent;
>         const char *devname;
>         uuid_t uuid;
> -       int rc;
>  
>         rc = create_region_validate_config(ctx, p);
>         if (rc)
>

diff --git a/cxl/region.c b/cxl/region.c
index 334fcc291de7..494da5139c05 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -21,21 +21,23 @@ 
 static struct region_params {
 	const char *bus;
 	const char *size;
-	const char *ways;
 	const char *granularity;
 	const char *type;
 	const char *root_decoder;
 	const char *region;
+	int ways;
 	bool memdevs;
 	bool force;
 	bool human;
 	bool debug;
-} param;
+} param = {
+	.ways = INT_MAX,
+};
 
 struct parsed_params {
 	u64 size;
 	u64 ep_min_size;
-	unsigned int ways;
+	int ways;
 	unsigned int granularity;
 	const char **targets;
 	int num_targets;
@@ -63,9 +65,8 @@  OPT_BOOLEAN(0, "debug", &param.debug, "turn on debug")
 OPT_STRING('s', "size", &param.size, \
 	   "size in bytes or with a K/M/G etc. suffix", \
 	   "total size desired for the resulting region."), \
-OPT_STRING('w', "ways", &param.ways, \
-	   "number of interleave ways", \
-	   "number of memdevs participating in the regions interleave set"), \
+OPT_INTEGER('w', "ways", &param.ways, \
+	    "number of memdevs participating in the regions interleave set"), \
 OPT_STRING('g', "granularity", \
 	   &param.granularity, "interleave granularity", \
 	   "granularity of the interleave set"), \
@@ -126,15 +127,11 @@  static int parse_create_options(int argc, const char **argv,
 		}
 	}
 
-	if (param.ways) {
-		unsigned long ways = strtoul(param.ways, NULL, 0);
-
-		if (ways == ULONG_MAX || (int)ways <= 0) {
-			log_err(&rl, "Invalid interleave ways: %s\n",
-				param.ways);
-			return -EINVAL;
-		}
-		p->ways = ways;
+	if (param.ways <= 0) {
+		log_err(&rl, "Invalid interleave ways: %d\n", param.ways);
+		return -EINVAL;
+	} else if (param.ways < INT_MAX) {
+		p->ways = param.ways;
 	} else if (argc) {
 		p->ways = argc;
 	} else {
@@ -155,13 +152,13 @@  static int parse_create_options(int argc, const char **argv,
 	}
 
 
-	if (argc > (int)p->ways) {
+	if (argc > p->ways) {
 		for (i = p->ways; i < argc; i++)
 			log_err(&rl, "extra argument: %s\n", p->targets[i]);
 		return -EINVAL;
 	}
 
-	if (argc < (int)p->ways) {
+	if (argc < p->ways) {
 		log_err(&rl,
 			"too few target arguments (%d) for interleave ways (%u)\n",
 			argc, p->ways);
@@ -253,7 +250,7 @@  static bool validate_memdev(struct cxl_memdev *memdev, const char *target,
 
 static int validate_config_memdevs(struct cxl_ctx *ctx, struct parsed_params *p)
 {
-	unsigned int i, matched = 0;
+	int i, matched = 0;
 
 	for (i = 0; i < p->ways; i++) {
 		struct cxl_memdev *memdev;
@@ -393,7 +390,8 @@  static int cxl_region_determine_granularity(struct cxl_region *region,
 					    struct parsed_params *p)
 {
 	const char *devname = cxl_region_get_devname(region);
-	unsigned int granularity, ways;
+	unsigned int granularity;
+	int ways;
 
 	/* Default granularity will be the root decoder's granularity */
 	granularity = cxl_decoder_get_interleave_granularity(p->root_decoder);
@@ -408,7 +406,7 @@  static int cxl_region_determine_granularity(struct cxl_region *region,
 		return granularity;
 
 	ways = cxl_decoder_get_interleave_ways(p->root_decoder);
-	if (ways == 0 || ways == UINT_MAX) {
+	if (ways == 0 || ways == -1) {
 		log_err(&rl, "%s: unable to determine root decoder ways\n",
 			devname);
 		return -ENXIO;
@@ -436,12 +434,11 @@  static int create_region(struct cxl_ctx *ctx, int *count,
 {
 	unsigned long flags = UTIL_JSON_TARGETS;
 	struct json_object *jregion;
-	unsigned int i, granularity;
 	struct cxl_region *region;
+	int i, rc, granularity;
 	u64 size, max_extent;
 	const char *devname;
 	uuid_t uuid;
-	int rc;
 
 	rc = create_region_validate_config(ctx, p);
 	if (rc)

[ndctl,09/15] cxl/region: Make ways an integer argument

Commit Message

Comments

Patch