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.
