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...