Re: [PATCH V2] scsi:spraid: initial commit of Ramaxel spraid driver

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

On 11/25/21 11:33 PM, Yanling Song wrote:
> +struct spraid_dev {
> +	struct pci_dev *pdev;

Why a pointer to struct pci_dev instead of embedding struct pci_dev in this structure? The
latter approach is more efficient.

> +	struct device *dev;

What does this pointer represent? There is already a struct device inside struct pci_dev. Can
this member be left out?

> +	struct spraid_cmd *adm_cmds;
> +	struct list_head adm_cmd_list;
> +	spinlock_t adm_cmd_lock; /* spinlock for lock handling */
> +
> +	struct spraid_cmd *ioq_ptcmds;
> +	struct list_head ioq_pt_list;
> +	spinlock_t ioq_pt_lock; /* spinlock for lock handling */
> +
> +	struct work_struct scan_work;
> +	struct work_struct timesyn_work;
> +	struct work_struct reset_work;
> +
> +	enum spraid_state state;
> +	spinlock_t state_lock; /* spinlock for lock handling */
> +	struct request_queue *bsg_queue;

The "spinlock for lock handling" comments are not useful. Please make these comments useful or
remove these.

> +#include <linux/version.h>

Upstream drivers should not include this header file.

> +static u32 admin_tmout = 60;
> +module_param(admin_tmout, uint, 0644);
> +MODULE_PARM_DESC(admin_tmout, "admin commands timeout (seconds)");
> +
> +static u32 scmd_tmout_pt = 30;
> +module_param(scmd_tmout_pt, uint, 0644);
> +MODULE_PARM_DESC(scmd_tmout_pt, "scsi commands timeout for passthrough(seconds)");
> +
> +static u32 scmd_tmout_nonpt = 180;
> +module_param(scmd_tmout_nonpt, uint, 0644);
> +MODULE_PARM_DESC(scmd_tmout_nonpt, "scsi commands timeout for rawdisk&raid(seconds)");
> +
> +static u32 wait_abl_tmout = 3;
> +module_param(wait_abl_tmout, uint, 0644);
> +MODULE_PARM_DESC(wait_abl_tmout, "wait abnormal io timeout(seconds)");
> +
> +static bool use_sgl_force;
> +module_param(use_sgl_force, bool, 0644);
> +MODULE_PARM_DESC(use_sgl_force, "force IO use sgl format, default false");

Consider leaving out all kernel module parameters. Kernel module parameters are easy to introduce
but can't be removed. Is it really necessary that the above parameters can be configured? If so,
most of the above parameters probably should be per SCSI host or SCSI device instead of module
parameters.

> +static u32 io_queue_depth = 1024;
> +module_param_cb(io_queue_depth, &ioq_depth_ops, &io_queue_depth, 0644);
> +MODULE_PARM_DESC(io_queue_depth, "set io queue depth, should >= 2");

How does this differ from the SCSI sysfs attribute "can_queue"?

> +static unsigned char log_debug_switch;
> +module_param_cb(log_debug_switch, &log_debug_switch_ops, &log_debug_switch, 0644);
> +MODULE_PARM_DESC(log_debug_switch, "set log state, default non-zero for switch on");

Can this parameter be left out by using pr_debug()?

> +static unsigned char small_pool_num = 4;
> +module_param_cb(small_pool_num, &small_pool_num_ops, &small_pool_num, 0644);
> +MODULE_PARM_DESC(small_pool_num, "set prp small pool num, default 4, MAX 16");

The description of the above parameter is too cryptic.

Thanks,

Bart.