s390/dasd: Improve parameter list parsing

The function dasd_busid() still uses simple_strtoul() to convert a
string to an integer value. This function is obsolete for quite some
time already and should be replaced.

The whole parameter parsing semantic still relies somewhat on the fact,
that simple_strtoul() parses a string containing literals without
complains and just returns the parsed integer value plus the residual
string. kstrtoint(), however, would return -EINVAL in such a case.
Since we want to get rid of simple_strtoul() and now have a nice dasd[]
containing only single elements, we can clean up and simplify a few
things.

Replace simple_strtoul() with kstrtouint(), improve and simplify the
overall parameter parsing by the following:
- instead of residual strings return proper error codes
- remove dasd_parse_next_element() and decide directly what sort of
  element is being parsed
- if we parse a device or a range of devices, split that element into
  separate bits with a new function
- remove warning about invalid ending as it doesn't apply anymore
- annotate all parsing functions and data that can be freed after
  initialisation with __init and __initdata respectively
- clean up bits and pieces while at it
Reviewed-by: Stefan Haberland <sth@linux.vnet.ibm.com>
Reviewed-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.vnet.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

drivers/s390/block/dasd_devmap.c

@@ -107,7 +107,7 @@ dasd_hash_busid(const char *bus_id)
 #ifndef MODULE
 static int __init dasd_call_setup(char *opt)
 {
-	static int i;
+	static int i __initdata;
 	char *tmp;
 
 	while (i < DASD_MAX_PARAMS) {
@@ -129,14 +129,13 @@ __setup ("dasd=", dasd_call_setup);
 /*
  * Read a device busid/devno from a string.
  */
-static int
-
-dasd_busid(char **str, int *id0, int *id1, int *devno)
+static int __init dasd_busid(char *str, int *id0, int *id1, int *devno)
 {
-	int val, old_style;
+	unsigned int val;
+	char *tok;
 
 	/* Interpret ipldev busid */
-	if (strncmp(DASD_IPLDEV, *str, strlen(DASD_IPLDEV)) == 0) {
+	if (strncmp(DASD_IPLDEV, str, strlen(DASD_IPLDEV)) == 0) {
 		if (ipl_info.type != IPL_TYPE_CCW) {
 			pr_err("The IPL device is not a CCW device\n");
 			return -EINVAL;
@@ -144,63 +143,50 @@ dasd_busid(char **str, int *id0, int *id1, int *devno)
 		*id0 = 0;
 		*id1 = ipl_info.data.ccw.dev_id.ssid;
 		*devno = ipl_info.data.ccw.dev_id.devno;
-		*str += strlen(DASD_IPLDEV);
 
 		return 0;
 	}
-	/* check for leading '0x' */
-	old_style = 0;
-	if ((*str)[0] == '0' && (*str)[1] == 'x') {
-		*str += 2;
-		old_style = 1;
-	}
-	if (!isxdigit((*str)[0]))	/* We require at least one hex digit */
-		return -EINVAL;
-	val = simple_strtoul(*str, str, 16);
-	if (old_style || (*str)[0] != '.') {
+
+	/* Old style 0xXXXX or XXXX */
+	if (!kstrtouint(str, 16, &val)) {
 		*id0 = *id1 = 0;
 		if (val < 0 || val > 0xffff)
 			return -EINVAL;
 		*devno = val;
 		return 0;
 	}
+
 	/* New style x.y.z busid */
-	if (val < 0 || val > 0xff)
+	tok = strsep(&str, ".");
+	if (kstrtouint(tok, 16, &val) || val > 0xff)
 		return -EINVAL;
 	*id0 = val;
-	(*str)++;
-	if (!isxdigit((*str)[0]))	/* We require at least one hex digit */
-		return -EINVAL;
-	val = simple_strtoul(*str, str, 16);
-	if (val < 0 || val > 0xff || (*str)++[0] != '.')
+
+	tok = strsep(&str, ".");
+	if (kstrtouint(tok, 16, &val) || val > 0xff)
 		return -EINVAL;
 	*id1 = val;
-	if (!isxdigit((*str)[0]))	/* We require at least one hex digit */
-		return -EINVAL;
-	val = simple_strtoul(*str, str, 16);
-	if (val < 0 || val > 0xffff)
+
+	tok = strsep(&str, ".");
+	if (kstrtouint(tok, 16, &val) || val > 0xffff)
 		return -EINVAL;
 	*devno = val;
+
 	return 0;
 }
 
 /*
- * Read colon separated list of dasd features. Currently there is
- * only one: "ro" for read-only devices. The default feature set
- * is empty (value 0).
+ * Read colon separated list of dasd features.
  */
-static int
-dasd_feature_list(char *str, char **endp)
+static int __init dasd_feature_list(char *str)
 {
 	int features, len, rc;
 
+	features = 0;
 	rc = 0;
-	if (*str != '(') {
-		*endp = str;
+
+	if (!str)
 		return DASD_FEATURE_DEFAULT;
-	}
-	str++;
-	features = 0;
 
 	while (1) {
 		for (len = 0;
@@ -225,15 +211,8 @@ dasd_feature_list(char *str, char **endp)
 			break;
 		str++;
 	}
-	if (*str != ')') {
-		pr_warn("A closing parenthesis ')' is missing in the dasd= parameter\n");
-		rc = -EINVAL;
-	} else
-		str++;
-	*endp = str;
-	if (rc != 0)
-		return rc;
-	return features;
+
+	return rc ? : features;
 }
 
 /*
@@ -242,48 +221,38 @@ dasd_feature_list(char *str, char **endp)
  * action and return a pointer to the residual string. If the first element
  * could not be matched to any keyword then return an error code.
  */
-static char *
-dasd_parse_keyword( char *parsestring ) {
-
-	char *nextcomma, *residual_str;
-	int length;
+static int __init dasd_parse_keyword(char *keyword)
+{
+	int length = strlen(keyword);
 
-	nextcomma = strchr(parsestring,',');
-	if (nextcomma) {
-		length = nextcomma - parsestring;
-		residual_str = nextcomma + 1;
-	} else {
-		length = strlen(parsestring);
-		residual_str = parsestring + length;
-        }
-	if (strncmp("autodetect", parsestring, length) == 0) {
+	if (strncmp("autodetect", keyword, length) == 0) {
 		dasd_autodetect = 1;
 		pr_info("The autodetection mode has been activated\n");
-                return residual_str;
+		return 0;
         }
-	if (strncmp("probeonly", parsestring, length) == 0) {
+	if (strncmp("probeonly", keyword, length) == 0) {
 		dasd_probeonly = 1;
 		pr_info("The probeonly mode has been activated\n");
-                return residual_str;
+		return 0;
         }
-	if (strncmp("nopav", parsestring, length) == 0) {
+	if (strncmp("nopav", keyword, length) == 0) {
 		if (MACHINE_IS_VM)
 			pr_info("'nopav' is not supported on z/VM\n");
 		else {
 			dasd_nopav = 1;
 			pr_info("PAV support has be deactivated\n");
 		}
-		return residual_str;
+		return 0;
 	}
-	if (strncmp("nofcx", parsestring, length) == 0) {
+	if (strncmp("nofcx", keyword, length) == 0) {
 		dasd_nofcx = 1;
 		pr_info("High Performance FICON support has been "
 			"deactivated\n");
-		return residual_str;
+		return 0;
 	}
-	if (strncmp("fixedbuffers", parsestring, length) == 0) {
+	if (strncmp("fixedbuffers", keyword, length) == 0) {
 		if (dasd_page_cache)
-			return residual_str;
+			return 0;
 		dasd_page_cache =
 			kmem_cache_create("dasd_page_cache", PAGE_SIZE,
 					  PAGE_SIZE, SLAB_CACHE_DMA,
@@ -294,73 +263,97 @@ dasd_parse_keyword( char *parsestring ) {
 		else
 			DBF_EVENT(DBF_INFO, "%s",
 				 "turning on fixed buffer mode");
-                return residual_str;
-        }
-	return ERR_PTR(-EINVAL);
+		return 0;
+	}
+
+	return -EINVAL;
 }
 
 /*
- * Try to interprete the first element on the comma separated parse string
- * as a device number or a range of devices. If the interpretation is
- * successful, create the matching dasd_devmap entries and return a pointer
- * to the residual string.
- * If interpretation fails or in case of an error, return an error code.
+ * Split a string of a device range into its pieces and return the from, to, and
+ * feature parts separately.
+ * e.g.:
+ * 0.0.1234-0.0.5678(ro:erplog) -> from: 0.0.1234 to: 0.0.5678 features: ro:erplog
+ * 0.0.8765(raw) -> from: 0.0.8765 to: null features: raw
+ * 0x4321 -> from: 0x4321 to: null features: null
  */
-static char *
-dasd_parse_range( char *parsestring ) {
+static int __init dasd_evaluate_range_param(char *range, char **from_str,
+					    char **to_str, char **features_str)
+{
+	int rc = 0;
 
+	/* Do we have a range or a single device? */
+	if (strchr(range, '-')) {
+		*from_str = strsep(&range, "-");
+		*to_str = strsep(&range, "(");
+		*features_str = strsep(&range, ")");
+	} else {
+		*from_str = strsep(&range, "(");
+		*features_str = strsep(&range, ")");
+	}
+
+	if (*features_str && !range) {
+		pr_warn("A closing parenthesis ')' is missing in the dasd= parameter\n");
+		rc = -EINVAL;
+	}
+
+	return rc;
+}
+
+/*
+ * Try to interprete the range string as a device number or a range of devices.
+ * If the interpretation is successful, create the matching dasd_devmap entries.
+ * If interpretation fails or in case of an error, return an error code.
+ */
+static int __init dasd_parse_range(const char *range)
+{
 	struct dasd_devmap *devmap;
 	int from, from_id0, from_id1;
 	int to, to_id0, to_id1;
-	int features, rc;
-	char bus_id[DASD_BUS_ID_SIZE+1], *str;
-
-	str = parsestring;
-	rc = dasd_busid(&str, &from_id0, &from_id1, &from);
-	if (rc == 0) {
-		to = from;
-		to_id0 = from_id0;
-		to_id1 = from_id1;
-		if (*str == '-') {
-			str++;
-			rc = dasd_busid(&str, &to_id0, &to_id1, &to);
+	int features;
+	char bus_id[DASD_BUS_ID_SIZE + 1];
+	char *features_str = NULL;
+	char *from_str = NULL;
+	char *to_str = NULL;
+	size_t len = strlen(range) + 1;
+	char tmp[len];
+
+	strlcpy(tmp, range, len);
+
+	if (dasd_evaluate_range_param(tmp, &from_str, &to_str, &features_str))
+		goto out_err;
+
+	if (dasd_busid(from_str, &from_id0, &from_id1, &from))
+		goto out_err;
+
+	to = from;
+	to_id0 = from_id0;
+	to_id1 = from_id1;
+	if (to_str) {
+		if (dasd_busid(to_str, &to_id0, &to_id1, &to))
+			goto out_err;
+		if (from_id0 != to_id0 || from_id1 != to_id1 || from > to) {
+			pr_err("%s is not a valid device range\n", range);
+			goto out_err;
 		}
 	}
-	if (rc == 0 &&
-	    (from_id0 != to_id0 || from_id1 != to_id1 || from > to))
-		rc = -EINVAL;
-	if (rc) {
-		pr_err("%s is not a valid device range\n", parsestring);
-		return ERR_PTR(rc);
-	}
-	features = dasd_feature_list(str, &str);
+
+	features = dasd_feature_list(features_str);
 	if (features < 0)
-		return ERR_PTR(-EINVAL);
+		goto out_err;
 	/* each device in dasd= parameter should be set initially online */
 	features |= DASD_FEATURE_INITIAL_ONLINE;
 	while (from <= to) {
-		sprintf(bus_id, "%01x.%01x.%04x",
-			from_id0, from_id1, from++);
+		sprintf(bus_id, "%01x.%01x.%04x", from_id0, from_id1, from++);
 		devmap = dasd_add_busid(bus_id, features);
 		if (IS_ERR(devmap))
-			return (char *)devmap;
+			return PTR_ERR(devmap);
 	}
-	if (*str == ',')
-		return str + 1;
-	if (*str == '\0')
-		return str;
-	pr_warn("The dasd= parameter value %s has an invalid ending\n", str);
-	return ERR_PTR(-EINVAL);
-}
 
-static char *
-dasd_parse_next_element( char *parsestring ) {
-	char * residual_str;
-	residual_str = dasd_parse_keyword(parsestring);
-	if (!IS_ERR(residual_str))
-		return residual_str;
-	residual_str = dasd_parse_range(parsestring);
-	return residual_str;
+	return 0;
+
+out_err:
+	return -EINVAL;
 }
 
 /*
@@ -369,30 +362,27 @@ dasd_parse_next_element( char *parsestring ) {
  * keywords and device ranges. The parameters in that list will be stored as
  * separate elementes in dasd[].
  */
-int
-dasd_parse(void)
+int __init dasd_parse(void)
 {
 	int rc, i;
-	char *parsestring;
+	char *cur;
 
 	rc = 0;
 	for (i = 0; i < DASD_MAX_PARAMS; i++) {
-		if (dasd[i] == NULL)
+		cur = dasd[i];
+		if (!cur)
 			break;
-		parsestring = dasd[i];
-		/* loop over the comma separated list in the parsestring */
-		while (*parsestring) {
-			parsestring = dasd_parse_next_element(parsestring);
-			if(IS_ERR(parsestring)) {
-				rc = PTR_ERR(parsestring);
-				break;
-			}
-		}
-		if (rc) {
-			DBF_EVENT(DBF_ALERT, "%s", "invalid range found");
+		if (*cur == '\0')
+			continue;
+
+		rc = dasd_parse_keyword(cur);
+		if (rc)
+			rc = dasd_parse_range(cur);
+
+		if (rc)
 			break;
-		}
 	}
+
 	return rc;
 }
 

drivers/s390/block/dasd_int.h

@@ -805,7 +805,7 @@ struct dasd_device *dasd_device_from_devindex(int);
 void dasd_add_link_to_gendisk(struct gendisk *, struct dasd_device *);
 struct dasd_device *dasd_device_from_gendisk(struct gendisk *);
 
-int dasd_parse(void);
+int dasd_parse(void) __init;
 int dasd_busid_known(const char *);
 
 /* externals in dasd_gendisk.c */