[PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[bo yang]

Adding module parameters to configure max sectors per request & # of cmds per lun.

Signed-off-by: Bo Yang <bo.yang@lsi.com>

---
 drivers/scsi/megaraid/megaraid_sas.c |   94 ++++++++++++++++++++++++-
 drivers/scsi/megaraid/megaraid_sas.h |    2
 2 files changed, 94 insertions(+), 2 deletions(-)

diff -uprN linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c
--- linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c	2007-09-26 13:30:36.000000000 -0700
+++ linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c	2007-09-27 20:37:11.000000000 -0700
@@ -62,6 +62,23 @@ MODULE_PARM_DESC(fast_load,
 	"megasas: Faster loading of the driver, skips physical devices! \
 	 (default = 0)");
 
+/*
+ * Number of sectors per IO command will be set in megasas_init_mfi
+ * if user does not provide
+ */
+static unsigned int max_sectors;
+module_param_named(max_sectors, max_sectors, int, 0);
+MODULE_PARM_DESC(max_sectors,
+	"Maximum number of sectors per IO command");
+
+/*
+ * Number of cmds per logical unit
+ */
+static unsigned int cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
+module_param_named(cmd_per_lun, cmd_per_lun, int, 0);
+MODULE_PARM_DESC(cmd_per_lun,
+	"Maximum number of commands per logical unit (default=128)");
+
 MODULE_LICENSE("GPL");
 MODULE_VERSION(MEGASAS_VERSION);
 MODULE_AUTHOR("megaraidlinux@lsi.com");
@@ -2301,6 +2318,31 @@ static int megasas_start_aen(struct mega
 				    class_locale.word);
 }
 
+static ssize_t
+sysfs_max_sectors_read(struct kobject *kobj,
+			struct bin_attribute *bin_attr,
+			char *buf, loff_t off, size_t count)
+{
+	struct Scsi_Host *host = class_to_shost(container_of(kobj,
+					struct class_device, kobj));
+	struct megasas_instance *instance =
+				(struct megasas_instance *)host->hostdata;
+
+	count = sprintf(buf, "%u\n", instance->max_sectors_per_req);
+
+	return count+1;
+}
+
+static struct bin_attribute sysfs_max_sectors_attr = {
+	.attr = {
+		.name = "max_sectors",
+		.mode = S_IRUSR|S_IRGRP|S_IROTH,
+		.owner = THIS_MODULE,
+	},
+	.size = 7,
+	.read = sysfs_max_sectors_read,
+};
+
 /**
  * megasas_io_attach -	Attaches this driver to SCSI mid-layer
  * @instance:		Adapter soft state
@@ -2308,17 +2350,48 @@ static int megasas_start_aen(struct mega
 static int megasas_io_attach(struct megasas_instance *instance)
 {
 	struct Scsi_Host *host = instance->host;
+	int error;
 
 	/*
-	 * Export parameters required by SCSI mid-layer
+	 * Export host parameters required by SCSI
+	 * mid-layer
 	 */
 	host->irq = instance->pdev->irq;
 	host->unique_id = instance->unique_id;
 	host->can_queue = instance->max_fw_cmds - MEGASAS_INT_CMDS;
 	host->this_id = instance->init_id;
 	host->sg_tablesize = instance->max_num_sge;
+
+	/*
+	 * Check if the module parameter value for max_sectors can be used
+	 */
+	if (max_sectors && max_sectors <= instance->max_sectors_per_req)
+		instance->max_sectors_per_req = max_sectors;
+	else {
+		if (max_sectors)
+			printk(KERN_INFO
+				"megasas: max_sectors should be > 0 and"
+				"<= %d\n",
+				instance->max_sectors_per_req);
+	}
+
 	host->max_sectors = instance->max_sectors_per_req;
