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

[yanling.song@linux.dev]

December 10, 2021 7:15 AM, "Bart Van Assche" <bvanassche@acm.org> wrote:

> 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.
> 
The pointer of pci_dev is from linux driver frame work probe() function, 
spraid driver does not allocate memory for it, just save the pointer.

>> + struct device *dev;
> 
> What does this pointer represent? There is already a struct device inside struct pci_dev. Can
> this member be left out?
> 
The pointer of dev is from struct pci_dev. It is saved in struct spraid_dev just for convenience:
so that we do not need to get the dev from pci_dev every time when using dev.

>> + 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.
> 
Comments will be removed in the next version.

>> +#include <linux/version.h>
> 
> Upstream drivers should not include this header file.
>
Ok. Changes will be included in the next version.

>> +static u32 admin_tmout = 60;
>> +module_param(admin_tmout, uint, 0644);
>> +MODULE_PARM_DESC(admin_tmout, "admin commands timeout (seconds)");
>> +
Will be changed to per SCSI host.

>> +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)");
>> +
Will be deleted.

>> +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)");
>> +
Will be splitted into two attributes of scsi host:scmd_tmout_rawdisk, scmd_tmout_vd

>> +static u32 wait_abl_tmout = 3;
>> +module_param(wait_abl_tmout, uint, 0644);
>> +MODULE_PARM_DESC(wait_abl_tmout, "wait abnormal io timeout(seconds)");
>> +
Will be deleted.

>> +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");
>
Will be deleted.
 
> 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.
> 
Comments as the above.

>> +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"?
>
Yes. it is the same as SCSI sysfs attribute "can_queue". 
The maximum queue depth supported by hardware is 1024.  
The parameter is to change the queue depth for different usages to get the best performance.

 
>> +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()?
>
Ye. Will use pr_debug in the next version.

 
>> +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.
>
Will add more comments:
It was found that the spindlock of a single pool conflicts a lot with multiple CPUs.
So multiple pools are introduced to reduce the conflictions. 

 
> Thanks,
> 
> Bart.