v4 udpates 3/3 description
max_lun can be less or equal to 32
v3 checks that max_lun is less or equal to 31
v2 of this series only fix the format type of max_lun:
drivers/scsi/ibmvscsi/ibmvscsi.c:2298:4:
warning: format '%d' expects argument of type 'int',
but argument 4 has type 'u64 {aka long long unsigned int}' [-Wformat=]
"Maximum ID: %d Maximum LUN: %d Maximum Channel: %d\n",
Laurent Vivier (3):
ibmvscsi: make parameters max_id and max_channel read-only
ibmvscsi: display default value for max_id, max_lun and max_channel.
ibmvscsi: Allow to configure maximum LUN
drivers/scsi/ibmvscsi/ibmvscsi.c | 21 ++++++++++++++++-----
drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
2 files changed, 17 insertions(+), 5 deletions(-)
--
2.1.0
The value of the parameter is never re-read by the driver,
so a new value is ignored. Let know the user he
can't modify it by removing writable attribute.
Signed-off-by: Laurent Vivier <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 6a41c36..3e76490 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -105,9 +105,9 @@ MODULE_AUTHOR("Dave Boutcher");
MODULE_LICENSE("GPL");
MODULE_VERSION(IBMVSCSI_VERSION);
-module_param_named(max_id, max_id, int, S_IRUGO | S_IWUSR);
+module_param_named(max_id, max_id, int, S_IRUGO);
MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
-module_param_named(max_channel, max_channel, int, S_IRUGO | S_IWUSR);
+module_param_named(max_channel, max_channel, int, S_IRUGO);
MODULE_PARM_DESC(max_channel, "Largest channel value");
module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
--
2.1.0
As devices with values greater than that are silently ignored,
this gives some hints to the sys admin to know why he doesn't see
his devices...
Signed-off-by: Laurent Vivier <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 3e76490..04de287 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -106,9 +106,9 @@ MODULE_LICENSE("GPL");
MODULE_VERSION(IBMVSCSI_VERSION);
module_param_named(max_id, max_id, int, S_IRUGO);
-MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
+MODULE_PARM_DESC(max_id, "Largest ID value for each channel [Default=64]");
module_param_named(max_channel, max_channel, int, S_IRUGO);
-MODULE_PARM_DESC(max_channel, "Largest channel value");
+MODULE_PARM_DESC(max_channel, "Largest channel value [Default=3]");
module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
module_param_named(max_requests, max_requests, int, S_IRUGO);
@@ -2294,6 +2294,10 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
host->max_channel = max_channel;
host->max_cmd_len = 16;
+ dev_info(dev,
+ "Maximum ID: %d Maximum LUN: %llu Maximum Channel: %d\n",
+ host->max_id, host->max_lun, host->max_channel);
+
if (scsi_add_host(hostdata->host, hostdata->dev))
goto add_host_failed;
--
2.1.0
This patch allows to define the maximum LUN numbers.
As defined in 4.6.9 of SAM-4, the encoding of LUN is
on 5 bits (max_lun=32) and the current value is only 8.
Signed-off-by: Laurent Vivier <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 9 ++++++++-
drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 04de287..adcd5e8 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -84,6 +84,7 @@
*/
static int max_id = 64;
static int max_channel = 3;
+static int max_lun = 8;
static int init_timeout = 300;
static int login_timeout = 60;
static int info_timeout = 30;
@@ -117,6 +118,9 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
module_param_named(client_reserve, client_reserve, int, S_IRUGO );
MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
+module_param(max_lun, int, S_IRUGO);
+MODULE_PARM_DESC(max_lun, "Maximum allowed LUN "
+ "[Default=8,Max="__stringify(IBMVSCSI_MAX_LUN)"]");
static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
struct ibmvscsi_host_data *hostdata);
@@ -2289,7 +2293,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
goto init_pool_failed;
}
- host->max_lun = 8;
+ host->max_lun = max_lun;
host->max_id = max_id;
host->max_channel = max_channel;
host->max_cmd_len = 16;
@@ -2414,6 +2418,9 @@ int __init ibmvscsi_module_init(void)
{
int ret;
+ if (max_lun > IBMVSCSI_MAX_LUN)
+ return -EINVAL;
+
/* Ensure we have two requests to do error recovery */
driver_template.can_queue = max_requests;
max_events = max_requests + 2;
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 7d64867..1067367 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -48,6 +48,7 @@ struct Scsi_Host;
#define IBMVSCSI_CMDS_PER_LUN_DEFAULT 16
#define IBMVSCSI_MAX_SECTORS_DEFAULT 256 /* 32 * 8 = default max I/O 32 pages */
#define IBMVSCSI_MAX_CMDS_PER_LUN 64
+#define IBMVSCSI_MAX_LUN 32
/* ------------------------------------------------------------
* Data Structures
--
2.1.0
On 11/09/2015 03:47 PM, Laurent Vivier wrote:
> This patch allows to define the maximum LUN numbers.
> As defined in 4.6.9 of SAM-4, the encoding of LUN is
> on 5 bits (max_lun=32) and the current value is only 8.
>
> Signed-off-by: Laurent Vivier <[email protected]>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 9 ++++++++-
> drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 04de287..adcd5e8 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -84,6 +84,7 @@
> */
> static int max_id = 64;
> static int max_channel = 3;
> +static int max_lun = 8;
> static int init_timeout = 300;
> static int login_timeout = 60;
> static int info_timeout = 30;
> @@ -117,6 +118,9 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
> +module_param(max_lun, int, S_IRUGO);
> +MODULE_PARM_DESC(max_lun, "Maximum allowed LUN "
> + "[Default=8,Max="__stringify(IBMVSCSI_MAX_LUN)"]");
>
> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
> struct ibmvscsi_host_data *hostdata);
> @@ -2289,7 +2293,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
> goto init_pool_failed;
> }
>
> - host->max_lun = 8;
> + host->max_lun = max_lun;
> host->max_id = max_id;
> host->max_channel = max_channel;
> host->max_cmd_len = 16;
> @@ -2414,6 +2418,9 @@ int __init ibmvscsi_module_init(void)
> {
> int ret;
>
> + if (max_lun > IBMVSCSI_MAX_LUN)
> + return -EINVAL;
> +
> /* Ensure we have two requests to do error recovery */
> driver_template.can_queue = max_requests;
> max_events = max_requests + 2;
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
> index 7d64867..1067367 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.h
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
> @@ -48,6 +48,7 @@ struct Scsi_Host;
> #define IBMVSCSI_CMDS_PER_LUN_DEFAULT 16
> #define IBMVSCSI_MAX_SECTORS_DEFAULT 256 /* 32 * 8 = default max I/O 32 pages */
> #define IBMVSCSI_MAX_CMDS_PER_LUN 64
> +#define IBMVSCSI_MAX_LUN 32
>
> /* ------------------------------------------------------------
> * Data Structures
>
Please set max_lun to 31, and remove everything else.
As discussed, max_lun is the max number of LUNs supported by the
hardware/HBA. There is no point in restricting it further.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)
On 09/11/2015 15:50, Hannes Reinecke wrote:
> On 11/09/2015 03:47 PM, Laurent Vivier wrote:
>> This patch allows to define the maximum LUN numbers.
>> As defined in 4.6.9 of SAM-4, the encoding of LUN is
>> on 5 bits (max_lun=32) and the current value is only 8.
>>
>> Signed-off-by: Laurent Vivier <[email protected]>
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 9 ++++++++-
>> drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 04de287..adcd5e8 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -84,6 +84,7 @@
>> */
>> static int max_id = 64;
>> static int max_channel = 3;
>> +static int max_lun = 8;
>> static int init_timeout = 300;
>> static int login_timeout = 60;
>> static int info_timeout = 30;
>> @@ -117,6 +118,9 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>> +module_param(max_lun, int, S_IRUGO);
>> +MODULE_PARM_DESC(max_lun, "Maximum allowed LUN "
>> + "[Default=8,Max="__stringify(IBMVSCSI_MAX_LUN)"]");
>>
>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>> struct ibmvscsi_host_data *hostdata);
>> @@ -2289,7 +2293,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>> goto init_pool_failed;
>> }
>>
>> - host->max_lun = 8;
>> + host->max_lun = max_lun;
>> host->max_id = max_id;
>> host->max_channel = max_channel;
>> host->max_cmd_len = 16;
>> @@ -2414,6 +2418,9 @@ int __init ibmvscsi_module_init(void)
>> {
>> int ret;
>>
>> + if (max_lun > IBMVSCSI_MAX_LUN)
>> + return -EINVAL;
>> +
>> /* Ensure we have two requests to do error recovery */
>> driver_template.can_queue = max_requests;
>> max_events = max_requests + 2;
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
>> index 7d64867..1067367 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.h
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
>> @@ -48,6 +48,7 @@ struct Scsi_Host;
>> #define IBMVSCSI_CMDS_PER_LUN_DEFAULT 16
>> #define IBMVSCSI_MAX_SECTORS_DEFAULT 256 /* 32 * 8 = default max I/O 32 pages */
>> #define IBMVSCSI_MAX_CMDS_PER_LUN 64
>> +#define IBMVSCSI_MAX_LUN 32
>>
>> /* ------------------------------------------------------------
>> * Data Structures
>>
> Please set max_lun to 31, and remove everything else.
> As discussed, max_lun is the max number of LUNs supported by the
> hardware/HBA. There is no point in restricting it further.
You mean no module parameter and a simple "host->max_lun = 31" ?
I'm wondering if it should be "host->max_lun = 32". What about this ?
include/scsi/scsi_host.h:
/*
* These three parameters can be used to allow for wide scsi,
* and for host adapters that support multiple busses
* The last two should be set to 1 more than the actual max id
* or lun (e.g. 8 for SCSI parallel systems).
*/
unsigned int max_channel;
unsigned int max_id;
u64 max_lun;
As ibmvscsi maximum LUN value is 31, host->max_lun should be 32.
Laurent
On 11/09/2015 03:59 PM, Laurent Vivier wrote:
>
>
> On 09/11/2015 15:50, Hannes Reinecke wrote:
>> On 11/09/2015 03:47 PM, Laurent Vivier wrote:
>>> This patch allows to define the maximum LUN numbers.
>>> As defined in 4.6.9 of SAM-4, the encoding of LUN is
>>> on 5 bits (max_lun=32) and the current value is only 8.
>>>
>>> Signed-off-by: Laurent Vivier <[email protected]>
>>> ---
>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 9 ++++++++-
>>> drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
>>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>> index 04de287..adcd5e8 100644
>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>>> @@ -84,6 +84,7 @@
>>> */
>>> static int max_id = 64;
>>> static int max_channel = 3;
>>> +static int max_lun = 8;
>>> static int init_timeout = 300;
>>> static int login_timeout = 60;
>>> static int info_timeout = 30;
>>> @@ -117,6 +118,9 @@ module_param_named(fast_fail, fast_fail, int, S_IRUGO | S_IWUSR);
>>> MODULE_PARM_DESC(fast_fail, "Enable fast fail. [Default=1]");
>>> module_param_named(client_reserve, client_reserve, int, S_IRUGO );
>>> MODULE_PARM_DESC(client_reserve, "Attempt client managed reserve/release");
>>> +module_param(max_lun, int, S_IRUGO);
>>> +MODULE_PARM_DESC(max_lun, "Maximum allowed LUN "
>>> + "[Default=8,Max="__stringify(IBMVSCSI_MAX_LUN)"]");
>>>
>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>>> struct ibmvscsi_host_data *hostdata);
>>> @@ -2289,7 +2293,7 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>>> goto init_pool_failed;
>>> }
>>>
>>> - host->max_lun = 8;
>>> + host->max_lun = max_lun;
>>> host->max_id = max_id;
>>> host->max_channel = max_channel;
>>> host->max_cmd_len = 16;
>>> @@ -2414,6 +2418,9 @@ int __init ibmvscsi_module_init(void)
>>> {
>>> int ret;
>>>
>>> + if (max_lun > IBMVSCSI_MAX_LUN)
>>> + return -EINVAL;
>>> +
>>> /* Ensure we have two requests to do error recovery */
>>> driver_template.can_queue = max_requests;
>>> max_events = max_requests + 2;
>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
>>> index 7d64867..1067367 100644
>>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.h
>>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
>>> @@ -48,6 +48,7 @@ struct Scsi_Host;
>>> #define IBMVSCSI_CMDS_PER_LUN_DEFAULT 16
>>> #define IBMVSCSI_MAX_SECTORS_DEFAULT 256 /* 32 * 8 = default max I/O 32 pages */
>>> #define IBMVSCSI_MAX_CMDS_PER_LUN 64
>>> +#define IBMVSCSI_MAX_LUN 32
>>>
>>> /* ------------------------------------------------------------
>>> * Data Structures
>>>
>> Please set max_lun to 31, and remove everything else.
>> As discussed, max_lun is the max number of LUNs supported by the
>> hardware/HBA. There is no point in restricting it further.
>
> You mean no module parameter and a simple "host->max_lun = 31" ?
>
Yes.
> I'm wondering if it should be "host->max_lun = 32". What about this ?
>
'max_lun' is used as the stopping condition while scanning LUNs
sequentially in drivers/scsi/scsi_scan.c:
for (lun = 1; lun < max_dev_lun; ++lun)
if ((scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan,
NULL) != SCSI_SCAN_LUN_PRESENT) &&
so indeed it needs to be set to '32'.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)
On Mon, 2015-11-09 at 15:47 +0100, Laurent Vivier wrote:
> The value of the parameter is never re-read by the driver,
> so a new value is ignored. Let know the user he
> can't modify it by removing writable attribute.
This isn't correct. They're read in every time a new SCSI host is
bound. I don't believe VIO is a hot plug bus, so the only way to get
the values to propagate is to unbind and rebind the driver. Now if you
want to argue they should be read only because users are getting
confused about how to propagate the values, that's a different story,
but this is a standard pattern in quite a few drivers, so you'd need to
argue why vscsi users are special.
James
> Signed-off-by: Laurent Vivier <[email protected]>
> ---
> drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 6a41c36..3e76490 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -105,9 +105,9 @@ MODULE_AUTHOR("Dave Boutcher");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(IBMVSCSI_VERSION);
>
> -module_param_named(max_id, max_id, int, S_IRUGO | S_IWUSR);
> +module_param_named(max_id, max_id, int, S_IRUGO);
> MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
> -module_param_named(max_channel, max_channel, int, S_IRUGO | S_IWUSR);
> +module_param_named(max_channel, max_channel, int, S_IRUGO);
> MODULE_PARM_DESC(max_channel, "Largest channel value");
> module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
On 09/11/2015 16:07, James Bottomley wrote:
> On Mon, 2015-11-09 at 15:47 +0100, Laurent Vivier wrote:
>> The value of the parameter is never re-read by the driver,
>> so a new value is ignored. Let know the user he
>> can't modify it by removing writable attribute.
>
> This isn't correct. They're read in every time a new SCSI host is
> bound. I don't believe VIO is a hot plug bus, so the only way to get
> the values to propagate is to unbind and rebind the driver. Now if you
> want to argue they should be read only because users are getting
> confused about how to propagate the values, that's a different story,
Yes, but the user was me and it is easy to confuse me...
> but this is a standard pattern in quite a few drivers, so you'd need to
> argue why vscsi users are special.
vscsi users are not special, I'll remove this patch from the series.
thanks,
Laurent
> James
>
>> Signed-off-by: Laurent Vivier <[email protected]>
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 6a41c36..3e76490 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -105,9 +105,9 @@ MODULE_AUTHOR("Dave Boutcher");
>> MODULE_LICENSE("GPL");
>> MODULE_VERSION(IBMVSCSI_VERSION);
>>
>> -module_param_named(max_id, max_id, int, S_IRUGO | S_IWUSR);
>> +module_param_named(max_id, max_id, int, S_IRUGO);
>> MODULE_PARM_DESC(max_id, "Largest ID value for each channel");
>> -module_param_named(max_channel, max_channel, int, S_IRUGO | S_IWUSR);
>> +module_param_named(max_channel, max_channel, int, S_IRUGO);
>> MODULE_PARM_DESC(max_channel, "Largest channel value");
>> module_param_named(init_timeout, init_timeout, int, S_IRUGO | S_IWUSR);
>> MODULE_PARM_DESC(init_timeout, "Initialization timeout in seconds");
>
>
>