* [PATCH v2 0/3] ibmvscsi parameter cleanup
@ 2015-11-04 10:20 Laurent Vivier
2015-11-04 10:20 ` [PATCH v2 1/3] ibmvscsi: make parameters max_id and max_channel read-only Laurent Vivier
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Laurent Vivier @ 2015-11-04 10:20 UTC (permalink / raw)
To: martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel, lvivier
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 | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/3] ibmvscsi: make parameters max_id and max_channel read-only 2015-11-04 10:20 [PATCH v2 0/3] ibmvscsi parameter cleanup Laurent Vivier @ 2015-11-04 10:20 ` Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 2/3] ibmvscsi: display default value for max_id, max_lun and max_channel Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN Laurent Vivier 2 siblings, 0 replies; 13+ messages in thread From: Laurent Vivier @ 2015-11-04 10:20 UTC (permalink / raw) To: martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel, lvivier 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 <lvivier@redhat.com> Reviewed-by: Brian King <brking@linux.vnet.ibm.com> Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> --- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] ibmvscsi: display default value for max_id, max_lun and max_channel. 2015-11-04 10:20 [PATCH v2 0/3] ibmvscsi parameter cleanup Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 1/3] ibmvscsi: make parameters max_id and max_channel read-only Laurent Vivier @ 2015-11-04 10:20 ` Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN Laurent Vivier 2 siblings, 0 replies; 13+ messages in thread From: Laurent Vivier @ 2015-11-04 10:20 UTC (permalink / raw) To: martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel, lvivier 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 <lvivier@redhat.com> Reviewed-by: Brian King <brking@linux.vnet.ibm.com> Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> --- v2: fix format of max_lun (u64): use %llu 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 10:20 [PATCH v2 0/3] ibmvscsi parameter cleanup Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 1/3] ibmvscsi: make parameters max_id and max_channel read-only Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 2/3] ibmvscsi: display default value for max_id, max_lun and max_channel Laurent Vivier @ 2015-11-04 10:20 ` Laurent Vivier 2015-11-04 10:24 ` Laurent Vivier 2015-11-04 11:16 ` Hannes Reinecke 2 siblings, 2 replies; 13+ messages in thread From: Laurent Vivier @ 2015-11-04 10:20 UTC (permalink / raw) To: martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel, lvivier QEMU allows until 32 LUNs. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); static void ibmvscsi_handle_crq(struct viosrp_crq *crq, struct ibmvscsi_host_data *hostdata); @@ -2289,7 +2292,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; -- 2.1.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 10:20 ` [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN Laurent Vivier @ 2015-11-04 10:24 ` Laurent Vivier 2015-11-04 11:16 ` Hannes Reinecke 1 sibling, 0 replies; 13+ messages in thread From: Laurent Vivier @ 2015-11-04 10:24 UTC (permalink / raw) To: martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel On 04/11/2015 11:20, Laurent Vivier wrote: > QEMU allows until 32 LUNs. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> I forget the: Reviewed-by: Brian King <brking@linux.vnet.ibm.com> Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); > > static void ibmvscsi_handle_crq(struct viosrp_crq *crq, > struct ibmvscsi_host_data *hostdata); > @@ -2289,7 +2292,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; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 10:20 ` [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN Laurent Vivier 2015-11-04 10:24 ` Laurent Vivier @ 2015-11-04 11:16 ` Hannes Reinecke 2015-11-04 11:46 ` Laurent Vivier 1 sibling, 1 reply; 13+ messages in thread From: Hannes Reinecke @ 2015-11-04 11:16 UTC (permalink / raw) To: Laurent Vivier, martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel On 11/04/2015 11:20 AM, Laurent Vivier wrote: > QEMU allows until 32 LUNs. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c > index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); > > static void ibmvscsi_handle_crq(struct viosrp_crq *crq, > struct ibmvscsi_host_data *hostdata); > @@ -2289,7 +2292,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; > Please, don't do this. 'max_lun' should only be set if the HBA / transport has some hard limitations on the number of bytes it can use. Otherwise the scanning algorithm in scsi_scan.c should do the correct thing, independent on the 'max_lun' setting. If qemu has some issues here someone should rather fix qemu ... Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +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) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 11:16 ` Hannes Reinecke @ 2015-11-04 11:46 ` Laurent Vivier 2015-11-04 13:02 ` Hannes Reinecke 0 siblings, 1 reply; 13+ messages in thread From: Laurent Vivier @ 2015-11-04 11:46 UTC (permalink / raw) To: Hannes Reinecke, martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel On 04/11/2015 12:16, Hannes Reinecke wrote: > On 11/04/2015 11:20 AM, Laurent Vivier wrote: >> QEMU allows until 32 LUNs. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >> >> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >> struct ibmvscsi_host_data *hostdata); >> @@ -2289,7 +2292,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; >> > Please, don't do this. > > 'max_lun' should only be set if the HBA / transport has some hard > limitations on the number of bytes it can use. > Otherwise the scanning algorithm in scsi_scan.c should do the > correct thing, independent on the 'max_lun' setting. So you are saying we can remove the line ? > If qemu has some issues here someone should rather fix qemu ... There is no issue with QEMU. QEMU can manage more than "8" LUNs, and we'd like to. Laurent ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 11:46 ` Laurent Vivier @ 2015-11-04 13:02 ` Hannes Reinecke 2015-11-04 23:06 ` Brian King 0 siblings, 1 reply; 13+ messages in thread From: Hannes Reinecke @ 2015-11-04 13:02 UTC (permalink / raw) To: Laurent Vivier, martin.petersen; +Cc: brking, tyreld, linux-scsi, linux-kernel On 11/04/2015 12:46 PM, Laurent Vivier wrote: > > > On 04/11/2015 12:16, Hannes Reinecke wrote: >> On 11/04/2015 11:20 AM, Laurent Vivier wrote: >>> QEMU allows until 32 LUNs. >>> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> --- >>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >>> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >>> >>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >>> struct ibmvscsi_host_data *hostdata); >>> @@ -2289,7 +2292,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; >>> >> Please, don't do this. >> >> 'max_lun' should only be set if the HBA / transport has some hard >> limitations on the number of bytes it can use. >> Otherwise the scanning algorithm in scsi_scan.c should do the >> correct thing, independent on the 'max_lun' setting. > > So you are saying we can remove the line ? > Ho-hum. In principle you could, as ibmvscsi is using SRP internally, which does support 64 bit LUNs. However, due to some weird design decisions there is the function 'lun_from_dev()', which mangles the incoming LUN number into something ... else. Which leaves only 4 bits free for the actual LUN number, requiring you to use max_lun = 16. Personally I would just do away with that function and use the incoming LUN numbers as is. Brian? Any comments? Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +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) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 13:02 ` Hannes Reinecke @ 2015-11-04 23:06 ` Brian King 2015-11-05 8:26 ` Laurent Vivier 2015-11-05 8:51 ` Hannes Reinecke 0 siblings, 2 replies; 13+ messages in thread From: Brian King @ 2015-11-04 23:06 UTC (permalink / raw) To: Hannes Reinecke, Laurent Vivier, martin.petersen Cc: tyreld, linux-scsi, linux-kernel On 11/04/2015 07:02 AM, Hannes Reinecke wrote: > On 11/04/2015 12:46 PM, Laurent Vivier wrote: >> >> >> On 04/11/2015 12:16, Hannes Reinecke wrote: >>> On 11/04/2015 11:20 AM, Laurent Vivier wrote: >>>> QEMU allows until 32 LUNs. >>>> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>> --- >>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >>>> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >>>> >>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >>>> struct ibmvscsi_host_data *hostdata); >>>> @@ -2289,7 +2292,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; >>>> >>> Please, don't do this. >>> >>> 'max_lun' should only be set if the HBA / transport has some hard >>> limitations on the number of bytes it can use. >>> Otherwise the scanning algorithm in scsi_scan.c should do the >>> correct thing, independent on the 'max_lun' setting. >> >> So you are saying we can remove the line ? >> > Ho-hum. In principle you could, as ibmvscsi is using SRP internally, > which does support 64 bit LUNs. > > However, due to some weird design decisions there is > the function 'lun_from_dev()', which mangles the incoming LUN number > into something ... else. > Which leaves only 4 bits free for the actual LUN number, requiring > you to use max_lun = 16. > > Personally I would just do away with that function and use the > incoming LUN numbers as is. I pulled out my copy of SAM, what lun_from_dev is doing is translating the incoming bus / id / LUN to a LUN in the logical unit addressing format, as defined in 4.6.9 of SAM-4. -------------------------------------- |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | -------------------------------------- | n | (10b) | Target | -------------------------------------- | n+1| Bus | LUN | -------------------------------------- So this means, in the current implementation, we have 6 bits for target (max=63), 3 bits for bus (max=7), and 5 bits for LUN (max=31). It might not be a bad idea to enforce these limits on the module parameters. Otherwise we'll have a mess when we run through lun_from_dev... As far as eliminating lun_from_dev and just passing the LUN through, I don't think we can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are doing the translation above. -Brian -- Brian King Power Linux I/O IBM Linux Technology Center ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 23:06 ` Brian King @ 2015-11-05 8:26 ` Laurent Vivier 2015-11-05 20:00 ` Brian King 2015-11-05 8:51 ` Hannes Reinecke 1 sibling, 1 reply; 13+ messages in thread From: Laurent Vivier @ 2015-11-05 8:26 UTC (permalink / raw) To: Brian King, Hannes Reinecke, martin.petersen Cc: tyreld, linux-scsi, linux-kernel On 05/11/2015 00:06, Brian King wrote: > On 11/04/2015 07:02 AM, Hannes Reinecke wrote: >> On 11/04/2015 12:46 PM, Laurent Vivier wrote: >>> >>> >>> On 04/11/2015 12:16, Hannes Reinecke wrote: >>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote: >>>>> QEMU allows until 32 LUNs. >>>>> >>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>>> --- >>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >>>>> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >>>>> >>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >>>>> struct ibmvscsi_host_data *hostdata); >>>>> @@ -2289,7 +2292,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; >>>>> >>>> Please, don't do this. >>>> >>>> 'max_lun' should only be set if the HBA / transport has some hard >>>> limitations on the number of bytes it can use. >>>> Otherwise the scanning algorithm in scsi_scan.c should do the >>>> correct thing, independent on the 'max_lun' setting. >>> >>> So you are saying we can remove the line ? >>> >> Ho-hum. In principle you could, as ibmvscsi is using SRP internally, >> which does support 64 bit LUNs. >> >> However, due to some weird design decisions there is >> the function 'lun_from_dev()', which mangles the incoming LUN number >> into something ... else. >> Which leaves only 4 bits free for the actual LUN number, requiring >> you to use max_lun = 16. >> >> Personally I would just do away with that function and use the >> incoming LUN numbers as is. > > I pulled out my copy of SAM, what lun_from_dev is doing is translating > the incoming bus / id / LUN to a LUN in the logical unit addressing format, > as defined in 4.6.9 of SAM-4. > > -------------------------------------- > |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | > -------------------------------------- > | n | (10b) | Target | > -------------------------------------- > | n+1| Bus | LUN | > -------------------------------------- > > So this means, in the current implementation, we have 6 bits for target (max=63), > 3 bits for bus (max=7), and 5 bits for LUN (max=31). > > It might not be a bad idea to enforce these limits on the module parameters. Otherwise > we'll have a mess when we run through lun_from_dev... > > As far as eliminating lun_from_dev and just passing the LUN through, I don't think we > can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are > doing the translation above. So what I understand is this patch is OK but I have to check the given value is less than 32 (the same max value as for QEMU) ? Laurent ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-05 8:26 ` Laurent Vivier @ 2015-11-05 20:00 ` Brian King 0 siblings, 0 replies; 13+ messages in thread From: Brian King @ 2015-11-05 20:00 UTC (permalink / raw) To: Laurent Vivier, Hannes Reinecke, martin.petersen Cc: tyreld, linux-scsi, linux-kernel On 11/05/2015 02:26 AM, Laurent Vivier wrote: > > > On 05/11/2015 00:06, Brian King wrote: >> On 11/04/2015 07:02 AM, Hannes Reinecke wrote: >>> On 11/04/2015 12:46 PM, Laurent Vivier wrote: >>>> >>>> >>>> On 04/11/2015 12:16, Hannes Reinecke wrote: >>>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote: >>>>>> QEMU allows until 32 LUNs. >>>>>> >>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>>>> --- >>>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >>>>>> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >>>>>> >>>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >>>>>> struct ibmvscsi_host_data *hostdata); >>>>>> @@ -2289,7 +2292,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; >>>>>> >>>>> Please, don't do this. >>>>> >>>>> 'max_lun' should only be set if the HBA / transport has some hard >>>>> limitations on the number of bytes it can use. >>>>> Otherwise the scanning algorithm in scsi_scan.c should do the >>>>> correct thing, independent on the 'max_lun' setting. >>>> >>>> So you are saying we can remove the line ? >>>> >>> Ho-hum. In principle you could, as ibmvscsi is using SRP internally, >>> which does support 64 bit LUNs. >>> >>> However, due to some weird design decisions there is >>> the function 'lun_from_dev()', which mangles the incoming LUN number >>> into something ... else. >>> Which leaves only 4 bits free for the actual LUN number, requiring >>> you to use max_lun = 16. >>> >>> Personally I would just do away with that function and use the >>> incoming LUN numbers as is. >> >> I pulled out my copy of SAM, what lun_from_dev is doing is translating >> the incoming bus / id / LUN to a LUN in the logical unit addressing format, >> as defined in 4.6.9 of SAM-4. >> >> -------------------------------------- >> |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | >> -------------------------------------- >> | n | (10b) | Target | >> -------------------------------------- >> | n+1| Bus | LUN | >> -------------------------------------- >> >> So this means, in the current implementation, we have 6 bits for target (max=63), >> 3 bits for bus (max=7), and 5 bits for LUN (max=31). >> >> It might not be a bad idea to enforce these limits on the module parameters. Otherwise >> we'll have a mess when we run through lun_from_dev... >> >> As far as eliminating lun_from_dev and just passing the LUN through, I don't think we >> can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are >> doing the translation above. > > So what I understand is this patch is OK but I have to check the given > value is less than 32 (the same max value as for QEMU) ? As far as I'm concerned, yes. Thanks, Brian -- Brian King Power Linux I/O IBM Linux Technology Center ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-04 23:06 ` Brian King 2015-11-05 8:26 ` Laurent Vivier @ 2015-11-05 8:51 ` Hannes Reinecke 2015-11-05 19:01 ` Brian King 1 sibling, 1 reply; 13+ messages in thread From: Hannes Reinecke @ 2015-11-05 8:51 UTC (permalink / raw) To: Brian King, Laurent Vivier, martin.petersen Cc: tyreld, linux-scsi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4168 bytes --] On 11/05/2015 12:06 AM, Brian King wrote: > On 11/04/2015 07:02 AM, Hannes Reinecke wrote: >> On 11/04/2015 12:46 PM, Laurent Vivier wrote: >>> >>> >>> On 04/11/2015 12:16, Hannes Reinecke wrote: >>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote: >>>>> QEMU allows until 32 LUNs. >>>>> >>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>>> --- >>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >>>>> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >>>>> >>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >>>>> struct ibmvscsi_host_data *hostdata); >>>>> @@ -2289,7 +2292,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; >>>>> >>>> Please, don't do this. >>>> >>>> 'max_lun' should only be set if the HBA / transport has some hard >>>> limitations on the number of bytes it can use. >>>> Otherwise the scanning algorithm in scsi_scan.c should do the >>>> correct thing, independent on the 'max_lun' setting. >>> >>> So you are saying we can remove the line ? >>> >> Ho-hum. In principle you could, as ibmvscsi is using SRP internally, >> which does support 64 bit LUNs. >> >> However, due to some weird design decisions there is >> the function 'lun_from_dev()', which mangles the incoming LUN number >> into something ... else. >> Which leaves only 4 bits free for the actual LUN number, requiring >> you to use max_lun = 16. >> >> Personally I would just do away with that function and use the >> incoming LUN numbers as is. > > I pulled out my copy of SAM, what lun_from_dev is doing is translating > the incoming bus / id / LUN to a LUN in the logical unit addressing format, > as defined in 4.6.9 of SAM-4. > > -------------------------------------- > |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | > -------------------------------------- > | n | (10b) | Target | > -------------------------------------- > | n+1| Bus | LUN | > -------------------------------------- > > So this means, in the current implementation, we have 6 bits for target (max=63), > 3 bits for bus (max=7), and 5 bits for LUN (max=31). > > It might not be a bad idea to enforce these limits on the module parameters. Otherwise > we'll have a mess when we run through lun_from_dev... > Correct. With the current implementation we need to set max_lun to 31. > As far as eliminating lun_from_dev and just passing the LUN through, I don't think we > can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are > doing the translation above. > ? How can that be ? REPORT LUNs will give you a list of available LUNs _from that target_. As the SCSI stack now moved to handle full 64bit LUNs the values from REPORT LUNs will be used as-is (well, byte-swapped, but still). Can you test with the attached patch? Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +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) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-ibmvscsi-Allow-64bit-LUNs.patch --] [-- Type: text/x-patch; name="0001-ibmvscsi-Allow-64bit-LUNs.patch", Size: 4209 bytes --] From 42d920e9ff2a4739d4a901e033a93364586c840c Mon Sep 17 00:00:00 2001 From: Hannes Reinecke <hare@suse.de> Date: Thu, 5 Nov 2015 09:50:23 +0100 Subject: [PATCH] ibmvscsi: Allow 64bit LUNs The SRP protocol allows full 64bit LUNs, so there is no need to mangle the LUNs in the driver. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/ibmvscsi/ibmvscsi.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index 6a41c36..0162b1d 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -1019,16 +1019,6 @@ static void handle_cmd_rsp(struct srp_event_struct *evt_struct) } /** - * lun_from_dev: - Returns the lun of the scsi device - * @dev: struct scsi_device - * -*/ -static inline u16 lun_from_dev(struct scsi_device *dev) -{ - return (0x2 << 14) | (dev->id << 8) | (dev->channel << 5) | dev->lun; -} - -/** * ibmvscsi_queue: - The queuecommand function of the scsi template * @cmd: struct scsi_cmnd to be executed * @done: Callback function to be called when cmd is completed @@ -1040,7 +1030,6 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, struct srp_event_struct *evt_struct; struct srp_indirect_buf *indirect; struct ibmvscsi_host_data *hostdata = shost_priv(cmnd->device->host); - u16 lun = lun_from_dev(cmnd->device); u8 out_fmt, in_fmt; cmnd->result = (DID_OK << 16); @@ -1053,7 +1042,7 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd, memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); srp_cmd->opcode = SRP_CMD; memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); - int_to_scsilun(lun, &srp_cmd->lun); + int_to_scsilun(cmnd->device->lun, &srp_cmd->lun); if (!map_data_for_srp_cmd(cmnd, evt_struct, srp_cmd, hostdata->dev)) { if (!firmware_has_feature(FW_FEATURE_CMO)) @@ -1489,7 +1478,6 @@ static int ibmvscsi_eh_abort_handler(struct scsi_cmnd *cmd) union viosrp_iu srp_rsp; int rsp_rc; unsigned long flags; - u16 lun = lun_from_dev(cmd->device); unsigned long wait_switch = 0; /* First, find this command in our sent list so we can figure @@ -1529,7 +1517,7 @@ static int ibmvscsi_eh_abort_handler(struct scsi_cmnd *cmd) /* Set up an abort SRP command */ memset(tsk_mgmt, 0x00, sizeof(*tsk_mgmt)); tsk_mgmt->opcode = SRP_TSK_MGMT; - int_to_scsilun(lun, &tsk_mgmt->lun); + int_to_scsilun(cmnd->device->lun, &tsk_mgmt->lun); tsk_mgmt->tsk_mgmt_func = SRP_TSK_ABORT_TASK; tsk_mgmt->task_tag = (u64) found_evt; @@ -1556,7 +1544,7 @@ static int ibmvscsi_eh_abort_handler(struct scsi_cmnd *cmd) sdev_printk(KERN_INFO, cmd->device, "aborting command. lun 0x%llx, tag 0x%llx\n", - (((u64) lun) << 48), (u64) found_evt); + cmd->device->lun, (u64) found_evt); wait_for_completion(&evt->comp); @@ -1628,7 +1616,6 @@ static int ibmvscsi_eh_device_reset_handler(struct scsi_cmnd *cmd) union viosrp_iu srp_rsp; int rsp_rc; unsigned long flags; - u16 lun = lun_from_dev(cmd->device); unsigned long wait_switch = 0; spin_lock_irqsave(hostdata->host->host_lock, flags); @@ -1652,7 +1639,7 @@ static int ibmvscsi_eh_device_reset_handler(struct scsi_cmnd *cmd) /* Set up a lun reset SRP command */ memset(tsk_mgmt, 0x00, sizeof(*tsk_mgmt)); tsk_mgmt->opcode = SRP_TSK_MGMT; - int_to_scsilun(lun, &tsk_mgmt->lun); + int_to_scsilun(cmnd->device->lun, &tsk_mgmt->lun); tsk_mgmt->tsk_mgmt_func = SRP_TSK_LUN_RESET; evt->sync_srp = &srp_rsp; @@ -1677,7 +1664,7 @@ static int ibmvscsi_eh_device_reset_handler(struct scsi_cmnd *cmd) } sdev_printk(KERN_INFO, cmd->device, "resetting device. lun 0x%llx\n", - (((u64) lun) << 48)); + cmnd->device->lun); wait_for_completion(&evt->comp); @@ -2289,7 +2276,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 = 0xFFFF; host->max_id = max_id; host->max_channel = max_channel; host->max_cmd_len = 16; -- 1.8.5.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN 2015-11-05 8:51 ` Hannes Reinecke @ 2015-11-05 19:01 ` Brian King 0 siblings, 0 replies; 13+ messages in thread From: Brian King @ 2015-11-05 19:01 UTC (permalink / raw) To: Hannes Reinecke, Laurent Vivier, martin.petersen Cc: tyreld, linux-scsi, linux-kernel On 11/05/2015 02:51 AM, Hannes Reinecke wrote: > On 11/05/2015 12:06 AM, Brian King wrote: >> On 11/04/2015 07:02 AM, Hannes Reinecke wrote: >>> On 11/04/2015 12:46 PM, Laurent Vivier wrote: >>>> >>>> >>>> On 04/11/2015 12:16, Hannes Reinecke wrote: >>>>> On 11/04/2015 11:20 AM, Laurent Vivier wrote: >>>>>> QEMU allows until 32 LUNs. >>>>>> >>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>>>>> --- >>>>>> drivers/scsi/ibmvscsi/ibmvscsi.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c >>>>>> index 04de287..4480d3e 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,8 @@ 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 LUN value [Default=8]"); >>>>>> >>>>>> static void ibmvscsi_handle_crq(struct viosrp_crq *crq, >>>>>> struct ibmvscsi_host_data *hostdata); >>>>>> @@ -2289,7 +2292,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; >>>>>> >>>>> Please, don't do this. >>>>> >>>>> 'max_lun' should only be set if the HBA / transport has some hard >>>>> limitations on the number of bytes it can use. >>>>> Otherwise the scanning algorithm in scsi_scan.c should do the >>>>> correct thing, independent on the 'max_lun' setting. >>>> >>>> So you are saying we can remove the line ? >>>> >>> Ho-hum. In principle you could, as ibmvscsi is using SRP internally, >>> which does support 64 bit LUNs. >>> >>> However, due to some weird design decisions there is >>> the function 'lun_from_dev()', which mangles the incoming LUN number >>> into something ... else. >>> Which leaves only 4 bits free for the actual LUN number, requiring >>> you to use max_lun = 16. >>> >>> Personally I would just do away with that function and use the >>> incoming LUN numbers as is. >> >> I pulled out my copy of SAM, what lun_from_dev is doing is translating >> the incoming bus / id / LUN to a LUN in the logical unit addressing format, >> as defined in 4.6.9 of SAM-4. >> >> -------------------------------------- >> |Bit | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | >> -------------------------------------- >> | n | (10b) | Target | >> -------------------------------------- >> | n+1| Bus | LUN | >> -------------------------------------- >> >> So this means, in the current implementation, we have 6 bits for target (max=63), >> 3 bits for bus (max=7), and 5 bits for LUN (max=31). >> >> It might not be a bad idea to enforce these limits on the module parameters. Otherwise >> we'll have a mess when we run through lun_from_dev... >> > Correct. With the current implementation we need to set max_lun to 31. > >> As far as eliminating lun_from_dev and just passing the LUN through, I don't think we >> can do that. The LUN list returned by Report LUNs to a VSCSI disk assumes we are >> doing the translation above. >> > ? How can that be ? I was just going by what sg_luns returned on a VSCSI disk: [root@powerio-blue-lp6 ~]# lsscsi [0:0:1:0] disk AIX VDASD 0001 /dev/sda [0:0:2:0] cd/dvd AIX VOPTA /dev/sr0 [0:0:3:0] disk AIX VDASD 0001 /dev/sdb [0:0:4:0] disk AIX VDASD 0001 /dev/sdc [0:0:7:0] cd/dvd AIX VOPTA /dev/sr1 [root@powerio-blue-lp6 ~]# sg_luns /dev/sda Lun list length = 8 which imples 1 lun entry Report luns [select_report=0x0]: 0000000000000000 [root@powerio-blue-lp6 ~]# sg_luns /dev/sdb Lun list length = 8 which imples 1 lun entry Report luns [select_report=0x0]: 0000000000000000 This is without your patch, but I don't think there should be any filtering of report luns even in this case. Its what the VIOS is returning that is at issue here. Thanks, Brian -- Brian King Power Linux I/O IBM Linux Technology Center ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-11-05 20:00 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-04 10:20 [PATCH v2 0/3] ibmvscsi parameter cleanup Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 1/3] ibmvscsi: make parameters max_id and max_channel read-only Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 2/3] ibmvscsi: display default value for max_id, max_lun and max_channel Laurent Vivier 2015-11-04 10:20 ` [PATCH v2 3/3] ibmvscsi: Allow to configure maximum LUN Laurent Vivier 2015-11-04 10:24 ` Laurent Vivier 2015-11-04 11:16 ` Hannes Reinecke 2015-11-04 11:46 ` Laurent Vivier 2015-11-04 13:02 ` Hannes Reinecke 2015-11-04 23:06 ` Brian King 2015-11-05 8:26 ` Laurent Vivier 2015-11-05 20:00 ` Brian King 2015-11-05 8:51 ` Hannes Reinecke 2015-11-05 19:01 ` Brian King
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).