From: yanling.song@linux.dev
To: "Bart Van Assche" <bvanassche@acm.org>,
"Yanling Song" <songyl@ramaxel.com>,
martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org, yujiang@ramaxel.com,
xuyun@ramaxel.com, yanggan@ramaxel.com
Subject: Re: [PATCH V2] scsi:spraid: initial commit of Ramaxel spraid driver
Date: Sun, 12 Dec 2021 02:56:01 +0000 [thread overview]
Message-ID: <52b05051cf00a9ad617c31f227654dcc@linux.dev> (raw)
In-Reply-To: <399c013b-aaf9-1781-09a1-1acbc82b49ae@acm.org>
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.
next prev parent reply other threads:[~2021-12-12 2:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-26 7:33 [PATCH V2] scsi:spraid: initial commit of Ramaxel spraid driver Yanling Song
2021-11-26 16:21 ` James Bottomley
2021-11-26 19:24 ` Randy Dunlap
[not found] ` <20211130113449.45751209@songyl>
2021-12-01 0:26 ` Yanling song
2021-11-29 13:04 ` Hannes Reinecke
[not found] ` <20211130113836.1bb8e91c@songyl>
2021-11-30 11:55 ` Hannes Reinecke
2021-12-02 13:56 ` Yanling song
2021-12-06 17:00 ` Bart Van Assche
2021-12-09 23:15 ` Bart Van Assche
2021-12-10 17:42 ` Bart Van Assche
2021-12-12 3:02 ` Yanling song
2021-12-10 21:40 ` Bart Van Assche
2021-12-12 3:04 ` Yanling song
2021-12-12 2:56 ` yanling.song [this message]
2021-12-27 7:55 ` Yanling Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52b05051cf00a9ad617c31f227654dcc@linux.dev \
--to=yanling.song@linux.dev \
--cc=bvanassche@acm.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=songyl@ramaxel.com \
--cc=xuyun@ramaxel.com \
--cc=yanggan@ramaxel.com \
--cc=yujiang@ramaxel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).