[PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

Add strsplit_u32() and its __user variant to allow for splitting
specified string into array of u32 tokens.

Originally this functionality was added for the SOF sound driver. As
more users are on the horizon, relocate it so it becomes a common good.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/linux/string_helpers.h    |  3 +
 lib/string_helpers.c              | 96 +++++++++++++++++++++++++++++++
 sound/soc/sof/sof-client-probes.c | 51 +---------------
 3 files changed, 100 insertions(+), 50 deletions(-)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 4d72258d42fd..a4630ddfca27 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -126,4 +126,7 @@ static inline const char *str_enabled_disabled(bool v)
 	return v ? "enabled" : "disabled";
 }
 
+int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns);
+int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
+		      u32 **tkns, size_t *num_tkns);
 #endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5ed3beb066e6..bb24f0c62539 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -984,3 +984,99 @@ void fortify_panic(const char *name)
 }
 EXPORT_SYMBOL(fortify_panic);
 #endif /* CONFIG_FORTIFY_SOURCE */
+
+/**
+ * strsplit_u32 - Split string into sequence of u32 tokens
+ * @str:	The string to split into tokens.
+ * @delim:	The string containing delimiter characters.
+ * @tkns:	Returned u32 sequence pointer.
+ * @num_tkns:	Returned number of tokens obtained.
+ *
+ * On success @num_tkns and @tkns are assigned the number of tokens extracted
+ * and the array itself respectively.
+ * Caller takes responsibility for freeing @tkns when no longer needed.
+ */
+int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns)
+{
+	size_t max_count = 32;
+	size_t count = 0;
+	char *s, **p;
+	u32 *buf, *tmp;
+	int ret = 0;
+
+	p = (char **)&str;
+	*tkns = NULL;
+	*num_tkns = 0;
+
+	buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	while ((s = strsep(p, delim)) != NULL) {
+		ret = kstrtouint(s, 0, buf + count);
+		if (ret)
+			goto free_buf;
+
+		if (++count > max_count) {
+			max_count *= 2;
+			tmp = krealloc(buf, max_count * sizeof(*buf), GFP_KERNEL);
+			if (!tmp) {
+				ret = -ENOMEM;
+				goto free_buf;
+			}
+			buf = tmp;
+		}
+	}
+
+	if (!count)
+		goto free_buf;
+	*tkns = kmemdup(buf, count * sizeof(*buf), GFP_KERNEL);
+	if (*tkns == NULL) {
+		ret = -ENOMEM;
+		goto free_buf;
+	}
+	*num_tkns = count;
+
+free_buf:
+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL(strsplit_u32);
+
+/**
+ * strsplit_u32_user - Split string into sequence of u32 tokens
+ * @from:	The user space buffer to read from
+ * @ppos:	The current position in the buffer
+ * @count:	The maximum number of bytes to read
+ * @delim:	The string containing delimiter characters.
+ * @tkns:	Returned u32 sequence pointer.
+ * @num_tkns:	Returned number of tokens obtained.
+ *
+ * On success @num_tkns and @tkns are assigned the number of tokens extracted
+ * and the array itself respectively.
+ * Caller takes responsibility for freeing @tkns when no longer needed.
+ */
+int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
+		      u32 **tkns, size_t *num_tkns)
+{
+	char *buf;
+	int ret;
+
+	buf = kmalloc(count + 1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = simple_write_to_buffer(buf, count, ppos, from, count);
+	if (ret != count) {
+		ret = (ret < 0) ? ret : -EIO;
+		goto free_buf;
+	}
+
+	buf[count] = '\0';
+	ret = strsplit_u32(buf, delim, tkns, num_tkns);
+
+free_buf:
+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL(strsplit_u32_user);
diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 1f1ea93a7fbf..48ebbe58e2b9 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -12,6 +12,7 @@
 #include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/string_helpers.h>
 #include <sound/soc.h>
 #include <sound/sof/header.h>
 #include "sof-client.h"
@@ -410,56 +411,6 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
 	.copy = sof_probes_compr_copy,
 };
 
-/**
- * strsplit_u32 - Split string into sequence of u32 tokens
- * @buf:	String to split into tokens.
- * @delim:	String containing delimiter characters.
- * @tkns:	Returned u32 sequence pointer.
- * @num_tkns:	Returned number of tokens obtained.
- */
-static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tkns)
-{
-	char *s;
-	u32 *data, *tmp;
-	size_t count = 0;
-	size_t cap = 32;
-	int ret = 0;
-
-	*tkns = NULL;
-	*num_tkns = 0;
-	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	while ((s = strsep(&buf, delim)) != NULL) {
-		ret = kstrtouint(s, 0, data + count);
-		if (ret)
-			goto exit;
-		if (++count >= cap) {
-			cap *= 2;
-			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
-			if (!tmp) {
-				ret = -ENOMEM;
-				goto exit;
-			}
-			data = tmp;
-		}
-	}
-
-	if (!count)
-		goto exit;
-	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
-	if (!(*tkns)) {
-		ret = -ENOMEM;
-		goto exit;
-	}
-	*num_tkns = count;
-
-exit:
-	kfree(data);
-	return ret;
-}
-
 static int tokenize_input(const char __user *from, size_t count,
 			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
 {
-- 
2.25.1

[PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

Add strsplit_u32() and its __user variant to allow for splitting
specified string into array of u32 tokens.

Originally this functionality was added for the SOF sound driver. As
more users are on the horizon, relocate it so it becomes a common good.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/linux/string_helpers.h    |  3 +
 lib/string_helpers.c              | 96 +++++++++++++++++++++++++++++++
 sound/soc/sof/sof-client-probes.c | 51 +---------------
 3 files changed, 100 insertions(+), 50 deletions(-)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 4d72258d42fd..a4630ddfca27 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -126,4 +126,7 @@ static inline const char *str_enabled_disabled(bool v)
 	return v ? "enabled" : "disabled";
 }
 
+int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns);
+int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
+		      u32 **tkns, size_t *num_tkns);
 #endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5ed3beb066e6..bb24f0c62539 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -984,3 +984,99 @@ void fortify_panic(const char *name)
 }
 EXPORT_SYMBOL(fortify_panic);
 #endif /* CONFIG_FORTIFY_SOURCE */