-	host->cmd_per_lun = 128;
+
+	/*
+	 * Check if the module parameter value for cmd_per_lun can be used
+	 */
+	instance->cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
+	if (cmd_per_lun && cmd_per_lun <= MEGASAS_DEFAULT_CMD_PER_LUN)
+		instance->cmd_per_lun = cmd_per_lun;
+	else
+		printk(KERN_INFO "megasas: cmd_per_lun should be > 0 and"
+			"<= %d\n", MEGASAS_DEFAULT_CMD_PER_LUN);
+
+	host->cmd_per_lun = instance->cmd_per_lun;
+
+	printk(KERN_DEBUG "megasas: max_sectors : 0x%x, cmd_per_lun : 0x%x\n",
+			instance->max_sectors_per_req, instance->cmd_per_lun);
+
 	host->max_channel = MEGASAS_MAX_CHANNELS - 1;
 	host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
 	host->max_lun = MEGASAS_MAX_LUN;
@@ -2333,10 +2406,25 @@ static int megasas_io_attach(struct mega
 	}
 
 	/*
+	 * Create sysfs entries for module paramaters
+	 */
+	error = sysfs_create_bin_file(&instance->host->shost_classdev.kobj,
+			&sysfs_max_sectors_attr);
+	if (error) {
+		printk(KERN_INFO "megasas: Error in creating the sysfs entry"
+				" max_sectors.\n");
+		goto out_remove_host;
+	}
+
+	/*
 	 * Trigger SCSI to scan our drives
 	 */
 	scsi_scan_host(host);
 	return 0;
+
+out_remove_host:
+	scsi_remove_host(host);
+	return error;
 }
 
 static int
