Re: [PATCH] scsi_debug: add cdb_len parameter

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Wed, 2017-11-15 at 23:36 -0500, Douglas Gilbert wrote:
> -#define SDEBUG_VERSION "1.86"
> -static const char *sdebug_version_date = "20160430";
> +#define SDEBUG_VERSION "1.87"
> +static const char *sdebug_version_date = "20171105";

Is this kind of version information really useful for an upstream driver? Can
this version information be removed?

> +static void all_config_cdb_len(void)
> +{
> +	struct sdebug_host_info *sdbg_host;
> +	struct Scsi_Host *shost;
> +	struct scsi_device *sdev;
> +
> +
> +	spin_lock(&sdebug_host_list_lock);

A minor remark, but anyway: other kernel code only uses a single blank line
between declarations and code.

> +static ssize_t cdb_len_store(struct device_driver *ddp, const char *buf,
> +			     size_t count)
> +{
> +	int n;
> +
> +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
> +		sdebug_cdb_len = n;
> +		all_config_cdb_len();
> +		return count;
> +	}
> +	return -EINVAL;
> +}

Checkpatch should have recommended to use kstrtoint() instead of sscanf().
The count check is not necessary since the buffer passed to sysfs store
functions is '\0'-terminated. Additionally, please do not use parentheses if
these are not necessary. The approach used in most parts of the kernel to
handle errors is the opposite of the above, namely:

	error = /* process inputs */
	if (error)
		return error;
	/* actual processing code */

Please consider to follow that style.

Thanks,

Bart.