+
+/**
+ * strsplit_u32 - Split string into sequence of u32 tokens
+ * @str:	The string to split into tokens.
+ * @delim:	The string containing delimiter characters.
+ * @tkns:	Returned u32 sequence pointer.
+ * @num_tkns:	Returned number of tokens obtained.
+ *
+ * On success @num_tkns and @tkns are assigned the number of tokens extracted
+ * and the array itself respectively.
+ * Caller takes responsibility for freeing @tkns when no longer needed.
+ */
+int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns)
+{
+	size_t max_count = 32;
+	size_t count = 0;
+	char *s, **p;
+	u32 *buf, *tmp;
+	int ret = 0;
+
+	p = (char **)&str;
+	*tkns = NULL;
+	*num_tkns = 0;
+
+	buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	while ((s = strsep(p, delim)) != NULL) {
+		ret = kstrtouint(s, 0, buf + count);
+		if (ret)
+			goto free_buf;
+
+		if (++count > max_count) {
+			max_count *= 2;
+			tmp = krealloc(buf, max_count * sizeof(*buf), GFP_KERNEL);
+			if (!tmp) {
+				ret = -ENOMEM;
+				goto free_buf;
+			}
+			buf = tmp;
+		}
+	}
+
+	if (!count)
+		goto free_buf;
+	*tkns = kmemdup(buf, count * sizeof(*buf), GFP_KERNEL);
+	if (*tkns == NULL) {
+		ret = -ENOMEM;
+		goto free_buf;
+	}
+	*num_tkns = count;
+
+free_buf:
+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL(strsplit_u32);
+
+/**
+ * strsplit_u32_user - Split string into sequence of u32 tokens
+ * @from:	The user space buffer to read from
+ * @ppos:	The current position in the buffer
+ * @count:	The maximum number of bytes to read
+ * @delim:	The string containing delimiter characters.
+ * @tkns:	Returned u32 sequence pointer.
+ * @num_tkns:	Returned number of tokens obtained.
+ *
+ * On success @num_tkns and @tkns are assigned the number of tokens extracted
+ * and the array itself respectively.
+ * Caller takes responsibility for freeing @tkns when no longer needed.
+ */
+int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
+		      u32 **tkns, size_t *num_tkns)
+{
+	char *buf;
+	int ret;
+
+	buf = kmalloc(count + 1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = simple_write_to_buffer(buf, count, ppos, from, count);
+	if (ret != count) {
+		ret = (ret < 0) ? ret : -EIO;
+		goto free_buf;
+	}
+
+	buf[count] = '\0';
+	ret = strsplit_u32(buf, delim, tkns, num_tkns);
+
+free_buf:
+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL(strsplit_u32_user);
diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 1f1ea93a7fbf..48ebbe58e2b9 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -12,6 +12,7 @@
 #include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/string_helpers.h>
 #include <sound/soc.h>
 #include <sound/sof/header.h>
 #include "sof-client.h"
@@ -410,56 +411,6 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
 	.copy = sof_probes_compr_copy,
 };
 
-/**
- * strsplit_u32 - Split string into sequence of u32 tokens
- * @buf:	String to split into tokens.
- * @delim:	String containing delimiter characters.
- * @tkns:	Returned u32 sequence pointer.
- * @num_tkns:	Returned number of tokens obtained.
- */
-static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tkns)
-{
-	char *s;
-	u32 *data, *tmp;
-	size_t count = 0;
-	size_t cap = 32;
-	int ret = 0;
-
-	*tkns = NULL;
-	*num_tkns = 0;
-	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	while ((s = strsep(&buf, delim)) != NULL) {
-		ret = kstrtouint(s, 0, data + count);
-		if (ret)
-			goto exit;
-		if (++count >= cap) {
-			cap *= 2;
-			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
-			if (!tmp) {
-				ret = -ENOMEM;
-				goto exit;
-			}
-			data = tmp;
-		}
-	}
-
-	if (!count)
-		goto exit;
-	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
-	if (!(*tkns)) {
-		ret = -ENOMEM;
-		goto exit;
-	}
-	*num_tkns = count;
-
-exit:
-	kfree(data);
-	return ret;
-}
-
 static int tokenize_input(const char __user *from, size_t count,
 			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
 {
-- 
2.25.1

[PATCH 2/2] ASoC: SOF: Remove tokenize_input()

[Cezary Rojewski]

Now that we have strsplit_u32_user() tokenize_input() is needed no
longer. Remove it and update all occurrences of its usage.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/sof/sof-client-probes.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 48ebbe58e2b9..7a79a529e2b7 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -411,29 +411,6 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
 	.copy = sof_probes_compr_copy,
 };
 
-static int tokenize_input(const char __user *from, size_t count,
-			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
-{
-	char *buf;
-	int ret;
-
-	buf = kmalloc(count + 1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = simple_write_to_buffer(buf, count, ppos, from, count);
-	if (ret != count) {
-		ret = ret >= 0 ? -EIO : ret;
-		goto exit;
-	}
-
-	buf[count] = '\0';
-	ret = strsplit_u32(buf, ",", tkns, num_tkns);
-exit:
-	kfree(buf);
-	return ret;
-}
-
 static ssize_t sof_probes_dfs_points_read(struct file *file, char __user *to,
 					  size_t count, loff_t *ppos)
 {
@@ -508,7 +485,7 @@ sof_probes_dfs_points_write(struct file *file, const char __user *from,
 		return -ENOENT;
 	}
 
-	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	ret = strsplit_u32_user(from, count, ppos, ",", &tkns, &num_tkns);
 	if (ret < 0)
 		return ret;
 	bytes = sizeof(*tkns) * num_tkns;
@@ -563,7 +540,7 @@ sof_probes_dfs_points_remove_write(struct file *file, const char __user *from,
 		return -ENOENT;
 	}
 
-	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	ret = strsplit_u32_user(from, count, ppos, ",", &tkns, &num_tkns);
 	if (ret < 0)
 		return ret;
 	if (!num_tkns) {
-- 
2.25.1

[PATCH 2/2] ASoC: SOF: Remove tokenize_input()

[Cezary Rojewski]

Now that we have strsplit_u32_user() tokenize_input() is needed no
longer. Remove it and update all occurrences of its usage.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/sof/sof-client-probes.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 48ebbe58e2b9..7a79a529e2b7 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -411,29 +411,6 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
 	.copy = sof_probes_compr_copy,
 };
 
-static int tokenize_input(const char __user *from, size_t count,
-			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
-{
-	char *buf;
-	int ret;
-
-	buf = kmalloc(count + 1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = simple_write_to_buffer(buf, count, ppos, from, count);
-	if (ret != count) {
-		ret = ret >= 0 ? -EIO : ret;
-		goto exit;
-	}
-
-	buf[count] = '\0';
-	ret = strsplit_u32(buf, ",", tkns, num_tkns);
-exit:
-	kfree(buf);
-	return ret;
-}
-
 static ssize_t sof_probes_dfs_points_read(struct file *file, char __user *to,
 					  size_t count, loff_t *ppos)
 {
@@ -508,7 +485,7 @@ sof_probes_dfs_points_write(struct file *file, const char __user *from,
 		return -ENOENT;
 	}
 
-	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	ret = strsplit_u32_user(from, count, ppos, ",", &tkns, &num_tkns);
 	if (ret < 0)
 		return ret;
 	bytes = sizeof(*tkns) * num_tkns;
@@ -563,7 +540,7 @@ sof_probes_dfs_points_remove_write(struct file *file, const char __user *from,
 		return -ENOENT;
 	}
 
-	ret = tokenize_input(from, count, ppos, &tkns, &num_tkns);
+	ret = strsplit_u32_user(from, count, ppos, ",", &tkns, &num_tkns);
 	if (ret < 0)
 		return ret;
 	if (!num_tkns) {
-- 
2.25.1

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Péter Ujfalusi]

On 07/07/2022 12:13, Cezary Rojewski wrote:
> Add strsplit_u32() and its __user variant to allow for splitting
> specified string into array of u32 tokens.
> 
> Originally this functionality was added for the SOF sound driver. As
> more users are on the horizon, relocate it so it becomes a common good.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/linux/string_helpers.h    |  3 +
>  lib/string_helpers.c              | 96 +++++++++++++++++++++++++++++++
>  sound/soc/sof/sof-client-probes.c | 51 +---------------
>  3 files changed, 100 insertions(+), 50 deletions(-)
> 
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 4d72258d42fd..a4630ddfca27 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -126,4 +126,7 @@ static inline const char *str_enabled_disabled(bool v)
>  	return v ? "enabled" : "disabled";
>  }
>  
> +int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns);
> +int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
> +		      u32 **tkns, size_t *num_tkns);
>  #endif
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5ed3beb066e6..bb24f0c62539 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -984,3 +984,99 @@ void fortify_panic(const char *name)
>  }
>  EXPORT_SYMBOL(fortify_panic);
>  #endif /* CONFIG_FORTIFY_SOURCE */
> +
> +/**
> + * strsplit_u32 - Split string into sequence of u32 tokens
> + * @str:	The string to split into tokens.
> + * @delim:	The string containing delimiter characters.
> + * @tkns:	Returned u32 sequence pointer.
> + * @num_tkns:	Returned number of tokens obtained.
> + *
> + * On success @num_tkns and @tkns are assigned the number of tokens extracted
> + * and the array itself respectively.
> + * Caller takes responsibility for freeing @tkns when no longer needed.
> + */
> +int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns)
> +{
> +	size_t max_count = 32;
> +	size_t count = 0;
> +	char *s, **p;
> +	u32 *buf, *tmp;
> +	int ret = 0;
> +
> +	p = (char **)&str;
> +	*tkns = NULL;
> +	*num_tkns = 0;
> +
> +	buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	while ((s = strsep(p, delim)) != NULL) {
> +		ret = kstrtouint(s, 0, buf + count);
> +		if (ret)
> +			goto free_buf;
> +
> +		if (++count > max_count) {

I think this should be as it was originally:
if (++count >= max_count) {

Otherwise when we reach the max_count we would not realloc to get more
space and the data + max_count is pointing outside of the allocated area.

> +			max_count *= 2;
> +			tmp = krealloc(buf, max_count * sizeof(*buf), GFP_KERNEL);
> +			if (!tmp) {
> +				ret = -ENOMEM;
> +				goto free_buf;
> +			}
> +			buf = tmp;
> +		}
> +	}
> +
> +	if (!count)
> +		goto free_buf;
> +	*tkns = kmemdup(buf, count * sizeof(*buf), GFP_KERNEL);
> +	if (*tkns == NULL) {
> +		ret = -ENOMEM;
> +		goto free_buf;
> +	}
> +	*num_tkns = count;
> +
> +free_buf:
> +	kfree(buf);
> +	return ret;
> +}
> +EXPORT_SYMBOL(strsplit_u32);
> +
> +/**
> + * strsplit_u32_user - Split string into sequence of u32 tokens
> + * @from:	The user space buffer to read from
> + * @ppos:	The current position in the buffer
> + * @count:	The maximum number of bytes to read
> + * @delim:	The string containing delimiter characters.
> + * @tkns:	Returned u32 sequence pointer.
> + * @num_tkns:	Returned number of tokens obtained.
> + *
> + * On success @num_tkns and @tkns are assigned the number of tokens extracted
> + * and the array itself respectively.
> + * Caller takes responsibility for freeing @tkns when no longer needed.
> + */
> +int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
> +		      u32 **tkns, size_t *num_tkns)
> +{
> +	char *buf;
> +	int ret;
> +
> +	buf = kmalloc(count + 1, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = simple_write_to_buffer(buf, count, ppos, from, count);
> +	if (ret != count) {
> +		ret = (ret < 0) ? ret : -EIO;
> +		goto free_buf;
> +	}
> +
> +	buf[count] = '\0';
> +	ret = strsplit_u32(buf, delim, tkns, num_tkns);
> +
> +free_buf:
> +	kfree(buf);
> +	return ret;
> +}
> +EXPORT_SYMBOL(strsplit_u32_user);
> diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
> index 1f1ea93a7fbf..48ebbe58e2b9 100644
> --- a/sound/soc/sof/sof-client-probes.c
> +++ b/sound/soc/sof/sof-client-probes.c
> @@ -12,6 +12,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/string_helpers.h>
>  #include <sound/soc.h>
>  #include <sound/sof/header.h>
>  #include "sof-client.h"
> @@ -410,56 +411,6 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
>  	.copy = sof_probes_compr_copy,
>  };
>  
> -/**
> - * strsplit_u32 - Split string into sequence of u32 tokens
> - * @buf:	String to split into tokens.
> - * @delim:	String containing delimiter characters.
> - * @tkns:	Returned u32 sequence pointer.
> - * @num_tkns:	Returned number of tokens obtained.
> - */
> -static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tkns)
> -{
> -	char *s;
> -	u32 *data, *tmp;
> -	size_t count = 0;
> -	size_t cap = 32;
> -	int ret = 0;
> -
> -	*tkns = NULL;
> -	*num_tkns = 0;
> -	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	while ((s = strsep(&buf, delim)) != NULL) {
> -		ret = kstrtouint(s, 0, data + count);
> -		if (ret)
> -			goto exit;
> -		if (++count >= cap) {
> -			cap *= 2;
> -			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
> -			if (!tmp) {
> -				ret = -ENOMEM;
> -				goto exit;
> -			}
> -			data = tmp;
> -		}
> -	}
> -
> -	if (!count)
> -		goto exit;
> -	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
> -	if (!(*tkns)) {
> -		ret = -ENOMEM;
> -		goto exit;
> -	}
> -	*num_tkns = count;
> -
> -exit:
> -	kfree(data);
> -	return ret;
> -}
> -
>  static int tokenize_input(const char __user *from, size_t count,
>  			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
>  {

-- 
Péter

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Péter Ujfalusi]

On 07/07/2022 12:13, Cezary Rojewski wrote:
> Add strsplit_u32() and its __user variant to allow for splitting
> specified string into array of u32 tokens.
> 
> Originally this functionality was added for the SOF sound driver. As
> more users are on the horizon, relocate it so it becomes a common good.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/linux/string_helpers.h    |  3 +
>  lib/string_helpers.c              | 96 +++++++++++++++++++++++++++++++
>  sound/soc/sof/sof-client-probes.c | 51 +---------------
>  3 files changed, 100 insertions(+), 50 deletions(-)
> 
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 4d72258d42fd..a4630ddfca27 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -126,4 +126,7 @@ static inline const char *str_enabled_disabled(bool v)
>  	return v ? "enabled" : "disabled";
>  }
>  
> +int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns);
> +int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
> +		      u32 **tkns, size_t *num_tkns);
>  #endif
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5ed3beb066e6..bb24f0c62539 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -984,3 +984,99 @@ void fortify_panic(const char *name)
>  }
>  EXPORT_SYMBOL(fortify_panic);
>  #endif /* CONFIG_FORTIFY_SOURCE */
> +
> +/**
> + * strsplit_u32 - Split string into sequence of u32 tokens
> + * @str:	The string to split into tokens.
> + * @delim:	The string containing delimiter characters.
> + * @tkns:	Returned u32 sequence pointer.
> + * @num_tkns:	Returned number of tokens obtained.
> + *
> + * On success @num_tkns and @tkns are assigned the number of tokens extracted
> + * and the array itself respectively.
> + * Caller takes responsibility for freeing @tkns when no longer needed.
> + */
> +int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns)
> +{
> +	size_t max_count = 32;
> +	size_t count = 0;
> +	char *s, **p;
> +	u32 *buf, *tmp;
> +	int ret = 0;
> +
> +	p = (char **)&str;
> +	*tkns = NULL;
> +	*num_tkns = 0;
> +
> +	buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	while ((s = strsep(p, delim)) != NULL) {
> +		ret = kstrtouint(s, 0, buf + count);
> +		if (ret)
> +			goto free_buf;
> +
> +		if (++count > max_count) {

I think this should be as it was originally:
if (++count >= max_count) {

Otherwise when we reach the max_count we would not realloc to get more
space and the data + max_count is pointing outside of the allocated area.

> +			max_count *= 2;
> +			tmp = krealloc(buf, max_count * sizeof(*buf), GFP_KERNEL);
> +			if (!tmp) {
> +				ret = -ENOMEM;
> +				goto free_buf;
> +			}
> +			buf = tmp;
> +		}
> +	}
> +
> +	if (!count)
> +		goto free_buf;
> +	*tkns = kmemdup(buf, count * sizeof(*buf), GFP_KERNEL);
> +	if (*tkns == NULL) {
> +		ret = -ENOMEM;
> +		goto free_buf;
> +	}
> +	*num_tkns = count;
> +
> +free_buf:
> +	kfree(buf);
> +	return ret;
> +}
> +EXPORT_SYMBOL(strsplit_u32);
> +
> +/**
> + * strsplit_u32_user - Split string into sequence of u32 tokens
> + * @from:	The user space buffer to read from
> + * @ppos:	The current position in the buffer
> + * @count:	The maximum number of bytes to read
> + * @delim:	The string containing delimiter characters.
> + * @tkns:	Returned u32 sequence pointer.
> + * @num_tkns:	Returned number of tokens obtained.
> + *
> + * On success @num_tkns and @tkns are assigned the number of tokens extracted
> + * and the array itself respectively.
> + * Caller takes responsibility for freeing @tkns when no longer needed.
> + */
> +int strsplit_u32_user(const char __user *from, size_t count, loff_t *ppos, const char *delim,
> +		      u32 **tkns, size_t *num_tkns)
> +{
> +	char *buf;
> +	int ret;
> +
> +	buf = kmalloc(count + 1, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = simple_write_to_buffer(buf, count, ppos, from, count);
> +	if (ret != count) {
> +		ret = (ret < 0) ? ret : -EIO;
> +		goto free_buf;
> +	}
> +
> +	buf[count] = '\0';
> +	ret = strsplit_u32(buf, delim, tkns, num_tkns);
> +
> +free_buf:
> +	kfree(buf);
> +	return ret;
> +}
> +EXPORT_SYMBOL(strsplit_u32_user);
> diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
> index 1f1ea93a7fbf..48ebbe58e2b9 100644
> --- a/sound/soc/sof/sof-client-probes.c
> +++ b/sound/soc/sof/sof-client-probes.c
> @@ -12,6 +12,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/string_helpers.h>
>  #include <sound/soc.h>
>  #include <sound/sof/header.h>
>  #include "sof-client.h"
> @@ -410,56 +411,6 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
>  	.copy = sof_probes_compr_copy,
>  };
>  
> -/**
> - * strsplit_u32 - Split string into sequence of u32 tokens
> - * @buf:	String to split into tokens.
> - * @delim:	String containing delimiter characters.
> - * @tkns:	Returned u32 sequence pointer.
> - * @num_tkns:	Returned number of tokens obtained.
> - */
> -static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tkns)
> -{
> -	char *s;
> -	u32 *data, *tmp;
> -	size_t count = 0;
> -	size_t cap = 32;
> -	int ret = 0;
> -
> -	*tkns = NULL;
> -	*num_tkns = 0;
> -	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	while ((s = strsep(&buf, delim)) != NULL) {
> -		ret = kstrtouint(s, 0, data + count);
> -		if (ret)
> -			goto exit;
> -		if (++count >= cap) {
> -			cap *= 2;
> -			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
> -			if (!tmp) {
> -				ret = -ENOMEM;
> -				goto exit;
> -			}
> -			data = tmp;
> -		}
> -	}
> -
> -	if (!count)
> -		goto exit;
> -	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
> -	if (!(*tkns)) {
> -		ret = -ENOMEM;
> -		goto exit;
> -	}
> -	*num_tkns = count;
> -
> -exit:
> -	kfree(data);
> -	return ret;
> -}
> -
>  static int tokenize_input(const char __user *from, size_t count,
>  			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
>  {

-- 
Péter

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> Add strsplit_u32() and its __user variant to allow for splitting
> specified string into array of u32 tokens.

And I believe we have more of this done in old code.
Since all callers use ',' as a delimiter, have you considered using
get_options()?

> Originally this functionality was added for the SOF sound driver. As
> more users are on the horizon, relocate it so it becomes a common good.

Maybe it can be fixed just there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> Add strsplit_u32() and its __user variant to allow for splitting
> specified string into array of u32 tokens.

And I believe we have more of this done in old code.
Since all callers use ',' as a delimiter, have you considered using
get_options()?

> Originally this functionality was added for the SOF sound driver. As
> more users are on the horizon, relocate it so it becomes a common good.

Maybe it can be fixed just there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 12:22 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
> >
> > Add strsplit_u32() and its __user variant to allow for splitting
> > specified string into array of u32 tokens.
>
> And I believe we have more of this done in old code.
> Since all callers use ',' as a delimiter, have you considered using
> get_options()?
>
> > Originally this functionality was added for the SOF sound driver. As
> > more users are on the horizon, relocate it so it becomes a common good.
>
> Maybe it can be fixed just there.

Forgot to add that we (trying to) don't accept new code in the lib
w.o. test cases. get_options() is somehow covered. If you have
different test cases in mind, do not hesitate to add!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 12:22 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
> >
> > Add strsplit_u32() and its __user variant to allow for splitting
> > specified string into array of u32 tokens.
>
> And I believe we have more of this done in old code.
> Since all callers use ',' as a delimiter, have you considered using
> get_options()?
>
> > Originally this functionality was added for the SOF sound driver. As
> > more users are on the horizon, relocate it so it becomes a common good.
>
> Maybe it can be fixed just there.

Forgot to add that we (trying to) don't accept new code in the lib
w.o. test cases. get_options() is somehow covered. If you have
different test cases in mind, do not hesitate to add!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 12:22 PM, Andy Shevchenko wrote:
> On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
>>
>> Add strsplit_u32() and its __user variant to allow for splitting
>> specified string into array of u32 tokens.
> 
> And I believe we have more of this done in old code.
> Since all callers use ',' as a delimiter, have you considered using
> get_options()?


Thanks for your input, Andy.

When I'd written the very first version of this function many months 
ago, get_options() looked as it does not fulfill our needs. It seems to 
be true even today: caller needs to know the number of elements in an 
array upfront. Also, kstrtox() takes into account '0x' and modifies the 
base accordingly if that's the case. simple_strtoull() looks as not 
capable of doing the same thing.

The goal is to be able to parse input such as:

0x1000003,0,0,0x1000004,0,0

into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.

>> Originally this functionality was added for the SOF sound driver. As
>> more users are on the horizon, relocate it so it becomes a common good.
> 
> Maybe it can be fixed just there.

avs-driver, which is also part of the ASoC framework has very similar 
debug-interface. I believe there's no need to duplicate the functions - 
move them to common code instead.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 12:22 PM, Andy Shevchenko wrote:
> On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
>>
>> Add strsplit_u32() and its __user variant to allow for splitting
>> specified string into array of u32 tokens.
> 
> And I believe we have more of this done in old code.
> Since all callers use ',' as a delimiter, have you considered using
> get_options()?


Thanks for your input, Andy.

When I'd written the very first version of this function many months 
ago, get_options() looked as it does not fulfill our needs. It seems to 
be true even today: caller needs to know the number of elements in an 
array upfront. Also, kstrtox() takes into account '0x' and modifies the 
base accordingly if that's the case. simple_strtoull() looks as not 
capable of doing the same thing.

The goal is to be able to parse input such as:

0x1000003,0,0,0x1000004,0,0

into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.

>> Originally this functionality was added for the SOF sound driver. As
>> more users are on the horizon, relocate it so it becomes a common good.
> 
> Maybe it can be fixed just there.

avs-driver, which is also part of the ASoC framework has very similar 
debug-interface. I believe there's no need to duplicate the functions - 
move them to common code instead.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-07 3:51 PM, Péter Ujfalusi wrote:
> On 07/07/2022 12:13, Cezary Rojewski wrote:

...

>> +int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns)
>> +{
>> +	size_t max_count = 32;
>> +	size_t count = 0;
>> +	char *s, **p;
>> +	u32 *buf, *tmp;
>> +	int ret = 0;
>> +
>> +	p = (char **)&str;
>> +	*tkns = NULL;
>> +	*num_tkns = 0;
>> +
>> +	buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	while ((s = strsep(p, delim)) != NULL) {
>> +		ret = kstrtouint(s, 0, buf + count);
>> +		if (ret)
>> +			goto free_buf;
>> +
>> +		if (++count > max_count) {
> 
> I think this should be as it was originally:
> if (++count >= max_count) {
> 
> Otherwise when we reach the max_count we would not realloc to get more
> space and the data + max_count is pointing outside of the allocated area.

I believe you're right. Will change in v2.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-07 3:51 PM, Péter Ujfalusi wrote:
> On 07/07/2022 12:13, Cezary Rojewski wrote:

...

>> +int strsplit_u32(const char *str, const char *delim, u32 **tkns, size_t *num_tkns)
>> +{
>> +	size_t max_count = 32;
>> +	size_t count = 0;
>> +	char *s, **p;
>> +	u32 *buf, *tmp;
>> +	int ret = 0;
>> +
>> +	p = (char **)&str;
>> +	*tkns = NULL;
>> +	*num_tkns = 0;
>> +
>> +	buf = kcalloc(max_count, sizeof(*buf), GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	while ((s = strsep(p, delim)) != NULL) {
>> +		ret = kstrtouint(s, 0, buf + count);
>> +		if (ret)
>> +			goto free_buf;
>> +
>> +		if (++count > max_count) {
> 
> I think this should be as it was originally:
> if (++count >= max_count) {
> 
> Otherwise when we reach the max_count we would not realloc to get more
> space and the data + max_count is pointing outside of the allocated area.

I believe you're right. Will change in v2.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
> On 2022-07-08 12:22 PM, Andy Shevchenko wrote:
> > On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
> > <cezary.rojewski@intel.com> wrote:
> >>
> >> Add strsplit_u32() and its __user variant to allow for splitting
> >> specified string into array of u32 tokens.
> >
> > And I believe we have more of this done in old code.
> > Since all callers use ',' as a delimiter, have you considered using
> > get_options()?
>
>
> Thanks for your input, Andy.
>
> When I'd written the very first version of this function many months
> ago, get_options() looked as it does not fulfill our needs. It seems to
> be true even today: caller needs to know the number of elements in an
> array upfront.

Have you read a kernel doc for it? It does return the number of
elements at the first pass.

> Also, kstrtox() takes into account '0x' and modifies the
> base accordingly if that's the case. simple_strtoull() looks as not
> capable of doing the same thing.

How come?! It does parse all known prefixes: 0x, 0, +, -.

> The goal is to be able to parse input such as:
>
> 0x1000003,0,0,0x1000004,0,0
>
> into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.

Yes. Have you checked the test cases for get_options()?

> >> Originally this functionality was added for the SOF sound driver. As
> >> more users are on the horizon, relocate it so it becomes a common good.
> >
> > Maybe it can be fixed just there.
>
> avs-driver, which is also part of the ASoC framework has very similar
> debug-interface. I believe there's no need to duplicate the functions -
> move them to common code instead.

Taking the above into account, please try to use get_options() and
then tell me what's not working with it. If so, we will add test cases
to get_options() and fix it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
> On 2022-07-08 12:22 PM, Andy Shevchenko wrote:
> > On Thu, Jul 7, 2022 at 11:03 AM Cezary Rojewski
> > <cezary.rojewski@intel.com> wrote:
> >>
> >> Add strsplit_u32() and its __user variant to allow for splitting
> >> specified string into array of u32 tokens.
> >
> > And I believe we have more of this done in old code.
> > Since all callers use ',' as a delimiter, have you considered using
> > get_options()?
>
>
> Thanks for your input, Andy.
>
> When I'd written the very first version of this function many months
> ago, get_options() looked as it does not fulfill our needs. It seems to
> be true even today: caller needs to know the number of elements in an
> array upfront.

Have you read a kernel doc for it? It does return the number of
elements at the first pass.

> Also, kstrtox() takes into account '0x' and modifies the
> base accordingly if that's the case. simple_strtoull() looks as not
> capable of doing the same thing.

How come?! It does parse all known prefixes: 0x, 0, +, -.

> The goal is to be able to parse input such as:
>
> 0x1000003,0,0,0x1000004,0,0
>
> into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.

Yes. Have you checked the test cases for get_options()?

> >> Originally this functionality was added for the SOF sound driver. As
> >> more users are on the horizon, relocate it so it becomes a common good.
> >
> > Maybe it can be fixed just there.
>
> avs-driver, which is also part of the ASoC framework has very similar
> debug-interface. I believe there's no need to duplicate the functions -
> move them to common code instead.

Taking the above into account, please try to use get_options() and
then tell me what's not working with it. If so, we will add test cases
to get_options() and fix it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 1:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:

...

> Taking the above into account, please try to use get_options() and
> then tell me what's not working with it. If so, we will add test cases
> to get_options() and fix it.

That said, I would probably expect new cases for hexdecimal input
along with using unsigned int * as an acceptor to see that there are
no bugs related to signed vs. unsigned.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 1:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:

...

> Taking the above into account, please try to use get_options() and
> then tell me what's not working with it. If so, we will add test cases
> to get_options() and fix it.

That said, I would probably expect new cases for hexdecimal input
along with using unsigned int * as an acceptor to see that there are
no bugs related to signed vs. unsigned.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 1:46 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:

...

>> When I'd written the very first version of this function many months
>> ago, get_options() looked as it does not fulfill our needs. It seems to
>> be true even today: caller needs to know the number of elements in an
>> array upfront.
> 
> Have you read a kernel doc for it? It does return the number of
> elements at the first pass.

Yes, I've checked several parts of it. Perhaps I did miss something but 
simple_strtoull() doc reads: use kstrtox() instead. Thus the 
strsplit_u32() makes use of kstrtox().

>> Also, kstrtox() takes into account '0x' and modifies the
>> base accordingly if that's the case. simple_strtoull() looks as not
>> capable of doing the same thing.
> 
> How come?! It does parse all known prefixes: 0x, 0, +, -.

Hmm.. doc says that it stops at the first non-digit character. Will 
re-check.

>> The goal is to be able to parse input such as:
>>
>> 0x1000003,0,0,0x1000004,0,0
>>
>> into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.
> 
> Yes. Have you checked the test cases for get_options()?
> 

...

>> avs-driver, which is also part of the ASoC framework has very similar
>> debug-interface. I believe there's no need to duplicate the functions -
>> move them to common code instead.
> 
> Taking the above into account, please try to use get_options() and
> then tell me what's not working with it. If so, we will add test cases
> to get_options() and fix it.

There is a difference:

	// get_options
	int ints[5];

	s = get_options(str, ARRAY_SIZE(ints), ints);

	// strsplit_u32()
	u32 *tkns, num_tkns;

	ret = strsplit_u32(str, delim, &tkns, &num_tkns);

Nothing has been told upfront for in the second case.

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 1:46 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:

...

>> When I'd written the very first version of this function many months
>> ago, get_options() looked as it does not fulfill our needs. It seems to
>> be true even today: caller needs to know the number of elements in an
>> array upfront.
> 
> Have you read a kernel doc for it? It does return the number of
> elements at the first pass.

Yes, I've checked several parts of it. Perhaps I did miss something but 
simple_strtoull() doc reads: use kstrtox() instead. Thus the 
strsplit_u32() makes use of kstrtox().

>> Also, kstrtox() takes into account '0x' and modifies the
>> base accordingly if that's the case. simple_strtoull() looks as not
>> capable of doing the same thing.
> 
> How come?! It does parse all known prefixes: 0x, 0, +, -.

Hmm.. doc says that it stops at the first non-digit character. Will 
re-check.

>> The goal is to be able to parse input such as:
>>
>> 0x1000003,0,0,0x1000004,0,0
>>
>> into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.
> 
> Yes. Have you checked the test cases for get_options()?
> 

...

>> avs-driver, which is also part of the ASoC framework has very similar
>> debug-interface. I believe there's no need to duplicate the functions -
>> move them to common code instead.
> 
> Taking the above into account, please try to use get_options() and
> then tell me what's not working with it. If so, we will add test cases
> to get_options() and fix it.

There is a difference:

	// get_options
	int ints[5];

	s = get_options(str, ARRAY_SIZE(ints), ints);

	// strsplit_u32()
	u32 *tkns, num_tkns;

	ret = strsplit_u32(str, delim, &tkns, &num_tkns);

Nothing has been told upfront for in the second case.

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 08, 2022 at 02:13:14PM +0200, Cezary Rojewski wrote:
> On 2022-07-08 1:46 PM, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
> > <cezary.rojewski@intel.com> wrote:

...

> > > When I'd written the very first version of this function many months
> > > ago, get_options() looked as it does not fulfill our needs. It seems to
> > > be true even today: caller needs to know the number of elements in an
> > > array upfront.
> > 
> > Have you read a kernel doc for it? It does return the number of
> > elements at the first pass.
> 
> Yes, I've checked several parts of it. Perhaps I did miss something but
> simple_strtoull() doc reads: use kstrtox() instead.

Doc was fixed to make clearer that in some cases it's okay to use
simple_strtox(). And this was exactly due to obfuscation code with this
recommendation. Yes, in general one supposed to use kstrtox(), but it's
not 100% obligatory.

> Thus the strsplit_u32()
> makes use of kstrtox().

Yeah...

> > > Also, kstrtox() takes into account '0x' and modifies the
> > > base accordingly if that's the case. simple_strtoull() looks as not
> > > capable of doing the same thing.
> > 
> > How come?! It does parse all known prefixes: 0x, 0, +, -.
> 
> Hmm.. doc says that it stops at the first non-digit character. Will
> re-check.

Yes, but under non-digit implies the standard prefixes of digits.
simple_strtox() and kstrotox() use the very same function for prefixes.

> > > The goal is to be able to parse input such as:
> > > 
> > > 0x1000003,0,0,0x1000004,0,0
> > > 
> > > into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.
> > 
> > Yes. Have you checked the test cases for get_options()?

(1)

...

> > > avs-driver, which is also part of the ASoC framework has very similar
> > > debug-interface. I believe there's no need to duplicate the functions -
> > > move them to common code instead.
> > 
> > Taking the above into account, please try to use get_options() and
> > then tell me what's not working with it. If so, we will add test cases
> > to get_options() and fix it.
> 
> There is a difference:
> 
> 	// get_options
> 	int ints[5];
> 
> 	s = get_options(str, ARRAY_SIZE(ints), ints);
> 
> 	// strsplit_u32()
> 	u32 *tkns, num_tkns;
> 
> 	ret = strsplit_u32(str, delim, &tkns, &num_tkns);
> 
> Nothing has been told upfront for in the second case.

It seems you are missing the (1). The code has checks for the case where you
can do get number upfront, it would just require two passes, but it's nothing
in comparison of heave realloc().

  unsigned int *tokens;
  char *p;
  int num;

  p = get_options(str, 0, &num);
  if (num == 0)
	// No numbers in the string!

  tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
  if (!tokens)
	return -ENOMEM;

  p = get_oprions(str, num, &tokens);
  if (*p)
	// String was parsed only partially!
	// assuming it's not a fatal error

  return tokens;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 08, 2022 at 02:13:14PM +0200, Cezary Rojewski wrote:
> On 2022-07-08 1:46 PM, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
> > <cezary.rojewski@intel.com> wrote:

...

> > > When I'd written the very first version of this function many months
> > > ago, get_options() looked as it does not fulfill our needs. It seems to
> > > be true even today: caller needs to know the number of elements in an
> > > array upfront.
> > 
> > Have you read a kernel doc for it? It does return the number of
> > elements at the first pass.
> 
> Yes, I've checked several parts of it. Perhaps I did miss something but
> simple_strtoull() doc reads: use kstrtox() instead.

Doc was fixed to make clearer that in some cases it's okay to use
simple_strtox(). And this was exactly due to obfuscation code with this
recommendation. Yes, in general one supposed to use kstrtox(), but it's
not 100% obligatory.

> Thus the strsplit_u32()
> makes use of kstrtox().

Yeah...

> > > Also, kstrtox() takes into account '0x' and modifies the
> > > base accordingly if that's the case. simple_strtoull() looks as not
> > > capable of doing the same thing.
> > 
> > How come?! It does parse all known prefixes: 0x, 0, +, -.
> 
> Hmm.. doc says that it stops at the first non-digit character. Will
> re-check.

Yes, but under non-digit implies the standard prefixes of digits.
simple_strtox() and kstrotox() use the very same function for prefixes.

> > > The goal is to be able to parse input such as:
> > > 
> > > 0x1000003,0,0,0x1000004,0,0
> > > 
> > > into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.
> > 
> > Yes. Have you checked the test cases for get_options()?

(1)

...

> > > avs-driver, which is also part of the ASoC framework has very similar
> > > debug-interface. I believe there's no need to duplicate the functions -
> > > move them to common code instead.
> > 
> > Taking the above into account, please try to use get_options() and
> > then tell me what's not working with it. If so, we will add test cases
> > to get_options() and fix it.
> 
> There is a difference:
> 
> 	// get_options
> 	int ints[5];
> 
> 	s = get_options(str, ARRAY_SIZE(ints), ints);
> 
> 	// strsplit_u32()
> 	u32 *tkns, num_tkns;
> 
> 	ret = strsplit_u32(str, delim, &tkns, &num_tkns);
> 
> Nothing has been told upfront for in the second case.

It seems you are missing the (1). The code has checks for the case where you
can do get number upfront, it would just require two passes, but it's nothing
in comparison of heave realloc().

  unsigned int *tokens;
  char *p;
  int num;

  p = get_options(str, 0, &num);
  if (num == 0)
	// No numbers in the string!

  tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
  if (!tokens)
	return -ENOMEM;

  p = get_oprions(str, num, &tokens);
  if (*p)
	// String was parsed only partially!
	// assuming it's not a fatal error

  return tokens;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Péter Ujfalusi]

On 08/07/2022 15:30, Andy Shevchenko wrote:
> On Fri, Jul 08, 2022 at 02:13:14PM +0200, Cezary Rojewski wrote:
>> On 2022-07-08 1:46 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
>>> <cezary.rojewski@intel.com> wrote:
> 
> ...
> 
>>>> When I'd written the very first version of this function many months
>>>> ago, get_options() looked as it does not fulfill our needs. It seems to
>>>> be true even today: caller needs to know the number of elements in an
>>>> array upfront.
>>>
>>> Have you read a kernel doc for it? It does return the number of
>>> elements at the first pass.
>>
>> Yes, I've checked several parts of it. Perhaps I did miss something but
>> simple_strtoull() doc reads: use kstrtox() instead.
> 
> Doc was fixed to make clearer that in some cases it's okay to use
> simple_strtox(). And this was exactly due to obfuscation code with this
> recommendation. Yes, in general one supposed to use kstrtox(), but it's
> not 100% obligatory.
> 
>> Thus the strsplit_u32()
>> makes use of kstrtox().
> 
> Yeah...
> 
>>>> Also, kstrtox() takes into account '0x' and modifies the
>>>> base accordingly if that's the case. simple_strtoull() looks as not
>>>> capable of doing the same thing.
>>>
>>> How come?! It does parse all known prefixes: 0x, 0, +, -.
>>
>> Hmm.. doc says that it stops at the first non-digit character. Will
>> re-check.
> 
> Yes, but under non-digit implies the standard prefixes of digits.
> simple_strtox() and kstrotox() use the very same function for prefixes.
> 
>>>> The goal is to be able to parse input such as:
>>>>
>>>> 0x1000003,0,0,0x1000004,0,0
>>>>
>>>> into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.
>>>
>>> Yes. Have you checked the test cases for get_options()?
> 
> (1)
> 
> ...
> 
>>>> avs-driver, which is also part of the ASoC framework has very similar
>>>> debug-interface. I believe there's no need to duplicate the functions -
>>>> move them to common code instead.
>>>
>>> Taking the above into account, please try to use get_options() and
>>> then tell me what's not working with it. If so, we will add test cases
>>> to get_options() and fix it.
>>
>> There is a difference:
>>
>> 	// get_options
>> 	int ints[5];
>>
>> 	s = get_options(str, ARRAY_SIZE(ints), ints);
>>
>> 	// strsplit_u32()
>> 	u32 *tkns, num_tkns;
>>
>> 	ret = strsplit_u32(str, delim, &tkns, &num_tkns);
>>
>> Nothing has been told upfront for in the second case.
> 
> It seems you are missing the (1). The code has checks for the case where you
> can do get number upfront, it would just require two passes, but it's nothing
> in comparison of heave realloc().
> 
>   unsigned int *tokens;
>   char *p;
>   int num;
> 
>   p = get_options(str, 0, &num);
>   if (num == 0)
> 	// No numbers in the string!
> 
>   tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
>   if (!tokens)
> 	return -ENOMEM;
> 
>   p = get_oprions(str, num, &tokens);
>   if (*p)
> 	// String was parsed only partially!
> 	// assuming it's not a fatal error
> 
>   return tokens;
> 

This diff is tested and works:
diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 60e4250fac87..48d405f78a83 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -410,61 +410,11 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
 	.copy = sof_probes_compr_copy,
 };
 
-/**
- * strsplit_u32 - Split string into sequence of u32 tokens
- * @buf:	String to split into tokens.
- * @delim:	String containing delimiter characters.
- * @tkns:	Returned u32 sequence pointer.
- * @num_tkns:	Returned number of tokens obtained.
- */
-static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tkns)
-{
-	char *s;
-	u32 *data, *tmp;
-	size_t count = 0;
-	size_t cap = 32;
-	int ret = 0;
-
-	*tkns = NULL;
-	*num_tkns = 0;
-	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	while ((s = strsep(&buf, delim)) != NULL) {
-		ret = kstrtouint(s, 0, data + count);
-		if (ret)
-			goto exit;
-		if (++count >= cap) {
-			cap *= 2;
-			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
-			if (!tmp) {
-				ret = -ENOMEM;
-				goto exit;
-			}
-			data = tmp;
-		}
-	}
-
-	if (!count)
-		goto exit;
-	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
-	if (!(*tkns)) {
-		ret = -ENOMEM;
-		goto exit;
-	}
-	*num_tkns = count;
-
-exit:
-	kfree(data);
-	return ret;
-}
-
 static int tokenize_input(const char __user *from, size_t count,
 			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
 {
+	int ret, nints, i, *ints;
 	char *buf;
-	int ret;
 
 	buf = kmalloc(count + 1, GFP_KERNEL);
 	if (!buf)
@@ -473,12 +423,36 @@ static int tokenize_input(const char __user *from, size_t count,
 	ret = simple_write_to_buffer(buf, count, ppos, from, count);
 	if (ret != count) {
 		ret = ret >= 0 ? -EIO : ret;
-		goto exit;
+		goto free_buf;
 	}
 
 	buf[count] = '\0';
-	ret = strsplit_u32(buf, ",", tkns, num_tkns);
-exit:
+	get_options(buf, 0, &nints);
+	if (!nints) {
+		ret = -ENOENT;
+		goto free_buf;
+	}
+
+	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
+	if (!ints) {
+		ret = -ENOMEM;
+		goto free_buf;
+	}
+
+	*tkns = kcalloc(nints, sizeof(u32), GFP_KERNEL);
+	if (!(*tkns)) {
+		ret = -ENOMEM;
+		goto free_ints;
+	}
+
+	get_options(buf, nints + 1, ints);
+	*num_tkns = nints;
+	for (i = 0; i < nints; i++)
+		(*tkns)[i] = ints[i + 1];
+	
+free_ints:
+	kfree(ints);
+free_buf:
 	kfree(buf);
 	return ret;
 }

Could be made nicer with some brain work put to it, we need strict u32 within the IPC message for the array...

-- 
Péter

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Péter Ujfalusi]

On 08/07/2022 15:30, Andy Shevchenko wrote:
> On Fri, Jul 08, 2022 at 02:13:14PM +0200, Cezary Rojewski wrote:
>> On 2022-07-08 1:46 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 8, 2022 at 1:33 PM Cezary Rojewski
>>> <cezary.rojewski@intel.com> wrote:
> 
> ...
> 
>>>> When I'd written the very first version of this function many months
>>>> ago, get_options() looked as it does not fulfill our needs. It seems to
>>>> be true even today: caller needs to know the number of elements in an
>>>> array upfront.
>>>
>>> Have you read a kernel doc for it? It does return the number of
>>> elements at the first pass.
>>
>> Yes, I've checked several parts of it. Perhaps I did miss something but
>> simple_strtoull() doc reads: use kstrtox() instead.
> 
> Doc was fixed to make clearer that in some cases it's okay to use
> simple_strtox(). And this was exactly due to obfuscation code with this
> recommendation. Yes, in general one supposed to use kstrtox(), but it's
> not 100% obligatory.
> 
>> Thus the strsplit_u32()
>> makes use of kstrtox().
> 
> Yeah...
> 
>>>> Also, kstrtox() takes into account '0x' and modifies the
>>>> base accordingly if that's the case. simple_strtoull() looks as not
>>>> capable of doing the same thing.
>>>
>>> How come?! It does parse all known prefixes: 0x, 0, +, -.
>>
>> Hmm.. doc says that it stops at the first non-digit character. Will
>> re-check.
> 
> Yes, but under non-digit implies the standard prefixes of digits.
> simple_strtox() and kstrotox() use the very same function for prefixes.
> 
>>>> The goal is to be able to parse input such as:
>>>>
>>>> 0x1000003,0,0,0x1000004,0,0
>>>>
>>>> into a sequence of 6 uints, filling the *tkns and *num_tkns for the caller.
>>>
>>> Yes. Have you checked the test cases for get_options()?
> 
> (1)
> 
> ...
> 
>>>> avs-driver, which is also part of the ASoC framework has very similar
>>>> debug-interface. I believe there's no need to duplicate the functions -
>>>> move them to common code instead.
>>>
>>> Taking the above into account, please try to use get_options() and
>>> then tell me what's not working with it. If so, we will add test cases
>>> to get_options() and fix it.
>>
>> There is a difference:
>>
>> 	// get_options
>> 	int ints[5];
>>
>> 	s = get_options(str, ARRAY_SIZE(ints), ints);
>>
>> 	// strsplit_u32()
>> 	u32 *tkns, num_tkns;
>>
>> 	ret = strsplit_u32(str, delim, &tkns, &num_tkns);
>>
>> Nothing has been told upfront for in the second case.
> 
> It seems you are missing the (1). The code has checks for the case where you
> can do get number upfront, it would just require two passes, but it's nothing
> in comparison of heave realloc().
> 
>   unsigned int *tokens;
>   char *p;
>   int num;
> 
>   p = get_options(str, 0, &num);
>   if (num == 0)
> 	// No numbers in the string!
> 
>   tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
>   if (!tokens)
> 	return -ENOMEM;
> 
>   p = get_oprions(str, num, &tokens);
>   if (*p)
> 	// String was parsed only partially!
> 	// assuming it's not a fatal error
> 
>   return tokens;
> 

This diff is tested and works:
diff --git a/sound/soc/sof/sof-client-probes.c b/sound/soc/sof/sof-client-probes.c
index 60e4250fac87..48d405f78a83 100644
--- a/sound/soc/sof/sof-client-probes.c
+++ b/sound/soc/sof/sof-client-probes.c
@@ -410,61 +410,11 @@ static const struct snd_compress_ops sof_probes_compressed_ops = {
 	.copy = sof_probes_compr_copy,
 };
 
-/**
- * strsplit_u32 - Split string into sequence of u32 tokens
- * @buf:	String to split into tokens.
- * @delim:	String containing delimiter characters.
- * @tkns:	Returned u32 sequence pointer.
- * @num_tkns:	Returned number of tokens obtained.
- */
-static int strsplit_u32(char *buf, const char *delim, u32 **tkns, size_t *num_tkns)
-{
-	char *s;
-	u32 *data, *tmp;
-	size_t count = 0;
-	size_t cap = 32;
-	int ret = 0;
-
-	*tkns = NULL;
-	*num_tkns = 0;
-	data = kcalloc(cap, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	while ((s = strsep(&buf, delim)) != NULL) {
-		ret = kstrtouint(s, 0, data + count);
-		if (ret)
-			goto exit;
-		if (++count >= cap) {
-			cap *= 2;
-			tmp = krealloc(data, cap * sizeof(*data), GFP_KERNEL);
-			if (!tmp) {
-				ret = -ENOMEM;
-				goto exit;
-			}
-			data = tmp;
-		}
-	}
-
-	if (!count)
-		goto exit;
-	*tkns = kmemdup(data, count * sizeof(*data), GFP_KERNEL);
-	if (!(*tkns)) {
-		ret = -ENOMEM;
-		goto exit;
-	}
-	*num_tkns = count;
-
-exit:
-	kfree(data);
-	return ret;
-}
-
 static int tokenize_input(const char __user *from, size_t count,
 			  loff_t *ppos, u32 **tkns, size_t *num_tkns)
 {
+	int ret, nints, i, *ints;
 	char *buf;
-	int ret;
 
 	buf = kmalloc(count + 1, GFP_KERNEL);
 	if (!buf)
@@ -473,12 +423,36 @@ static int tokenize_input(const char __user *from, size_t count,
 	ret = simple_write_to_buffer(buf, count, ppos, from, count);
 	if (ret != count) {
 		ret = ret >= 0 ? -EIO : ret;
-		goto exit;
+		goto free_buf;
 	}
 
 	buf[count] = '\0';
-	ret = strsplit_u32(buf, ",", tkns, num_tkns);
-exit:
+	get_options(buf, 0, &nints);
+	if (!nints) {
+		ret = -ENOENT;
+		goto free_buf;
+	}
+
+	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
+	if (!ints) {
+		ret = -ENOMEM;
+		goto free_buf;
+	}
+
+	*tkns = kcalloc(nints, sizeof(u32), GFP_KERNEL);
+	if (!(*tkns)) {
+		ret = -ENOMEM;
+		goto free_ints;
+	}
+
+	get_options(buf, nints + 1, ints);
+	*num_tkns = nints;
+	for (i = 0; i < nints; i++)
+		(*tkns)[i] = ints[i + 1];
+	
+free_ints:
+	kfree(ints);
+free_buf:
 	kfree(buf);
 	return ret;
 }

Could be made nicer with some brain work put to it, we need strict u32 within the IPC message for the array...

-- 
Péter

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
<peter.ujfalusi@linux.intel.com> wrote:
> On 08/07/2022 15:30, Andy Shevchenko wrote:
> > On Fri, Jul 08, 2022 at 02:13:14PM +0200, Cezary Rojewski wrote:

...

> > It seems you are missing the (1). The code has checks for the case where you
> > can do get number upfront, it would just require two passes, but it's nothing
> > in comparison of heave realloc().
> >
> >   unsigned int *tokens;
> >   char *p;
> >   int num;
> >
> >   p = get_options(str, 0, &num);
> >   if (num == 0)
> >       // No numbers in the string!
> >
> >   tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
> >   if (!tokens)
> >       return -ENOMEM;
> >
> >   p = get_oprions(str, num, &tokens);
> >   if (*p)
> >       // String was parsed only partially!
> >       // assuming it's not a fatal error
> >
> >   return tokens;

> This diff is tested and works:

Thanks, Peter!

But at least you can memove() to avoid second allocation.
ideally to refactor that the result of get_options is consumed as is
(it may be casted to struct tokens { int n; u32 v[]; })

...

> Could be made nicer with some brain work put to it, we need strict u32 within the IPC message for the array...

True, it needs to be thought through. But I guess you got the idea of
how to use existing library routines.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
<peter.ujfalusi@linux.intel.com> wrote:
> On 08/07/2022 15:30, Andy Shevchenko wrote:
> > On Fri, Jul 08, 2022 at 02:13:14PM +0200, Cezary Rojewski wrote:

...

> > It seems you are missing the (1). The code has checks for the case where you
> > can do get number upfront, it would just require two passes, but it's nothing
> > in comparison of heave realloc().
> >
> >   unsigned int *tokens;
> >   char *p;
> >   int num;
> >
> >   p = get_options(str, 0, &num);
> >   if (num == 0)
> >       // No numbers in the string!
> >
> >   tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
> >   if (!tokens)
> >       return -ENOMEM;
> >
> >   p = get_oprions(str, num, &tokens);
> >   if (*p)
> >       // String was parsed only partially!
> >       // assuming it's not a fatal error
> >
> >   return tokens;

> This diff is tested and works:

Thanks, Peter!

But at least you can memove() to avoid second allocation.
ideally to refactor that the result of get_options is consumed as is
(it may be casted to struct tokens { int n; u32 v[]; })

...

> Could be made nicer with some brain work put to it, we need strict u32 within the IPC message for the array...

True, it needs to be thought through. But I guess you got the idea of
how to use existing library routines.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 5:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> <peter.ujfalusi@linux.intel.com> wrote:

...

> (it may be casted to struct tokens { int n; u32 v[]; })

On second thought it is better not to do this (it will work on x86,
but in general it may be surprising on BE-64).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 5:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> <peter.ujfalusi@linux.intel.com> wrote:

...

> (it may be casted to struct tokens { int n; u32 v[]; })

On second thought it is better not to do this (it will work on x86,
but in general it may be surprising on BE-64).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> <peter.ujfalusi@linux.intel.com> wrote:

...

>>> It seems you are missing the (1). The code has checks for the case where you
>>> can do get number upfront, it would just require two passes, but it's nothing
>>> in comparison of heave realloc().
>>>
>>>    unsigned int *tokens;
>>>    char *p;
>>>    int num;
>>>
>>>    p = get_options(str, 0, &num);
>>>    if (num == 0)
>>>        // No numbers in the string!
>>>
>>>    tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
>>>    if (!tokens)
>>>        return -ENOMEM;
>>>
>>>    p = get_oprions(str, num, &tokens);
>>>    if (*p)
>>>        // String was parsed only partially!
>>>        // assuming it's not a fatal error
>>>
>>>    return tokens;
> 
>> This diff is tested and works:
> 
> Thanks, Peter!
> 
> But at least you can memove() to avoid second allocation.
> ideally to refactor that the result of get_options is consumed as is
> (it may be casted to struct tokens { int n; u32 v[]; })


A long shot, but what if we were to modify get_options() so it takes 
additional element-size parameter instead? Something like below - I've 
ignored get_range() though. Will re-visit if this option is viable.


diff --git a/lib/cmdline.c b/lib/cmdline.c
index 5546bf588780..272f892b71df 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -53,7 +53,7 @@ static int get_range(char **str, int *pint, int n)
   *     for the sake of simplification.
   */

-int get_option(char **str, int *pint)
+int get_num_option(char **str, void *pint, size_t nsize)
  {
         char *cur = *str;
         int value;
@@ -65,7 +65,7 @@ int get_option(char **str, int *pint)
         else
                 value = simple_strtoull(cur, str, 0);
         if (pint)
-               *pint = value;
+               memcpy(pint, &value, min(nsize, sizeof(value)));
         if (cur == *str)
                 return 0;
         if (**str == ',') {
@@ -77,6 +77,12 @@ int get_option(char **str, int *pint)

         return 1;
  }
+EXPORT_SYMBOL(get_num_option);
+
+int get_option(char **str, int *pint)
+{
+       return get_num_option(str, pint, sizeof(*pint));
+}
  EXPORT_SYMBOL(get_option);

  /**
@@ -104,15 +110,15 @@ EXPORT_SYMBOL(get_option);
   *     completely parseable).
   */

-char *get_options(const char *str, int nints, int *ints)
+char *get_num_options(const char *str, int nints, void *ints, size_t nsize)
  {
         bool validate = (nints == 0);
         int res, i = 1;

         while (i < nints || validate) {
-               int *pint = validate ? ints : ints + i;
+               int *pint = validate ? ints : ints + (i * nsize);

-               res = get_option((char **)&str, pint);
+               res = get_num_option((char **)&str, pint, nsize);
                 if (res == 0)
                         break;
                 if (res == 3) {
@@ -133,9 +139,17 @@ char *get_options(const char *str, int nints, int 
*ints)
                 if (res == 1)
                         break;
         }
-       ints[0] = i - 1;
+       --i;
+       memcpy(ints, &i, min(nsize, sizeof(i)));
         return (char *)str;
  }
+EXPORT_SYMBOL(get_num_options);
+
+char *get_options(const char *str, int nints, int *ints)
+{
+       return get_num_options(str, nints, ints, sizeof(*ints));
+}
+
  EXPORT_SYMBOL(get_options);

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> <peter.ujfalusi@linux.intel.com> wrote:

...

>>> It seems you are missing the (1). The code has checks for the case where you
>>> can do get number upfront, it would just require two passes, but it's nothing
>>> in comparison of heave realloc().
>>>
>>>    unsigned int *tokens;
>>>    char *p;
>>>    int num;
>>>
>>>    p = get_options(str, 0, &num);
>>>    if (num == 0)
>>>        // No numbers in the string!
>>>
>>>    tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
>>>    if (!tokens)
>>>        return -ENOMEM;
>>>
>>>    p = get_oprions(str, num, &tokens);
>>>    if (*p)
>>>        // String was parsed only partially!
>>>        // assuming it's not a fatal error
>>>
>>>    return tokens;
> 
>> This diff is tested and works:
> 
> Thanks, Peter!
> 
> But at least you can memove() to avoid second allocation.
> ideally to refactor that the result of get_options is consumed as is
> (it may be casted to struct tokens { int n; u32 v[]; })


A long shot, but what if we were to modify get_options() so it takes 
additional element-size parameter instead? Something like below - I've 
ignored get_range() though. Will re-visit if this option is viable.


diff --git a/lib/cmdline.c b/lib/cmdline.c
index 5546bf588780..272f892b71df 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -53,7 +53,7 @@ static int get_range(char **str, int *pint, int n)
   *     for the sake of simplification.
   */

-int get_option(char **str, int *pint)
+int get_num_option(char **str, void *pint, size_t nsize)
  {
         char *cur = *str;
         int value;
@@ -65,7 +65,7 @@ int get_option(char **str, int *pint)
         else
                 value = simple_strtoull(cur, str, 0);
         if (pint)
-               *pint = value;
+               memcpy(pint, &value, min(nsize, sizeof(value)));
         if (cur == *str)
                 return 0;
         if (**str == ',') {
@@ -77,6 +77,12 @@ int get_option(char **str, int *pint)

         return 1;
  }
+EXPORT_SYMBOL(get_num_option);
+
+int get_option(char **str, int *pint)
+{
+       return get_num_option(str, pint, sizeof(*pint));
+}
  EXPORT_SYMBOL(get_option);

  /**
@@ -104,15 +110,15 @@ EXPORT_SYMBOL(get_option);
   *     completely parseable).
   */

-char *get_options(const char *str, int nints, int *ints)
+char *get_num_options(const char *str, int nints, void *ints, size_t nsize)
  {
         bool validate = (nints == 0);
         int res, i = 1;

         while (i < nints || validate) {
-               int *pint = validate ? ints : ints + i;
+               int *pint = validate ? ints : ints + (i * nsize);

-               res = get_option((char **)&str, pint);
+               res = get_num_option((char **)&str, pint, nsize);
                 if (res == 0)
                         break;
                 if (res == 3) {
@@ -133,9 +139,17 @@ char *get_options(const char *str, int nints, int 
*ints)
                 if (res == 1)
                         break;
         }
-       ints[0] = i - 1;
+       --i;
+       memcpy(ints, &i, min(nsize, sizeof(i)));
         return (char *)str;
  }
+EXPORT_SYMBOL(get_num_options);
+
+char *get_options(const char *str, int nints, int *ints)
+{
+       return get_num_options(str, nints, ints, sizeof(*ints));
+}
+
  EXPORT_SYMBOL(get_options);

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> > <peter.ujfalusi@linux.intel.com> wrote:

> A long shot, but what if we were to modify get_options() so it takes
> additional element-size parameter instead?

But why? int / unsigned int, u32 / s32  are all compatible in the current cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
>
> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> > <peter.ujfalusi@linux.intel.com> wrote:

> A long shot, but what if we were to modify get_options() so it takes
> additional element-size parameter instead?

But why? int / unsigned int, u32 / s32  are all compatible in the current cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
>>
>> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
>>> <peter.ujfalusi@linux.intel.com> wrote:
> 
>> A long shot, but what if we were to modify get_options() so it takes
>> additional element-size parameter instead?
> 
> But why? int / unsigned int, u32 / s32  are all compatible in the current cases.

I'd like to avoid any additional operations, so that the retrieved 
payload can be provided to the IPC handler directly. The IPC handlers 
for AudioDSP drivers are expecting payload in u32s.

// u32 **tkns, size_t *num_tkns as foo() arguments
// u32 *ints, int nints as locals

	get_options(buf, 0, &nints);
	if (!nints) {
		ret = -ENOENT;
		goto free_buf;
	}

	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
	if (!ints) {
		ret = -ENOMEM;
		goto free_buf;
	}

	get_num_options(buf, nints + 1, ints, sizeof(*ints));

	*tkns = ints;
	*num_tkns = nints;

No additional operations in between. The intermediate IPC handler can 
later refer to the actual payload via &tkns[1] before passing it to the 
generic one.

Casting int array into u32 array does not feel right, or perhaps I'm 
missing something like in the doc case.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:
>>
>> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
>>> <peter.ujfalusi@linux.intel.com> wrote:
> 
>> A long shot, but what if we were to modify get_options() so it takes
>> additional element-size parameter instead?
> 
> But why? int / unsigned int, u32 / s32  are all compatible in the current cases.

I'd like to avoid any additional operations, so that the retrieved 
payload can be provided to the IPC handler directly. The IPC handlers 
for AudioDSP drivers are expecting payload in u32s.

// u32 **tkns, size_t *num_tkns as foo() arguments
// u32 *ints, int nints as locals

	get_options(buf, 0, &nints);
	if (!nints) {
		ret = -ENOENT;
		goto free_buf;
	}

	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
	if (!ints) {
		ret = -ENOMEM;
		goto free_buf;
	}

	get_num_options(buf, nints + 1, ints, sizeof(*ints));

	*tkns = ints;
	*num_tkns = nints;

No additional operations in between. The intermediate IPC handler can 
later refer to the actual payload via &tkns[1] before passing it to the 
generic one.

Casting int array into u32 array does not feel right, or perhaps I'm 
missing something like in the doc case.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Sat, Jul 09, 2022 at 10:45:49AM +0200, Cezary Rojewski wrote:
> On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
> > <cezary.rojewski@intel.com> wrote:
> > > 
> > > On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> > > > On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> > > > <peter.ujfalusi@linux.intel.com> wrote:
> > 
> > > A long shot, but what if we were to modify get_options() so it takes
> > > additional element-size parameter instead?
> > 
> > But why? int / unsigned int, u32 / s32  are all compatible in the current cases.
> 
> I'd like to avoid any additional operations, so that the retrieved payload
> can be provided to the IPC handler directly. The IPC handlers for AudioDSP
> drivers are expecting payload in u32s.
> 
> // u32 **tkns, size_t *num_tkns as foo() arguments
> // u32 *ints, int nints as locals
> 
> 	get_options(buf, 0, &nints);
> 	if (!nints) {
> 		ret = -ENOENT;
> 		goto free_buf;
> 	}
> 
> 	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
> 	if (!ints) {
> 		ret = -ENOMEM;
> 		goto free_buf;
> 	}
> 
> 	get_num_options(buf, nints + 1, ints, sizeof(*ints));
> 
> 	*tkns = ints;
> 	*num_tkns = nints;
> 
> No additional operations in between. The intermediate IPC handler can later
> refer to the actual payload via &tkns[1] before passing it to the generic
> one.
> 
> Casting int array into u32 array does not feel right, or perhaps I'm missing
> something like in the doc case.

C standard.

int to unsigned int is not promoted. And standard says that "The rank of any
unsigned integer type shall equal the rank of the corresponding signed integer
type, if any."

I don't know why one needs to have an additional churn here. int and unsigned
int are interoperable with the adjustment to the sign when the other argument
is signed or lesser rank of.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Sat, Jul 09, 2022 at 10:45:49AM +0200, Cezary Rojewski wrote:
> On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
> > <cezary.rojewski@intel.com> wrote:
> > > 
> > > On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> > > > On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> > > > <peter.ujfalusi@linux.intel.com> wrote:
> > 
> > > A long shot, but what if we were to modify get_options() so it takes
> > > additional element-size parameter instead?
> > 
> > But why? int / unsigned int, u32 / s32  are all compatible in the current cases.
> 
> I'd like to avoid any additional operations, so that the retrieved payload
> can be provided to the IPC handler directly. The IPC handlers for AudioDSP
> drivers are expecting payload in u32s.
> 
> // u32 **tkns, size_t *num_tkns as foo() arguments
> // u32 *ints, int nints as locals
> 
> 	get_options(buf, 0, &nints);
> 	if (!nints) {
> 		ret = -ENOENT;
> 		goto free_buf;
> 	}
> 
> 	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
> 	if (!ints) {
> 		ret = -ENOMEM;
> 		goto free_buf;
> 	}
> 
> 	get_num_options(buf, nints + 1, ints, sizeof(*ints));
> 
> 	*tkns = ints;
> 	*num_tkns = nints;
> 
> No additional operations in between. The intermediate IPC handler can later
> refer to the actual payload via &tkns[1] before passing it to the generic
> one.
> 
> Casting int array into u32 array does not feel right, or perhaps I'm missing
> something like in the doc case.

C standard.

int to unsigned int is not promoted. And standard says that "The rank of any
unsigned integer type shall equal the rank of the corresponding signed integer
type, if any."

I don't know why one needs to have an additional churn here. int and unsigned
int are interoperable with the adjustment to the sign when the other argument
is signed or lesser rank of.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> On Sat, Jul 09, 2022 at 10:45:49AM +0200, Cezary Rojewski wrote:
>> On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
>>> <cezary.rojewski@intel.com> wrote:
>>>>
>>>> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
>>>>> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
>>>>> <peter.ujfalusi@linux.intel.com> wrote:
>>>
>>>> A long shot, but what if we were to modify get_options() so it takes
>>>> additional element-size parameter instead?
>>>
>>> But why? int / unsigned int, u32 / s32  are all compatible in the current cases.
>>
>> I'd like to avoid any additional operations, so that the retrieved payload
>> can be provided to the IPC handler directly. The IPC handlers for AudioDSP
>> drivers are expecting payload in u32s.
>>
>> // u32 **tkns, size_t *num_tkns as foo() arguments
>> // u32 *ints, int nints as locals
>>
>> 	get_options(buf, 0, &nints);
>> 	if (!nints) {
>> 		ret = -ENOENT;
>> 		goto free_buf;
>> 	}
>>
>> 	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
>> 	if (!ints) {
>> 		ret = -ENOMEM;
>> 		goto free_buf;
>> 	}
>>
>> 	get_num_options(buf, nints + 1, ints, sizeof(*ints));
>>
>> 	*tkns = ints;
>> 	*num_tkns = nints;
>>
>> No additional operations in between. The intermediate IPC handler can later
>> refer to the actual payload via &tkns[1] before passing it to the generic
>> one.
>>
>> Casting int array into u32 array does not feel right, or perhaps I'm missing
>> something like in the doc case.
> 
> C standard.
> 
> int to unsigned int is not promoted. And standard says that "The rank of any
> unsigned integer type shall equal the rank of the corresponding signed integer
> type, if any."
> 
> I don't know why one needs to have an additional churn here. int and unsigned
> int are interoperable with the adjustment to the sign when the other argument
> is signed or lesser rank of.


I still believe that casting blindly is not the way to go. I did 
explicitly ask about int vs u32, not int vs unsigned int. Please note 
that these values are later passed to the IPC handlers, and this changes 
the context a bit. If hw expects u32, then u32 it shall be.

Please correct me if I'm wrong, but there is no guarantee that int is 
always 32bits long. What is guaranteed though, is that int holds at 
least -/+ 32,767. Also, values larger than INT_MAX are allowed in the 
IPC payload.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> On Sat, Jul 09, 2022 at 10:45:49AM +0200, Cezary Rojewski wrote:
>> On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
>>> On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
>>> <cezary.rojewski@intel.com> wrote:
>>>>
>>>> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
>>>>> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
>>>>> <peter.ujfalusi@linux.intel.com> wrote:
>>>
>>>> A long shot, but what if we were to modify get_options() so it takes
>>>> additional element-size parameter instead?
>>>
>>> But why? int / unsigned int, u32 / s32  are all compatible in the current cases.
>>
>> I'd like to avoid any additional operations, so that the retrieved payload
>> can be provided to the IPC handler directly. The IPC handlers for AudioDSP
>> drivers are expecting payload in u32s.
>>
>> // u32 **tkns, size_t *num_tkns as foo() arguments
>> // u32 *ints, int nints as locals
>>
>> 	get_options(buf, 0, &nints);
>> 	if (!nints) {
>> 		ret = -ENOENT;
>> 		goto free_buf;
>> 	}
>>
>> 	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
>> 	if (!ints) {
>> 		ret = -ENOMEM;
>> 		goto free_buf;
>> 	}
>>
>> 	get_num_options(buf, nints + 1, ints, sizeof(*ints));
>>
>> 	*tkns = ints;
>> 	*num_tkns = nints;
>>
>> No additional operations in between. The intermediate IPC handler can later
>> refer to the actual payload via &tkns[1] before passing it to the generic
>> one.
>>
>> Casting int array into u32 array does not feel right, or perhaps I'm missing
>> something like in the doc case.
> 
> C standard.
> 
> int to unsigned int is not promoted. And standard says that "The rank of any
> unsigned integer type shall equal the rank of the corresponding signed integer
> type, if any."
> 
> I don't know why one needs to have an additional churn here. int and unsigned
> int are interoperable with the adjustment to the sign when the other argument
> is signed or lesser rank of.


I still believe that casting blindly is not the way to go. I did 
explicitly ask about int vs u32, not int vs unsigned int. Please note 
that these values are later passed to the IPC handlers, and this changes 
the context a bit. If hw expects u32, then u32 it shall be.

Please correct me if I'm wrong, but there is no guarantee that int is 
always 32bits long. What is guaranteed though, is that int holds at 
least -/+ 32,767. Also, values larger than INT_MAX are allowed in the 
IPC payload.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 3:51 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> > On Sat, Jul 09, 2022 at 10:45:49AM +0200, Cezary Rojewski wrote:
> >> On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
> >>> On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
> >>> <cezary.rojewski@intel.com> wrote:
> >>>> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> >>>>> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> >>>>> <peter.ujfalusi@linux.intel.com> wrote:

...

> >>>> A long shot, but what if we were to modify get_options() so it takes
> >>>> additional element-size parameter instead?
> >>>
> >>> But why? int / unsigned int, u32 / s32  are all compatible in the current cases.
> >>
> >> I'd like to avoid any additional operations, so that the retrieved payload
> >> can be provided to the IPC handler directly. The IPC handlers for AudioDSP
> >> drivers are expecting payload in u32s.
> >>
> >> // u32 **tkns, size_t *num_tkns as foo() arguments
> >> // u32 *ints, int nints as locals
> >>
> >>      get_options(buf, 0, &nints);
> >>      if (!nints) {
> >>              ret = -ENOENT;
> >>              goto free_buf;
> >>      }
> >>
> >>      ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
> >>      if (!ints) {
> >>              ret = -ENOMEM;
> >>              goto free_buf;
> >>      }
> >>
> >>      get_num_options(buf, nints + 1, ints, sizeof(*ints));
> >>
> >>      *tkns = ints;
> >>      *num_tkns = nints;
> >>
> >> No additional operations in between. The intermediate IPC handler can later
> >> refer to the actual payload via &tkns[1] before passing it to the generic
> >> one.
> >>
> >> Casting int array into u32 array does not feel right, or perhaps I'm missing
> >> something like in the doc case.
> >
> > C standard.
> >
> > int to unsigned int is not promoted. And standard says that "The rank of any
> > unsigned integer type shall equal the rank of the corresponding signed integer
> > type, if any."
> >
> > I don't know why one needs to have an additional churn here. int and unsigned
> > int are interoperable with the adjustment to the sign when the other argument
> > is signed or lesser rank of.
>
> I still believe that casting blindly is not the way to go. I did
> explicitly ask about int vs u32,

There is no such type in the C standard.

> not int vs unsigned int. Please note
> that these values are later passed to the IPC handlers, and this changes
> the context a bit. If hw expects u32, then u32 it shall be.

H/W doesn't expect u32, HW expects bytes or group of bytes with:
1) dedicated address alignment (if required);
2) dedicated byte order;
3) dedicated padding (if required).

Correct me if I'm wrong.

> Please correct me if I'm wrong, but there is no guarantee that int is
> always 32bits long.

There is no guarantee by the C standard, indeed, but there is an upper
level guarantee, by the Linux kernel.

> What is guaranteed though, is that int holds at
> least -/+ 32,767. Also, values larger than INT_MAX are allowed in the
> IPC payload.

Yeah... this is binary protocol, right? So, what limits are you
talking about here if they are not applicable there anyway. It's
simply different dimension of limits (i.e. bytes and bits and not C
language types).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 3:51 PM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> > On Sat, Jul 09, 2022 at 10:45:49AM +0200, Cezary Rojewski wrote:
> >> On 2022-07-08 6:49 PM, Andy Shevchenko wrote:
> >>> On Fri, Jul 8, 2022 at 6:32 PM Cezary Rojewski
> >>> <cezary.rojewski@intel.com> wrote:
> >>>> On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> >>>>> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> >>>>> <peter.ujfalusi@linux.intel.com> wrote:

...

> >>>> A long shot, but what if we were to modify get_options() so it takes
> >>>> additional element-size parameter instead?
> >>>
> >>> But why? int / unsigned int, u32 / s32  are all compatible in the current cases.
> >>
> >> I'd like to avoid any additional operations, so that the retrieved payload
> >> can be provided to the IPC handler directly. The IPC handlers for AudioDSP
> >> drivers are expecting payload in u32s.
> >>
> >> // u32 **tkns, size_t *num_tkns as foo() arguments
> >> // u32 *ints, int nints as locals
> >>
> >>      get_options(buf, 0, &nints);
> >>      if (!nints) {
> >>              ret = -ENOENT;
> >>              goto free_buf;
> >>      }
> >>
> >>      ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
> >>      if (!ints) {
> >>              ret = -ENOMEM;
> >>              goto free_buf;
> >>      }
> >>
> >>      get_num_options(buf, nints + 1, ints, sizeof(*ints));
> >>
> >>      *tkns = ints;
> >>      *num_tkns = nints;
> >>
> >> No additional operations in between. The intermediate IPC handler can later
> >> refer to the actual payload via &tkns[1] before passing it to the generic
> >> one.
> >>
> >> Casting int array into u32 array does not feel right, or perhaps I'm missing
> >> something like in the doc case.
> >
> > C standard.
> >
> > int to unsigned int is not promoted. And standard says that "The rank of any
> > unsigned integer type shall equal the rank of the corresponding signed integer
> > type, if any."
> >
> > I don't know why one needs to have an additional churn here. int and unsigned
> > int are interoperable with the adjustment to the sign when the other argument
> > is signed or lesser rank of.
>
> I still believe that casting blindly is not the way to go. I did
> explicitly ask about int vs u32,

There is no such type in the C standard.

> not int vs unsigned int. Please note
> that these values are later passed to the IPC handlers, and this changes
> the context a bit. If hw expects u32, then u32 it shall be.

H/W doesn't expect u32, HW expects bytes or group of bytes with:
1) dedicated address alignment (if required);
2) dedicated byte order;
3) dedicated padding (if required).

Correct me if I'm wrong.

> Please correct me if I'm wrong, but there is no guarantee that int is
> always 32bits long.

There is no guarantee by the C standard, indeed, but there is an upper
level guarantee, by the Linux kernel.

> What is guaranteed though, is that int holds at
> least -/+ 32,767. Also, values larger than INT_MAX are allowed in the
> IPC payload.

Yeah... this is binary protocol, right? So, what limits are you
talking about here if they are not applicable there anyway. It's
simply different dimension of limits (i.e. bytes and bits and not C
language types).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:

> Please correct me if I'm wrong, but there is no guarantee that int is always
> 32bits long. What is guaranteed though, is that int holds at least -/+
> 32,767. Also, values larger than INT_MAX are allowed in the IPC payload.

Right, int is just the natural size for an integer on the platform.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:

> Please correct me if I'm wrong, but there is no guarantee that int is always
> 32bits long. What is guaranteed though, is that int holds at least -/+
> 32,767. Also, values larger than INT_MAX are allowed in the IPC payload.

Right, int is just the natural size for an integer on the platform.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:
> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:

...

> I still believe that casting blindly is not the way to go. I did explicitly
> ask about int vs u32, not int vs unsigned int. Please note that these values
> are later passed to the IPC handlers, and this changes the context a bit. If
> hw expects u32, then u32 it shall be.

What you can do is probably utilize _Generic() which will reduce the code base
and allow to use the same template for different types.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:
> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:

...

> I still believe that casting blindly is not the way to go. I did explicitly
> ask about int vs u32, not int vs unsigned int. Please note that these values
> are later passed to the IPC handlers, and this changes the context a bit. If
> hw expects u32, then u32 it shall be.

What you can do is probably utilize _Generic() which will reduce the code base
and allow to use the same template for different types.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[David Laight]

>          if (pint)
> -               *pint = value;
> +               memcpy(pint, &value, min(nsize, sizeof(value)));

That is just soooooo broken.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[David Laight]

>          if (pint)
> -               *pint = value;
> +               memcpy(pint, &value, min(nsize, sizeof(value)));

That is just soooooo broken.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 11:14 AM David Laight <David.Laight@aculab.com> wrote:
>
> >          if (pint)
> > -               *pint = value;
> > +               memcpy(pint, &value, min(nsize, sizeof(value)));
>
> That is just soooooo broken.

OK.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 11:14 AM David Laight <David.Laight@aculab.com> wrote:
>
> >          if (pint)
> > -               *pint = value;
> > +               memcpy(pint, &value, min(nsize, sizeof(value)));
>
> That is just soooooo broken.

OK.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-12 4:24 PM, Andy Shevchenko wrote:
> On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:
>> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> 
> ...
> 
>> I still believe that casting blindly is not the way to go. I did explicitly
>> ask about int vs u32, not int vs unsigned int. Please note that these values
>> are later passed to the IPC handlers, and this changes the context a bit. If
>> hw expects u32, then u32 it shall be.
> 
> What you can do is probably utilize _Generic() which will reduce the code base
> and allow to use the same template for different types.


Writing to let you know that the feedback is not ignored, just my TODO 
queue is large. Will come back once _Generic() idea is verified on my 
side or when I have more questions. Thanks once again for your input Andy!


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-12 4:24 PM, Andy Shevchenko wrote:
> On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:
>> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> 
> ...
> 
>> I still believe that casting blindly is not the way to go. I did explicitly
>> ask about int vs u32, not int vs unsigned int. Please note that these values
>> are later passed to the IPC handlers, and this changes the context a bit. If
>> hw expects u32, then u32 it shall be.
> 
> What you can do is probably utilize _Generic() which will reduce the code base
> and allow to use the same template for different types.


Writing to let you know that the feedback is not ignored, just my TODO 
queue is large. Will come back once _Generic() idea is verified on my 
side or when I have more questions. Thanks once again for your input Andy!


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-12 4:24 PM, Andy Shevchenko wrote:
> On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:
>> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> 
> ...
> 
>> I still believe that casting blindly is not the way to go. I did explicitly
>> ask about int vs u32, not int vs unsigned int. Please note that these values
>> are later passed to the IPC handlers, and this changes the context a bit. If
>> hw expects u32, then u32 it shall be.
> 
> What you can do is probably utilize _Generic() which will reduce the code base
> and allow to use the same template for different types.


Hello,

I've spent some time analyzing possible utilization of _Generic() in 
context of get_options() but in my opinion get_range() complicates 
things enough that get_range() and get_option() would basically need a 
copy per type.


If Linux kernel guarantees that sizeof(int), sizeof(unsigned int), 
sizeof(s32) and sizeof(u32) are all equal (given the currently supported 
arch set), then indeed modifying get_options() may not be necessary. 
This plus shamelessly casting (u32 *) to (int *) of course.

What's left to do is the __user helper function. What I have in mind is:

int tokenize_user_input(const char __user *from, size_t count, loff_t 
*ppos, int **tkns)
{
	int *ints, nints;
	char *buf;
	int ret;

	buf = kmalloc(count + 1, GFP_KERNEL);
	if (!buf)
		return -ENOMEM;

	ret = simple_write_to_buffer(buf, count, ppos, from, count);
	if (ret != count) {
		ret = (ret < 0) ? ret : -EIO;
		goto free_buf;
	}

	buf[count] = '\0';

	get_options(buf, 0, &nints);
	if (!nints) {
		ret = -ENOENT;
		goto free_buf;
	}

	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
	if (!ints) {
		ret = -ENOMEM;
		goto free_buf;
	}

	get_options(buf, nints + 1, ints);
	*tkns = ints;
	ret = 0;

free_buf:
	kfree(buf);
	return ret;
}


Usage:
	u32 *tkns;

	ret = tokenize_user_input(from, count, ppos, (int **)&tkns);


as a part of fs/libfs.c not lib/cmdline.c. Is such approach acceptable?



Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-07-12 4:24 PM, Andy Shevchenko wrote:
> On Tue, Jul 12, 2022 at 03:51:04PM +0200, Cezary Rojewski wrote:
>> On 2022-07-09 10:42 PM, Andy Shevchenko wrote:
> 
> ...
> 
>> I still believe that casting blindly is not the way to go. I did explicitly
>> ask about int vs u32, not int vs unsigned int. Please note that these values
>> are later passed to the IPC handlers, and this changes the context a bit. If
>> hw expects u32, then u32 it shall be.
> 
> What you can do is probably utilize _Generic() which will reduce the code base
> and allow to use the same template for different types.


Hello,

I've spent some time analyzing possible utilization of _Generic() in 
context of get_options() but in my opinion get_range() complicates 
things enough that get_range() and get_option() would basically need a 
copy per type.


If Linux kernel guarantees that sizeof(int), sizeof(unsigned int), 
sizeof(s32) and sizeof(u32) are all equal (given the currently supported 
arch set), then indeed modifying get_options() may not be necessary. 
This plus shamelessly casting (u32 *) to (int *) of course.

What's left to do is the __user helper function. What I have in mind is:

int tokenize_user_input(const char __user *from, size_t count, loff_t 
*ppos, int **tkns)
{
	int *ints, nints;
	char *buf;
	int ret;

	buf = kmalloc(count + 1, GFP_KERNEL);
	if (!buf)
		return -ENOMEM;

	ret = simple_write_to_buffer(buf, count, ppos, from, count);
	if (ret != count) {
		ret = (ret < 0) ? ret : -EIO;
		goto free_buf;
	}

	buf[count] = '\0';

	get_options(buf, 0, &nints);
	if (!nints) {
		ret = -ENOENT;
		goto free_buf;
	}

	ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
	if (!ints) {
		ret = -ENOMEM;
		goto free_buf;
	}

	get_options(buf, nints + 1, ints);
	*tkns = ints;
	ret = 0;

free_buf:
	kfree(buf);
	return ret;
}


Usage:
	u32 *tkns;

	ret = tokenize_user_input(from, count, ppos, (int **)&tkns);


as a part of fs/libfs.c not lib/cmdline.c. Is such approach acceptable?



Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Aug 9, 2022 at 11:55 AM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
> On 2022-07-12 4:24 PM, Andy Shevchenko wrote:

...

> I've spent some time analyzing possible utilization of _Generic() in
> context of get_options() but in my opinion get_range() complicates
> things enough that get_range() and get_option() would basically need a
> copy per type.

Thanks for keeping us updated.

> If Linux kernel guarantees that sizeof(int), sizeof(unsigned int),
> sizeof(s32) and sizeof(u32) are all equal (given the currently supported
> arch set), then indeed modifying get_options() may not be necessary.
> This plus shamelessly casting (u32 *) to (int *) of course.

I think as long as Linux kernel states that it requires (at least)
32-bit architecture to run, we are fine. I have heard of course about
a funny project of running Linux on 8-bit microcontrollers, but it's
such a fun niche, which by the way uses emulation without changing
actual 32-bit code, that I won't even talk about.

> What's left to do is the __user helper function. What I have in mind is:
>
> int tokenize_user_input(const char __user *from, size_t count, loff_t
> *ppos, int **tkns)
> {
>         int *ints, nints;
>         char *buf;
>         int ret;
>
>         buf = kmalloc(count + 1, GFP_KERNEL);
>         if (!buf)
>                 return -ENOMEM;
>
>         ret = simple_write_to_buffer(buf, count, ppos, from, count);
>         if (ret != count) {
>                 ret = (ret < 0) ? ret : -EIO;
>                 goto free_buf;
>         }
>
>         buf[count] = '\0';

I guess this may be simplified with memdup_user(). Otherwise it looks like that.

>         get_options(buf, 0, &nints);

(You don't use ppos here, so it's pointless to use
simple_write_to_buffer(), right? I have noticed this pattern in SOF
code, which might be simplified the same way as I suggested above)

>         if (!nints) {
>                 ret = -ENOENT;
>                 goto free_buf;
>         }
>
>         ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
>         if (!ints) {
>                 ret = -ENOMEM;
>                 goto free_buf;
>         }
>
>         get_options(buf, nints + 1, ints);
>         *tkns = ints;
>         ret = 0;
>
> free_buf:
>         kfree(buf);
>         return ret;
> }

...

> as a part of fs/libfs.c not lib/cmdline.c. Is such approach acceptable?

I think so.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Aug 9, 2022 at 11:55 AM Cezary Rojewski
<cezary.rojewski@intel.com> wrote:
> On 2022-07-12 4:24 PM, Andy Shevchenko wrote:

...

> I've spent some time analyzing possible utilization of _Generic() in
> context of get_options() but in my opinion get_range() complicates
> things enough that get_range() and get_option() would basically need a
> copy per type.

Thanks for keeping us updated.

> If Linux kernel guarantees that sizeof(int), sizeof(unsigned int),
> sizeof(s32) and sizeof(u32) are all equal (given the currently supported
> arch set), then indeed modifying get_options() may not be necessary.
> This plus shamelessly casting (u32 *) to (int *) of course.

I think as long as Linux kernel states that it requires (at least)
32-bit architecture to run, we are fine. I have heard of course about
a funny project of running Linux on 8-bit microcontrollers, but it's
such a fun niche, which by the way uses emulation without changing
actual 32-bit code, that I won't even talk about.

> What's left to do is the __user helper function. What I have in mind is:
>
> int tokenize_user_input(const char __user *from, size_t count, loff_t
> *ppos, int **tkns)
> {
>         int *ints, nints;
>         char *buf;
>         int ret;
>
>         buf = kmalloc(count + 1, GFP_KERNEL);
>         if (!buf)
>                 return -ENOMEM;
>
>         ret = simple_write_to_buffer(buf, count, ppos, from, count);
>         if (ret != count) {
>                 ret = (ret < 0) ? ret : -EIO;
>                 goto free_buf;
>         }
>
>         buf[count] = '\0';

I guess this may be simplified with memdup_user(). Otherwise it looks like that.

>         get_options(buf, 0, &nints);

(You don't use ppos here, so it's pointless to use
simple_write_to_buffer(), right? I have noticed this pattern in SOF
code, which might be simplified the same way as I suggested above)

>         if (!nints) {
>                 ret = -ENOENT;
>                 goto free_buf;
>         }
>
>         ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
>         if (!ints) {
>                 ret = -ENOMEM;
>                 goto free_buf;
>         }
>
>         get_options(buf, nints + 1, ints);
>         *tkns = ints;
>         ret = 0;
>
> free_buf:
>         kfree(buf);
>         return ret;
> }

...

> as a part of fs/libfs.c not lib/cmdline.c. Is such approach acceptable?

I think so.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-08-09 5:23 PM, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 11:55 AM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:

...

> 
> I guess this may be simplified with memdup_user(). Otherwise it looks like that.

...

> (You don't use ppos here, so it's pointless to use
> simple_write_to_buffer(), right? I have noticed this pattern in SOF
> code, which might be simplified the same way as I suggested above)


Hello Andy,

Given the two major suggestions (memdup_user() and re-using 
get_options()) that had a major impact on the patch are both provided by 
you, would you like me to add any tags to the commit message? I'm 
speaking of Suggested-by or Co-developed-by and such. In you choose 
'yes', please specify tags to be added.

By the way, I've provided 'the final form' on thesofproject/linux as PR 
[1] to see if no regression is caused in basic scenarios.


[1]: https://github.com/thesofproject/linux/pull/3812


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-08-09 5:23 PM, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 11:55 AM Cezary Rojewski
> <cezary.rojewski@intel.com> wrote:

...

> 
> I guess this may be simplified with memdup_user(). Otherwise it looks like that.

...

> (You don't use ppos here, so it's pointless to use
> simple_write_to_buffer(), right? I have noticed this pattern in SOF
> code, which might be simplified the same way as I suggested above)


Hello Andy,

Given the two major suggestions (memdup_user() and re-using 
get_options()) that had a major impact on the patch are both provided by 
you, would you like me to add any tags to the commit message? I'm 
speaking of Suggested-by or Co-developed-by and such. In you choose 
'yes', please specify tags to be added.

By the way, I've provided 'the final form' on thesofproject/linux as PR 
[1] to see if no regression is caused in basic scenarios.


[1]: https://github.com/thesofproject/linux/pull/3812


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Aug 16, 2022 at 11:28:46AM +0200, Cezary Rojewski wrote:
> On 2022-08-09 5:23 PM, Andy Shevchenko wrote:

...

> Given the two major suggestions (memdup_user() and re-using get_options())
> that had a major impact on the patch are both provided by you, would you
> like me to add any tags to the commit message? I'm speaking of Suggested-by
> or Co-developed-by and such. In you choose 'yes', please specify tags to be
> added.

Suggested-by would be enough.

> By the way, I've provided 'the final form' on thesofproject/linux as PR [1]
> to see if no regression is caused in basic scenarios.

When you will be ready, send it for upstream review. It would be easier for
the kernel community to look at and comment on.

> [1]: https://github.com/thesofproject/linux/pull/3812

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Andy Shevchenko]

On Tue, Aug 16, 2022 at 11:28:46AM +0200, Cezary Rojewski wrote:
> On 2022-08-09 5:23 PM, Andy Shevchenko wrote:

...

> Given the two major suggestions (memdup_user() and re-using get_options())
> that had a major impact on the patch are both provided by you, would you
> like me to add any tags to the commit message? I'm speaking of Suggested-by
> or Co-developed-by and such. In you choose 'yes', please specify tags to be
> added.

Suggested-by would be enough.

> By the way, I've provided 'the final form' on thesofproject/linux as PR [1]
> to see if no regression is caused in basic scenarios.

When you will be ready, send it for upstream review. It would be easier for
the kernel community to look at and comment on.

> [1]: https://github.com/thesofproject/linux/pull/3812

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-08-25 5:09 PM, Andy Shevchenko wrote:
> On Tue, Aug 16, 2022 at 11:28:46AM +0200, Cezary Rojewski wrote:
>> On 2022-08-09 5:23 PM, Andy Shevchenko wrote:

...

> Suggested-by would be enough.
> 
>> By the way, I've provided 'the final form' on thesofproject/linux as PR [1]
>> to see if no regression is caused in basic scenarios.
> 
> When you will be ready, send it for upstream review. It would be easier for
> the kernel community to look at and comment on.

On its way. Was awaiting your replay so I do not need to send the series 
twice : ) The PR was there to check if there is no regression on SOF 
side, at least on a BAT (Basic Acceptable Tests) level.


Regards,
Czarek

Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski]

On 2022-08-25 5:09 PM, Andy Shevchenko wrote:
> On Tue, Aug 16, 2022 at 11:28:46AM +0200, Cezary Rojewski wrote:
>> On 2022-08-09 5:23 PM, Andy Shevchenko wrote:

...

> Suggested-by would be enough.
> 
>> By the way, I've provided 'the final form' on thesofproject/linux as PR [1]
>> to see if no regression is caused in basic scenarios.
> 
> When you will be ready, send it for upstream review. It would be easier for
> the kernel community to look at and comment on.

On its way. Was awaiting your replay so I do not need to send the series 
twice : ) The PR was there to check if there is no regression on SOF 
side, at least on a BAT (Basic Acceptable Tests) level.


Regards,
Czarek