@@ -2750,6 +2838,8 @@ static void megasas_detach_one(struct pc
 	instance = pci_get_drvdata(pdev);
 	host = instance->host;
 
+	sysfs_remove_bin_file(&host->shost_classdev.kobj,
+				&sysfs_max_sectors_attr);
 	scsi_remove_host(instance->host);
 	megasas_flush_cache(instance);
 	megasas_shutdown_controller(instance, MR_DCMD_CTRL_SHUTDOWN);
diff -uprN linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.h linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.h
--- linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.h	2007-09-26 13:27:59.000000000 -0700
+++ linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.h	2007-09-26 13:27:31.000000000 -0700
@@ -537,6 +537,7 @@ struct megasas_ctrl_info {
 #define MEGASAS_DEFAULT_INIT_ID			-1
 #define MEGASAS_MAX_LUN				8
 #define MEGASAS_MAX_LD				64
+#define MEGASAS_DEFAULT_CMD_PER_LUN		128
 
 #define MEGASAS_DBG_LVL				1
 
@@ -1080,6 +1081,7 @@ struct megasas_instance {
 	u16 max_num_sge;
 	u16 max_fw_cmds;
 	u32 max_sectors_per_req;
+	u32 cmd_per_lun;
 
 	struct megasas_cmd **cmd_list;
 	struct list_head cmd_pool;

Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Randy Dunlap]

On Wed, 26 Sep 2007 11:27:50 -0400 bo yang wrote:

> Adding module parameters to configure max sectors per request & # of cmds per lun.
> 
> Signed-off-by: Bo Yang <bo.yang@lsi.com>
> 
> ---
>  drivers/scsi/megaraid/megaraid_sas.c |   94 ++++++++++++++++++++++++-
>  drivers/scsi/megaraid/megaraid_sas.h |    2
>  2 files changed, 94 insertions(+), 2 deletions(-)
> 
> diff -uprN linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c
> --- linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c	2007-09-26 13:30:36.000000000 -0700
> +++ linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c	2007-09-27 20:37:11.000000000 -0700

> @@ -2301,6 +2318,31 @@ static int megasas_start_aen(struct mega
>  				    class_locale.word);
>  }
>  
> +static ssize_t
> +sysfs_max_sectors_read(struct kobject *kobj,
> +			struct bin_attribute *bin_attr,
> +			char *buf, loff_t off, size_t count)
> +{
> +	struct Scsi_Host *host = class_to_shost(container_of(kobj,
> +					struct class_device, kobj));
> +	struct megasas_instance *instance =
> +				(struct megasas_instance *)host->hostdata;
> +
> +	count = sprintf(buf, "%u\n", instance->max_sectors_per_req);
> +
> +	return count+1;

What's the +1 for?

> +}
> +
> +static struct bin_attribute sysfs_max_sectors_attr = {
> +	.attr = {
> +		.name = "max_sectors",
> +		.mode = S_IRUSR|S_IRGRP|S_IROTH,
> +		.owner = THIS_MODULE,
> +	},
> +	.size = 7,
> +	.read = sysfs_max_sectors_read,
> +};
> +
>  /**
>   * megasas_io_attach -	Attaches this driver to SCSI mid-layer
>   * @instance:		Adapter soft state
> @@ -2308,17 +2350,48 @@ static int megasas_start_aen(struct mega
>  static int megasas_io_attach(struct megasas_instance *instance)
>  {
>  	struct Scsi_Host *host = instance->host;
> +	int error;
>  
>  	/*
> -	 * Export parameters required by SCSI mid-layer
> +	 * Export host parameters required by SCSI
> +	 * mid-layer
>  	 */
>  	host->irq = instance->pdev->irq;
>  	host->unique_id = instance->unique_id;
>  	host->can_queue = instance->max_fw_cmds - MEGASAS_INT_CMDS;
>  	host->this_id = instance->init_id;
>  	host->sg_tablesize = instance->max_num_sge;
> +
> +	/*
> +	 * Check if the module parameter value for max_sectors can be used
> +	 */
> +	if (max_sectors && max_sectors <= instance->max_sectors_per_req)
> +		instance->max_sectors_per_req = max_sectors;
> +	else {
> +		if (max_sectors)
> +			printk(KERN_INFO
> +				"megasas: max_sectors should be > 0 and"

Need a space after "and" above or before "<=" below here.

> +				"<= %d\n",
> +				instance->max_sectors_per_req);
> +	}
> +
>  	host->max_sectors = instance->max_sectors_per_req;
> -	host->cmd_per_lun = 128;
> +
> +	/*
> +	 * Check if the module parameter value for cmd_per_lun can be used
> +	 */
> +	instance->cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
> +	if (cmd_per_lun && cmd_per_lun <= MEGASAS_DEFAULT_CMD_PER_LUN)
> +		instance->cmd_per_lun = cmd_per_lun;
> +	else
> +		printk(KERN_INFO "megasas: cmd_per_lun should be > 0 and"

Same comment as above.

> +			"<= %d\n", MEGASAS_DEFAULT_CMD_PER_LUN);
> +
> +	host->cmd_per_lun = instance->cmd_per_lun;
> +
> +	printk(KERN_DEBUG "megasas: max_sectors : 0x%x, cmd_per_lun : 0x%x\n",
> +			instance->max_sectors_per_req, instance->cmd_per_lun);
> +
>  	host->max_channel = MEGASAS_MAX_CHANNELS - 1;
>  	host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
>  	host->max_lun = MEGASAS_MAX_LUN;

---
~Randy
Phaedrus says that Quality is about caring.

Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Brian King]

bo yang wrote:

> +static ssize_t
> +sysfs_max_sectors_read(struct kobject *kobj,
> +			struct bin_attribute *bin_attr,
> +			char *buf, loff_t off, size_t count)
> +{
> +	struct Scsi_Host *host = class_to_shost(container_of(kobj,
> +					struct class_device, kobj));
> +	struct megasas_instance *instance =
> +				(struct megasas_instance *)host->hostdata;
> +
> +	count = sprintf(buf, "%u\n", instance->max_sectors_per_req);
> +
> +	return count+1;
> +}
> +
> +static struct bin_attribute sysfs_max_sectors_attr = {
> +	.attr = {
> +		.name = "max_sectors",
> +		.mode = S_IRUSR|S_IRGRP|S_IROTH,
> +		.owner = THIS_MODULE,
> +	},
> +	.size = 7,
> +	.read = sysfs_max_sectors_read,
> +};

Why is this implemented as a binary sysfs attribute? Also, can you use
the existing shost_attrs infrastructure that's in the scsi_host_template
like megaraid_mbox uses?


