Re: [PATCH v3] scsi_debug: randomize command duration option

[Bart Van Assche <bvanassche@acm.org>]

On 10/1/19 8:27 PM, Douglas Gilbert wrote:
> -			kt = ns_to_ktime((u64)delta_jiff * (NSEC_PER_SEC / HZ));
> -		} else
> -			kt = ndelay;
> +			u64 ns = jiffies_to_nsecs(delta_jiff);
> +
> +			if (sdebug_random && ns < U32_MAX) {
> +				ns = prandom_u32_max((u32)ns);
> +			} else if (sdebug_random) {
> +				ns >>= 12;	/* scale to 4 usec precision */
> +				if (ns < U32_MAX)	/* over 4 hours max */
> +					ns = prandom_u32_max((u32)ns);
> +				ns <<= 12;

An explanation of how the scaling factor has been chosen is missing.

Is it user friendly to disable randomization silently if delta_jiff 
exceeds a certain value? How about restricting delta_jiff to e.g. ten 
times the SCSI timeout?

> +		} else {	/* ndelay has a 4.2 second max */
> +			kt = sdebug_random ? prandom_u32_max((u32)ndelay) :
> +					     (u32)ndelay;
> +		}

Are these (u32) casts necessary?

> +static ssize_t random_show(struct device_driver *ddp, char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", (int)sdebug_random);
> +}

Is the (int) cast necessary?

> +static ssize_t random_store(struct device_driver *ddp, const char *buf,
> +			    size_t count)
> +{
> +	int n;
> +
> +	if (count > 0 && kstrtoint(buf, 10, &n) == 0 && n >= 0) {
> +		sdebug_random = (n > 0);
> +		return count;
> +	}
> +	return -EINVAL;
> +}
> +static DRIVER_ATTR_RW(random);

Is the "count > 0" check useful? I don't think that sysfs ever passes 
count == 0 to store callback functions.

Thanks,

Bart.