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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 19 Apr 2024 07:00:04 +0000
From: Norihiko Hama <norihiko.hama@...salpine.com>
To: Alan Stern <stern@...land.harvard.edu>
CC: "mdharm-usb@...-eyed-alien.net" <mdharm-usb@...-eyed-alien.net>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"usb-storage@...ts.one-eyed-alien.net" <usb-storage@...ts.one-eyed-alien.net>
Subject: RE: [PATCH v3] usb-storage: Optimize scan delay more precisely

>> On Tue, Apr 16, 2024 at 05:28:21PM +0900, Norihiko Hama wrote:
> 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.

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

Thank you for your adevice.

>> +			Example:
>> +				delay_use=1
>> +					1 second delay
>> +				delay_use=0.1
>> +					0.1 second delay
>> +				delay_use=2.55
>> +					2.55 second elay
> 
> This should show all 3 decimal places:
> 
>				delay_use=2.567
>					2.567 second delay

I see.

> 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; I put in a lot of them.
> 
> /**
>  * str_to_fixed_point_uint - parse an unsigned fixed-point decimal integer
>  * @str: String to parse.
>  * @ndecimals: Number of decimal places in the fixed-point value.
>  * @val: Where to store the parsed value.
>  *
>  * Parse an unsigned fixed-point decimal value in @str, containing at
>  * most ndecimal digits to the right of the decimal point.
>  * Stores the parsed value in @val, scaled by 10^(@ndecimal).
>  *
>  * As with kstrtouint(), the string must be NUL-terminated and may
>  * include a single newline before the terminating NUL.  The first
>  * character may be a plus sign but not a minus sign.  The decimal
>  * point and fractional digits are optional.
>  *
>  * Returns 0 on success, a negative error code otherwise.
>  */
> static int str_to_fixed_point_uint(const char *str, int ndecimals,
>		unsigned int *val)
> {
>	int n, n1, n2;
>	const char *p;
>	char *q;
>	char buf[16];
>
>	n = strlen(str);
>	if (n > 0 && str[n - 1] == '\n');
>		--n;
>
>	p = strchr(str, '.');
>	if (p) {
>		n1 = p++ - str;		/* Length of integral part */
>		n2 = n - (n1 + 1);	/* Length of fractional part */
>		if (n2 > ndecimals)
>			return -EINVAL;
>	} else {
>		n1 = n;			/* Length of integral part */
>		n2 = 0;			/* No fractional part */
>	}
>	if (n1 + n2 == 0 || n1 + ndecimals > sizeof(buf) - 1)
>		return -EINVAL;		/* No digits present or too long */
>
>	memcpy(buf, str, n1);		/* Integer part */
>	memcpy(buf + n1, p, n2);	/* Fractional part */
>	for (q = buf + n1 + n2; n2 < ndecimals; ++n2)
>		*q++ = '0';		/* Remaining fractional digits */
>	*q = 0;
>
>	return kstrtouint(buf, 10, val);
> }

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

>> +
>> +static int delay_use_get(char *s, const struct kernel_param *kp) {
>> +	unsigned int delay_ms = *((unsigned int *)kp->arg);
>> +	unsigned int rem = do_div(delay_ms, MSEC_PER_SEC);
>> +	int len;
>> +	char buf[16];
>> +
>> +	len = scnprintf(buf, sizeof(buf), "%d", delay_ms);
>> +	if (rem) {
>> +		len += scnprintf(buf + len, sizeof(buf) - len, ".%03d", rem);
>> +		while (buf[len - 1] == '0') {
>> +			buf[len - 1] = '\0';
>> +			if (--len <= 1)
>> +				break;
>> +		}
>> +	}
> 
> While this could also go in a separate function, it's short enough to keep here.

OK, I see.