> +
> +	/*
> +	 * Check if the module parameter value for max_sectors can be used
> +	 */
> +	if (max_sectors && max_sectors <= instance->max_sectors_per_req)
> +		instance->max_sectors_per_req = max_sectors;
> +	else {
> +		if (max_sectors)
> +			printk(KERN_INFO
> +				"megasas: max_sectors should be > 0 and"
> +				"<= %d\n",
> +				instance->max_sectors_per_req);
> +	}

Could be simplified to an else if, which would remove one indent level...

-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

[PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[bo yang]

Adding module parameters to configure max sectors per request & # of cmds per lun.

Signed-off-by: Bo Yang <bo.yang@lsi.com>

---
 drivers/scsi/megaraid/megaraid_sas.c |   68 ++++++++++++++++++++++++-
 drivers/scsi/megaraid/megaraid_sas.h |    2
 2 files changed, 68 insertions(+), 2 deletions(-)

diff -uprN linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c
--- linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c	2007-10-01 00:14:29.000000000 -0700
+++ linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c	2007-10-01 02:15:16.000000000 -0700
@@ -62,6 +62,23 @@ MODULE_PARM_DESC(fast_load,
 	"megasas: Faster loading of the driver, skips physical devices! "\
 	"(default = 0)");
 
+/*
+ * Number of sectors per IO command will be set in megasas_init_mfi
+ * if user does not provide
+ */
+static unsigned int max_sectors;
+module_param_named(max_sectors, max_sectors, int, 0);
+MODULE_PARM_DESC(max_sectors,
+	"Maximum number of sectors per IO command");
+
+/*
+ * Number of cmds per logical unit
+ */
+static unsigned int cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
+module_param_named(cmd_per_lun, cmd_per_lun, int, 0);
+MODULE_PARM_DESC(cmd_per_lun,
+	"Maximum number of commands per logical unit (default=128)");
+
 MODULE_LICENSE("GPL");
 MODULE_VERSION(MEGASAS_VERSION);
 MODULE_AUTHOR("megaraidlinux@lsi.com");
@@ -1148,6 +1165,24 @@ static int megasas_slave_alloc(struct sc
 	return 0;
 }
 
+static ssize_t
+megasas_sysfs_max_sectors_read(struct class_device *cdev, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(cdev);
+	struct megasas_instance *instance =
+				(struct megasas_instance *)shost->hostdata;
+
+	return snprintf(buf, 8, "%u\n", instance->max_sectors_per_req);
+}
+
+CLASS_DEVICE_ATTR(max_sectors, S_IRUGO, megasas_sysfs_max_sectors_read,
+		NULL);
+
+static struct class_device_attribute *megasas_shost_attrs[] = {
+	&class_device_attr_max_sectors,
+	NULL,
+};
+
 /*
  * Scsi host template for megaraid_sas driver
  */
@@ -1165,6 +1200,7 @@ static struct scsi_host_template megasas
 	.eh_timed_out = megasas_reset_timer,
 	.bios_param = megasas_bios_param,
 	.use_clustering = ENABLE_CLUSTERING,
+	.shost_attrs = megasas_shost_attrs,
 };
 
 /**
@@ -2310,15 +2346,43 @@ static int megasas_io_attach(struct mega
 	struct Scsi_Host *host = instance->host;
 
 	/*
-	 * Export parameters required by SCSI mid-layer
+	 * Export host parameters required by SCSI
+	 * mid-layer
 	 */
 	host->irq = instance->pdev->irq;
 	host->unique_id = instance->unique_id;
 	host->can_queue = instance->max_fw_cmds - MEGASAS_INT_CMDS;
 	host->this_id = instance->init_id;
 	host->sg_tablesize = instance->max_num_sge;
+
+	/*
+	 * Check if the module parameter value for max_sectors can be used
+	 */
+	if (max_sectors && max_sectors <= instance->max_sectors_per_req)
+		instance->max_sectors_per_req = max_sectors;
+	else if (max_sectors) {
+		printk(KERN_INFO
+			"megasas: max_sectors should be > 0 and <= %d\n",
+			instance->max_sectors_per_req);
+	}
+
 	host->max_sectors = instance->max_sectors_per_req;
