RE: [PATCH v3] usb-storage: Optimize scan delay more precisely

[Norihiko Hama]

+AD4APg- On Tue, Apr 16, 2024 at 05:28:21PM +-0900, Norihiko Hama wrote:
+AD4- At this location you're supposed to describe how this version of the patch differs from the previous version.

Thank you for your support.
The difference from previous version is parsing parameter by kstrtouint() only with padding to 3 digit '0' based on your advice.
I'll split parser function and simplify get() function on next version.

+AD4APg- --- a/Documentation/admin-guide/kernel-parameters.txt
+AD4APg- +-+-+- b/Documentation/admin-guide/kernel-parameters.txt
+AD4APg- +AEAAQA- -6190,6 +-6190,16 +AEAAQA-
+AD4APg-  	usb-storage.delay+AF8-use+AD0-
+AD4APg-  			+AFs-UMS+AF0- The delay in seconds before a new device is
+AD4APg-  			scanned for Logical Units (default 1).
+AD4APg- +-			To specify more precise delay, supports 3 decimal point.
+AD4APg- +-			The range of decimal point is in milliseconds,
+AD4APg- +-			hence the minimum value is +ACI-0.001+ACI-.
+AD4- 
+AD4- The text could be better.  For example:
+AD4- 
+AD4-			The delay can have up to 3 decimal places, giving a
+AD4-			resolution of one millisecond.

Thank you for your adevice.

+AD4APg- +-			Example:
+AD4APg- +-				delay+AF8-use+AD0-1
+AD4APg- +-					1 second delay
+AD4APg- +-				delay+AF8-use+AD0-0.1
+AD4APg- +-					0.1 second delay
+AD4APg- +-				delay+AF8-use+AD0-2.55
+AD4APg- +-					2.55 second elay
+AD4- 
+AD4- This should show all 3 decimal places:
+AD4- 
+AD4-				delay+AF8-use+AD0-2.567
+AD4-					2.567 second delay

I see.

+AD4- As I said before, the parsing code should be in a separate function to make reviewing the code easier.  It also should be written more clearly.  Here's my attempt (not tested at all).  You might prefer to remove some of the comments+ADs- I put in a lot of them.
+AD4- 
+AD4- /+ACoAKg-
+AD4-  +ACo- str+AF8-to+AF8-fixed+AF8-point+AF8-uint - parse an unsigned fixed-point decimal integer
+AD4-  +ACo- +AEA-str: String to parse.
+AD4-  +ACo- +AEA-ndecimals: Number of decimal places in the fixed-point value.
+AD4-  +ACo- +AEA-val: Where to store the parsed value.
+AD4-  +ACo-
+AD4-  +ACo- Parse an unsigned fixed-point decimal value in +AEA-str, containing at
+AD4-  +ACo- most ndecimal digits to the right of the decimal point.
+AD4-  +ACo- Stores the parsed value in +AEA-val, scaled by 10+AF4-(+AEA-ndecimal).
+AD4-  +ACo-
+AD4-  +ACo- As with kstrtouint(), the string must be NUL-terminated and may
+AD4-  +ACo- include a single newline before the terminating NUL.  The first
+AD4-  +ACo- character may be a plus sign but not a minus sign.  The decimal
+AD4-  +ACo- point and fractional digits are optional.
+AD4-  +ACo-
+AD4-  +ACo- Returns 0 on success, a negative error code otherwise.
+AD4-  +ACo-/
+AD4- static int str+AF8-to+AF8-fixed+AF8-point+AF8-uint(const char +ACo-str, int ndecimals,
+AD4-		unsigned int +ACo-val)
+AD4- +AHs-
+AD4-	int n, n1, n2+ADs-
+AD4-	const char +ACo-p+ADs-
+AD4-	char +ACo-q+ADs-
+AD4-	char buf+AFs-16+AF0AOw-
+AD4-
+AD4-	n +AD0- strlen(str)+ADs-
+AD4-	if (n +AD4- 0 +ACYAJg- str+AFs-n - 1+AF0- +AD0APQ- '+AFw-n')+ADs-
+AD4-		--n+ADs-
+AD4-
+AD4-	p +AD0- strchr(str, '.')+ADs-
+AD4-	if (p) +AHs-
+AD4-		n1 +AD0- p+-+- - str+ADs-		/+ACo- Length of integral part +ACo-/
+AD4-		n2 +AD0- n - (n1 +- 1)+ADs-	/+ACo- Length of fractional part +ACo-/
+AD4-		if (n2 +AD4- ndecimals)
+AD4-			return -EINVAL+ADs-
+AD4-	+AH0- else +AHs-
+AD4-		n1 +AD0- n+ADs-			/+ACo- Length of integral part +ACo-/
+AD4-		n2 +AD0- 0+ADs-			/+ACo- No fractional part +ACo-/
+AD4-	+AH0-
+AD4-	if (n1 +- n2 +AD0APQ- 0 +AHwAfA- n1 +- ndecimals +AD4- sizeof(buf) - 1)
+AD4-		return -EINVAL+ADs-		/+ACo- No digits present or too long +ACo-/
+AD4-
+AD4-	memcpy(buf, str, n1)+ADs-		/+ACo- Integer part +ACo-/
+AD4-	memcpy(buf +- n1, p, n2)+ADs-	/+ACo- Fractional part +ACo-/
+AD4-	for (q +AD0- buf +- n1 +- n2+ADs- n2 +ADw- ndecimals+ADs- +-+-n2)
+AD4-		+ACo-q+-+- +AD0- '0'+ADs-		/+ACo- Remaining fractional digits +ACo-/
+AD4-	+ACo-q +AD0- 0+ADs-
+AD4-
+AD4-	return kstrtouint(buf, 10, val)+ADs-
+AD4- +AH0-

Thank you for your help.
I'll reconsider the code changes and test it.

+AD4APg- +-
+AD4APg- +-static int delay+AF8-use+AF8-get(char +ACo-s, const struct kernel+AF8-param +ACo-kp) +AHs-
+AD4APg- +-	unsigned int delay+AF8-ms +AD0- +ACo-((unsigned int +ACo-)kp-+AD4-arg)+ADs-
+AD4APg- +-	unsigned int rem +AD0- do+AF8-div(delay+AF8-ms, MSEC+AF8-PER+AF8-SEC)+ADs-
+AD4APg- +-	int len+ADs-
+AD4APg- +-	char buf+AFs-16+AF0AOw-
+AD4APg- +-
+AD4APg- +-	len +AD0- scnprintf(buf, sizeof(buf), +ACIAJQ-d+ACI-, delay+AF8-ms)+ADs-
+AD4APg- +-	if (rem) +AHs-
+AD4APg- +-		len +-+AD0- scnprintf(buf +- len, sizeof(buf) - len, +ACI-.+ACU-03d+ACI-, rem)+ADs-
+AD4APg- +-		while (buf+AFs-len - 1+AF0- +AD0APQ- '0') +AHs-
+AD4APg- +-			buf+AFs-len - 1+AF0- +AD0- '+AFw-0'+ADs-
+AD4APg- +-			if (--len +ADwAPQ- 1)
+AD4APg- +-				break+ADs-
+AD4APg- +-		+AH0-
+AD4APg- +-	+AH0-
+AD4- 
+AD4- While this could also go in a separate function, it's short enough to keep here.

OK, I see.