Re: [RFC PATCH v2 1/2] scsi: ufs: Add Multi-Circular Queue support

From: Bart Van Assche
Date: Thu Aug 18 2022 - 16:25:12 EST


On 8/12/22 02:10, Manivannan Sadhasivam wrote:
On Thu, Aug 11, 2022 at 03:33:03AM -0700, Can Guo wrote:
+static unsigned int dev_cmd_queue = 1;
+static unsigned int read_queues;

Where is this initialized?

The Linux kernel coding style does not allow to explicitly initialize static variables to zero.

+
+static int rw_queue_count_set(const char *val, const struct kernel_param *kp)
+{
+ unsigned int n;
+ int ret;
+
+ ret = kstrtouint(val, 10, &n);
+ if (ret)
+ return ret;
+ if (n > num_possible_cpus())
+ return -EINVAL;
+ return param_set_uint(val, kp);
+}
+
+static const struct kernel_param_ops rw_queue_count_ops = {
+ .set = rw_queue_count_set,
+ .get = param_get_uint,
+};
+
+static unsigned int rw_queues = 8;
+module_param_cb(rw_queues, &rw_queue_count_ops, &rw_queues, 0644);
+MODULE_PARM_DESC(rw_queues, "Number of queues used for rw. Default value is 8");
+

Using module params is not encouraged these days. So please switch to Kconfig.

Hmm ... I think using CONFIG_* variables is worse than using module parameters since modifying CONFIG_* variables requires rebuilding the kernel.

+struct cq_entry {
+ /* DW 0-1 */
+ __le32 command_desc_base_addr_lo;
+ __le32 command_desc_base_addr_hi;
+
+ /* DW 2 */
+ __le16 response_upiu_length;
+ __le16 response_upiu_offset;
+
+ /* DW 3 */
+ __le16 prd_table_length;
+ __le16 prd_table_offset;
+
+ /* DW 4 */
+ __le32 status;
+
+ /* DW 5-7 */
+ u32 reserved[3];
+};

packed?

Using __packed if it is not necessary is wrong since it makes code slower.

Thanks,

Bart.