-	host->cmd_per_lun = 128;
+
+	/*
+	 * Check if the module parameter value for cmd_per_lun can be used
+	 */
+	instance->cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
+	if (cmd_per_lun && cmd_per_lun <= MEGASAS_DEFAULT_CMD_PER_LUN)
+		instance->cmd_per_lun = cmd_per_lun;
+	else
+		printk(KERN_INFO "megasas: cmd_per_lun should be > 0 and "
+			"<= %d\n", MEGASAS_DEFAULT_CMD_PER_LUN);
+
+	host->cmd_per_lun = instance->cmd_per_lun;
+
+	printk(KERN_DEBUG "megasas: max_sectors : 0x%x, cmd_per_lun : 0x%x\n",
+			instance->max_sectors_per_req, instance->cmd_per_lun);
+
 	host->max_channel = MEGASAS_MAX_CHANNELS - 1;
 	host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
 	host->max_lun = MEGASAS_MAX_LUN;
diff -uprN linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.h linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.h
--- linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.h	2007-10-01 00:03:59.000000000 -0700
+++ linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.h	2007-10-01 00:03:59.000000000 -0700
@@ -537,6 +537,7 @@ struct megasas_ctrl_info {
 #define MEGASAS_DEFAULT_INIT_ID			-1
 #define MEGASAS_MAX_LUN				8
 #define MEGASAS_MAX_LD				64
+#define MEGASAS_DEFAULT_CMD_PER_LUN		128
 
 #define MEGASAS_DBG_LVL				1
 
@@ -1080,6 +1081,7 @@ struct megasas_instance {
 	u16 max_num_sge;
 	u16 max_fw_cmds;
 	u32 max_sectors_per_req;
+	u32 cmd_per_lun;
 
 	struct megasas_cmd **cmd_list;
 	struct list_head cmd_pool;

Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Brian King]

Yang, Bo wrote:
> Brian,
>  
> Thanks for your review.  What is your objection to the current
> implementation by using bin_attribute?

Binary attributes are generally used for data which cannot
be parsed by the user. Since this attribute is simply a
single decimal value, it makes most sense to just use a regular
class_device_attribute. Additionally, if you use the
existing shost_attrs infrastructure in the scsi_host_template,
you can simplify your code, since you don't need to manually
create and destroy the sysfs files as scsi core will do this for
you.

-Brian

>  
> Bo Yang
> 
> ------------------------------------------------------------------------
> *From:* Brian King [mailto:brking@linux.vnet.ibm.com]
> *Sent:* Fri 9/28/2007 3:30 PM
> *To:* Yang, Bo
> *Cc:* linux-scsi@vger.kernel.org; James.Bottomley@SteelEye.com;
> akpm@osdl.org; linux-kernel@vger.kernel.org; Patro, Sumant
> *Subject:* Re: [PATCH 3/8] scsi: megaraid_sas - add module param
> max_sectors, cmd_per_lun
> 
> bo yang wrote:
> 
>> +static ssize_t
>> +sysfs_max_sectors_read(struct kobject *kobj,
>> +                     struct bin_attribute *bin_attr,
>> +                     char *buf, loff_t off, size_t count)
>> +{
>> +     struct Scsi_Host *host = class_to_shost(container_of(kobj,
>> +                                     struct class_device, kobj));
>> +     struct megasas_instance *instance =
>> +                             (struct megasas_instance *)host->hostdata;
>> +
>> +     count = sprintf(buf, "%u\n", instance->max_sectors_per_req);
>> +
>> +     return count+1;
>> +}
>> +
>> +static struct bin_attribute sysfs_max_sectors_attr = {
>> +     .attr = {
>> +             .name = "max_sectors",
>> +             .mode = S_IRUSR|S_IRGRP|S_IROTH,
>> +             .owner = THIS_MODULE,
>> +     },
>> +     .size = 7,
>> +     .read = sysfs_max_sectors_read,
>> +};
> 
> Why is this implemented as a binary sysfs attribute? Also, can you use
> the existing shost_attrs infrastructure that's in the scsi_host_template
> like megaraid_mbox uses?
> 
> 
>> +
>> +     /*
>> +      * Check if the module parameter value for max_sectors can be used
>> +      */
>> +     if (max_sectors && max_sectors <= instance->max_sectors_per_req)
>> +             instance->max_sectors_per_req = max_sectors;
>> +     else {
>> +             if (max_sectors)
>> +                     printk(KERN_INFO
>> +                             "megasas: max_sectors should be > 0 and"
>> +                             "<= %d\n",
>> +                             instance->max_sectors_per_req);
>> +     }
> 
> Could be simplified to an else if, which would remove one indent level...
> 
> -Brian
> 
> --
> Brian King
> Linux on Power Virtualization
> IBM Linux Technology Center
> 


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Randy Dunlap]

On Mon, 01 Oct 2007 11:51:48 -0400 bo yang wrote:

> Adding module parameters to configure max sectors per request & # of cmds per lun.
> 
> Signed-off-by: Bo Yang <bo.yang@lsi.com>
> 
> ---
>  drivers/scsi/megaraid/megaraid_sas.c |   68 ++++++++++++++++++++++++-
>  drivers/scsi/megaraid/megaraid_sas.h |    2
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff -uprN linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c
> --- linux-2.6.22_orig/drivers/scsi/megaraid/megaraid_sas.c	2007-10-01 00:14:29.000000000 -0700
> +++ linux-2.6.22_new/drivers/scsi/megaraid/megaraid_sas.c	2007-10-01 02:15:16.000000000 -0700
> @@ -62,6 +62,23 @@ MODULE_PARM_DESC(fast_load,
>  	"megasas: Faster loading of the driver, skips physical devices! "\
>  	"(default = 0)");
>  
> +/*
> + * Number of sectors per IO command will be set in megasas_init_mfi
> + * if user does not provide
> + */
> +static unsigned int max_sectors;
> +module_param_named(max_sectors, max_sectors, int, 0);
> +MODULE_PARM_DESC(max_sectors,
> +	"Maximum number of sectors per IO command");

Are you sure that you want these parameters hidden (permission = 0)
instead of readable via sysfs?

(same applies to the fast_load parameter patch also)


> +/*
> + * Number of cmds per logical unit
> + */
> +static unsigned int cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
> +module_param_named(cmd_per_lun, cmd_per_lun, int, 0);
> +MODULE_PARM_DESC(cmd_per_lun,
> +	"Maximum number of commands per logical unit (default=128)");
> +
>  MODULE_LICENSE("GPL");
>  MODULE_VERSION(MEGASAS_VERSION);
>  MODULE_AUTHOR("megaraidlinux@lsi.com");


---
~Randy

Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Christoph Hellwig]

On Mon, Oct 01, 2007 at 11:51:48AM -0400, bo yang wrote:
> +/*
> + * Number of sectors per IO command will be set in megasas_init_mfi
> + * if user does not provide
> + */
> +static unsigned int max_sectors;
> +module_param_named(max_sectors, max_sectors, int, 0);
> +MODULE_PARM_DESC(max_sectors,
> +	"Maximum number of sectors per IO command");
> +
> +/*
> + * Number of cmds per logical unit
> + */
> +static unsigned int cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN;
> +module_param_named(cmd_per_lun, cmd_per_lun, int, 0);
> +MODULE_PARM_DESC(cmd_per_lun,
> +	"Maximum number of commands per logical unit (default=128)");

I don't think this should be in drivers.  cmd_per_lun is a scsi_host
attribute already and you should change the mid-layer to add an
optional host template method that allows the sysfs file to be writeable.
max_sectors should be handled similar although we don't seem to have
an attribute for it yet (maybe the block layer does?)

RE: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Yang, Bo]

Re-Sending:

Christoph,

Here are the clarification for the design to implement the fast_load,
cmd_per_lun and max_sectors:

The fast_load parameter is for the user to decide at driver load time if
(s)he wants to skip scan of devices in PD channels. 
After driver is loaded the user cannot be permitted to modify this
value. If the user needs to see the devices in the PD channels, (s)he
may initiate a scan via sysfs/proc based on the kernel being used. Once
the user has done the scan, the fast_load value does not have any
significance and thus not exposed for reading.

cmd_per_lun & max_sectors are also intended to be provided by user only
at driver load time. In the current implementation both these do appear
as read-only values under host# in sysfs. The current design is not to
allow these values to be modified on the fly. 

Regards,

Bo Yang 

-----Original Message-----
From: Yang, Bo 
Sent: Wednesday, October 31, 2007 10:14 AM
To: Christoph Hellwig
Cc: linux-scsi@vger.kernel.org; James.Bottomley@SteelEye.com;
akpm@osdl.org; linux-kernel@vger.kernel.org; Patro, Sumant; Kolli, Neela
Subject: RE: [PATCH 3/8] scsi: megaraid_sas - add module param
max_sectors, cmd_per_lun


Christoph,

Here are the clarification for the design to implement the fast_load,
cmd_per_lun and max_sectors:

The fast_load parameter is for the user to decide at driver load time if
(s)he wants to skip scan of devices in PD channels. 
After driver is loaded the user cannot be permitted to modify this
value. If the user needs to see the devices in the PD channels, (s)he
may initiate a scan via sysfs/proc based on the kernel being used. Once
the user has done the scan, the fast_load value does not have any
significance and thus not exposed for reading.

cmd_per_lun & max_sectors are also intended to be provided by user only
at driver load time. In the current implementation both these do appear
as read-only values under host# in sysfs. The current design is not to
allow these values to be modified on the fly. 

Regards,

Bo Yang 

-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 
Sent: Tuesday, October 30, 2007 1:39 PM
To: Yang, Bo
Cc: linux-scsi@vger.kernel.org; James.Bottomley@SteelEye.com;
akpm@osdl.org; linux-kernel@vger.kernel.org; Patro, Sumant
Subject: Re: [PATCH 3/8] scsi: megaraid_sas - add module param
max_sectors, cmd_per_lun

On Mon, Oct 01, 2007 at 11:51:48AM -0400, bo yang wrote:
> +/*
> + * Number of sectors per IO command will be set in megasas_init_mfi
> + * if user does not provide
> + */
> +static unsigned int max_sectors;
> +module_param_named(max_sectors, max_sectors, int, 0); 
> +MODULE_PARM_DESC(max_sectors,
> +	"Maximum number of sectors per IO command");
> +
> +/*
> + * Number of cmds per logical unit
> + */
> +static unsigned int cmd_per_lun = MEGASAS_DEFAULT_CMD_PER_LUN; 
> +module_param_named(cmd_per_lun, cmd_per_lun, int, 0); 
> +MODULE_PARM_DESC(cmd_per_lun,
> +	"Maximum number of commands per logical unit (default=128)");

I don't think this should be in drivers.  cmd_per_lun is a scsi_host
attribute already and you should change the mid-layer to add an optional
host template method that allows the sysfs file to be writeable.
max_sectors should be handled similar although we don't seem to have an
attribute for it yet (maybe the block layer does?)

Re: [PATCH 3/8] scsi: megaraid_sas - add module param max_sectors, cmd_per_lun

[Christoph Hellwig]

On Tue, Nov 06, 2007 at 12:06:39PM -0700, Yang, Bo wrote:
> The fast_load parameter is for the user to decide at driver load time if
> (s)he wants to skip scan of devices in PD channels.
> After driver is loaded the user cannot be permitted to modify this
> value. If the user needs to see the devices in the PD channels, (s)he
> may initiate a scan via sysfs/proc based on the kernel being used. Once
> the user has done the scan, the fast_load value does not have any
> significance and thus not exposed for reading.

The issue here is that this should really be a per-hba setting, and
as HBAs can appear anytime due to PCI hotplug a module paramater is not
enough.  Then again I still don't see why we need to spend so much effort
on this as you could trivially just fail the PD scanning commands in the
firmware without messing up the driver.

> cmd_per_lun & max_sectors are also intended to be provided by user only
> at driver load time. In the current implementation both these do appear
> as read-only values under host# in sysfs. The current design is not to
> allow these values to be modified on the fly. 

Same argument here about beeing per-hba.  And they really should be changeable
at runtime at least for hbas that don't have commands in flight.