[PATCH] ibmvfc: Add max_sectors module parameter

[PATCH] ibmvfc: Add max_sectors module parameter

[Brian King]

There are some scenarios that can occur, such as performing an
upgrade of the virtual I/O server, where the supported max transfer
of the backing device for an ibmvfc HBA can change. If the max
transfer of the backing device decreases, this can cause issues with
previously discovered LUNs. This patch accomplishes two things.
First, it changes the default ibmvfc max transfer value to 1MB.
This is generally supported by all backing devices, which should
mitigate this issue out of the box. Secondly, it adds a module
parameter, enabling a user to increase the max transfer value to
values that are larger than 1MB, as long as they have configured
these larger values on the virtual I/O server as well.

Signed-off-by: Brian King <brking@linux.ibm.com>
---
 drivers/scsi/ibmvscsi/ibmvfc.c | 10 +++++++---
 drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index a3d1013c8307..611901562e06 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -37,6 +37,7 @@ static unsigned int default_timeout = IBMVFC_DEFAULT_TIMEOUT;
 static u64 max_lun = IBMVFC_MAX_LUN;
 static unsigned int max_targets = IBMVFC_MAX_TARGETS;
 static unsigned int max_requests = IBMVFC_MAX_REQUESTS_DEFAULT;
+static unsigned int max_sectors = IBMVFC_MAX_SECTORS;
 static u16 scsi_qdepth = IBMVFC_SCSI_QDEPTH;
 static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;
 static unsigned int ibmvfc_debug = IBMVFC_DEBUG;
@@ -83,6 +84,9 @@ MODULE_PARM_DESC(default_timeout,
 module_param_named(max_requests, max_requests, uint, S_IRUGO);
 MODULE_PARM_DESC(max_requests, "Maximum requests for this adapter. "
 		 "[Default=" __stringify(IBMVFC_MAX_REQUESTS_DEFAULT) "]");
+module_param_named(max_sectors, max_sectors, uint, S_IRUGO);
+MODULE_PARM_DESC(max_sectors, "Maximum sectors for this adapter. "
+		 "[Default=" __stringify(IBMVFC_MAX_SECTORS) "]");
 module_param_named(scsi_qdepth, scsi_qdepth, ushort, S_IRUGO);
 MODULE_PARM_DESC(scsi_qdepth, "Maximum scsi command depth per adapter queue. "
 		 "[Default=" __stringify(IBMVFC_SCSI_QDEPTH) "]");
@@ -1494,7 +1498,7 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
 	memset(login_info, 0, sizeof(*login_info));
 
 	login_info->ostype = cpu_to_be32(IBMVFC_OS_LINUX);
-	login_info->max_dma_len = cpu_to_be64(IBMVFC_MAX_SECTORS << 9);
+	login_info->max_dma_len = cpu_to_be64(max_sectors << 9);
 	login_info->max_payload = cpu_to_be32(sizeof(struct ibmvfc_fcp_cmd_iu));
 	login_info->max_response = cpu_to_be32(sizeof(struct ibmvfc_fcp_rsp));
 	login_info->partition_num = cpu_to_be32(vhost->partition_number);
@@ -5230,7 +5234,7 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event *evt)
 	}
 
 	vhost->logged_in = 1;
-	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len) >> 9), IBMVFC_MAX_SECTORS);
+	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len) >> 9), max_sectors);
 	dev_info(vhost->dev, "Host partition: %s, device: %s %s %s max sectors %u\n",
 		 rsp->partition_name, rsp->device_name, rsp->port_loc_code,
 		 rsp->drc_name, npiv_max_sectors);
@@ -6329,7 +6333,7 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 	shost->can_queue = scsi_qdepth;
 	shost->max_lun = max_lun;
 	shost->max_id = max_targets;
-	shost->max_sectors = IBMVFC_MAX_SECTORS;
+	shost->max_sectors = max_sectors;
 	shost->max_cmd_len = IBMVFC_MAX_CDB_LEN;
 	shost->unique_id = shost->host_no;
 	shost->nr_hw_queues = mq_enabled ? min(max_scsi_queues, nr_scsi_hw_queues) : 1;
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 745ad5ac7251..c73ed2314ad0 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -32,7 +32,7 @@
 #define IBMVFC_DEBUG			0
 #define IBMVFC_MAX_TARGETS		1024
 #define IBMVFC_MAX_LUN			0xffffffff
-#define IBMVFC_MAX_SECTORS		0xffffu
+#define IBMVFC_MAX_SECTORS		2048
 #define IBMVFC_MAX_DISC_THREADS	4
 #define IBMVFC_TGT_MEMPOOL_SZ		64
 #define IBMVFC_MAX_CMDS_PER_LUN	64
-- 
2.43.5

Re: [PATCH] ibmvfc: Add max_sectors module parameter

[Martin Wilck]

On Tue, 2024-07-30 at 12:51 -0500, Brian King wrote:
> There are some scenarios that can occur, such as performing an
> upgrade of the virtual I/O server, where the supported max transfer
> of the backing device for an ibmvfc HBA can change. If the max
> transfer of the backing device decreases, this can cause issues with
> previously discovered LUNs. This patch accomplishes two things.
> First, it changes the default ibmvfc max transfer value to 1MB.
> This is generally supported by all backing devices, which should
> mitigate this issue out of the box. Secondly, it adds a module
> parameter, enabling a user to increase the max transfer value to
> values that are larger than 1MB, as long as they have configured
> these larger values on the virtual I/O server as well.
> 
> Signed-off-by: Brian King <brking@linux.ibm.com>
> ---
>  drivers/scsi/ibmvscsi/ibmvfc.c | 10 +++++++---
>  drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c
> b/drivers/scsi/ibmvscsi/ibmvfc.c
> index a3d1013c8307..611901562e06 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -37,6 +37,7 @@ static unsigned int default_timeout =
> IBMVFC_DEFAULT_TIMEOUT;
>  static u64 max_lun = IBMVFC_MAX_LUN;
>  static unsigned int max_targets = IBMVFC_MAX_TARGETS;
>  static unsigned int max_requests = IBMVFC_MAX_REQUESTS_DEFAULT;
> +static unsigned int max_sectors = IBMVFC_MAX_SECTORS;
>  static u16 scsi_qdepth = IBMVFC_SCSI_QDEPTH;
>  static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;
>  static unsigned int ibmvfc_debug = IBMVFC_DEBUG;
> @@ -83,6 +84,9 @@ MODULE_PARM_DESC(default_timeout,
>  module_param_named(max_requests, max_requests, uint, S_IRUGO);
>  MODULE_PARM_DESC(max_requests, "Maximum requests for this adapter. "
>  		 "[Default="
> __stringify(IBMVFC_MAX_REQUESTS_DEFAULT) "]");
> +module_param_named(max_sectors, max_sectors, uint, S_IRUGO);
> +MODULE_PARM_DESC(max_sectors, "Maximum sectors for this adapter. "
> +		 "[Default=" __stringify(IBMVFC_MAX_SECTORS) "]");
>  module_param_named(scsi_qdepth, scsi_qdepth, ushort, S_IRUGO);
>  MODULE_PARM_DESC(scsi_qdepth, "Maximum scsi command depth per
> adapter queue. "
>  		 "[Default=" __stringify(IBMVFC_SCSI_QDEPTH) "]");
> @@ -1494,7 +1498,7 @@ static void ibmvfc_set_login_info(struct
> ibmvfc_host *vhost)
>  	memset(login_info, 0, sizeof(*login_info));
>  
>  	login_info->ostype = cpu_to_be32(IBMVFC_OS_LINUX);
> -	login_info->max_dma_len = cpu_to_be64(IBMVFC_MAX_SECTORS <<
> 9);
> +	login_info->max_dma_len = cpu_to_be64(max_sectors << 9);
>  	login_info->max_payload = cpu_to_be32(sizeof(struct
> ibmvfc_fcp_cmd_iu));
>  	login_info->max_response = cpu_to_be32(sizeof(struct
> ibmvfc_fcp_rsp));
>  	login_info->partition_num = cpu_to_be32(vhost-
> >partition_number);
> @@ -5230,7 +5234,7 @@ static void ibmvfc_npiv_login_done(struct
> ibmvfc_event *evt)
>  	}
>  
>  	vhost->logged_in = 1;
> -	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len)
> >> 9), IBMVFC_MAX_SECTORS);
> +	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len)
> >> 9), max_sectors);
>  	dev_info(vhost->dev, "Host partition: %s, device: %s %s %s
> max sectors %u\n",
>  		 rsp->partition_name, rsp->device_name, rsp-
> >port_loc_code,
>  		 rsp->drc_name, npiv_max_sectors);
> @@ -6329,7 +6333,7 @@ static int ibmvfc_probe(struct vio_dev *vdev,
> const struct vio_device_id *id)
>  	shost->can_queue = scsi_qdepth;
>  	shost->max_lun = max_lun;
>  	shost->max_id = max_targets;
> -	shost->max_sectors = IBMVFC_MAX_SECTORS;
> +	shost->max_sectors = max_sectors;

Would it make sense to check whether the user-provided max_sectors
value is within some reasonable limits?

Thanks
Martin

Re: [PATCH] ibmvfc: Add max_sectors module parameter

[Brian King]

On 8/12/24 3:22 PM, Martin Wilck wrote:
> On Tue, 2024-07-30 at 12:51 -0500, Brian King wrote:
>> There are some scenarios that can occur, such as performing an
>> upgrade of the virtual I/O server, where the supported max transfer
>> of the backing device for an ibmvfc HBA can change. If the max
>> transfer of the backing device decreases, this can cause issues with
>> previously discovered LUNs. This patch accomplishes two things.
>> First, it changes the default ibmvfc max transfer value to 1MB.
>> This is generally supported by all backing devices, which should
>> mitigate this issue out of the box. Secondly, it adds a module
>> parameter, enabling a user to increase the max transfer value to
>> values that are larger than 1MB, as long as they have configured
>> these larger values on the virtual I/O server as well.
>>
>> Signed-off-by: Brian King <brking@linux.ibm.com>
>> ---
>>  drivers/scsi/ibmvscsi/ibmvfc.c | 10 +++++++---
>>  drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c
>> b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index a3d1013c8307..611901562e06 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -37,6 +37,7 @@ static unsigned int default_timeout =
>> IBMVFC_DEFAULT_TIMEOUT;
>>  static u64 max_lun = IBMVFC_MAX_LUN;
>>  static unsigned int max_targets = IBMVFC_MAX_TARGETS;
>>  static unsigned int max_requests = IBMVFC_MAX_REQUESTS_DEFAULT;
>> +static unsigned int max_sectors = IBMVFC_MAX_SECTORS;
>>  static u16 scsi_qdepth = IBMVFC_SCSI_QDEPTH;
>>  static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;
>>  static unsigned int ibmvfc_debug = IBMVFC_DEBUG;
>> @@ -83,6 +84,9 @@ MODULE_PARM_DESC(default_timeout,
>>  module_param_named(max_requests, max_requests, uint, S_IRUGO);
>>  MODULE_PARM_DESC(max_requests, "Maximum requests for this adapter. "
>>  		 "[Default="
>> __stringify(IBMVFC_MAX_REQUESTS_DEFAULT) "]");
>> +module_param_named(max_sectors, max_sectors, uint, S_IRUGO);
>> +MODULE_PARM_DESC(max_sectors, "Maximum sectors for this adapter. "
>> +		 "[Default=" __stringify(IBMVFC_MAX_SECTORS) "]");
>>  module_param_named(scsi_qdepth, scsi_qdepth, ushort, S_IRUGO);
>>  MODULE_PARM_DESC(scsi_qdepth, "Maximum scsi command depth per
>> adapter queue. "
>>  		 "[Default=" __stringify(IBMVFC_SCSI_QDEPTH) "]");
>> @@ -1494,7 +1498,7 @@ static void ibmvfc_set_login_info(struct
>> ibmvfc_host *vhost)
>>  	memset(login_info, 0, sizeof(*login_info));
>>  
>>  	login_info->ostype = cpu_to_be32(IBMVFC_OS_LINUX);
>> -	login_info->max_dma_len = cpu_to_be64(IBMVFC_MAX_SECTORS <<
>> 9);
>> +	login_info->max_dma_len = cpu_to_be64(max_sectors << 9);
>>  	login_info->max_payload = cpu_to_be32(sizeof(struct
>> ibmvfc_fcp_cmd_iu));
>>  	login_info->max_response = cpu_to_be32(sizeof(struct
>> ibmvfc_fcp_rsp));
>>  	login_info->partition_num = cpu_to_be32(vhost-
>>> partition_number);
>> @@ -5230,7 +5234,7 @@ static void ibmvfc_npiv_login_done(struct
>> ibmvfc_event *evt)
>>  	}
>>  
>>  	vhost->logged_in = 1;
>> -	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len)
>>>> 9), IBMVFC_MAX_SECTORS);
>> +	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len)
>>>> 9), max_sectors);
>>  	dev_info(vhost->dev, "Host partition: %s, device: %s %s %s
>> max sectors %u\n",
>>  		 rsp->partition_name, rsp->device_name, rsp-
>>> port_loc_code,
>>  		 rsp->drc_name, npiv_max_sectors);
>> @@ -6329,7 +6333,7 @@ static int ibmvfc_probe(struct vio_dev *vdev,
>> const struct vio_device_id *id)
>>  	shost->can_queue = scsi_qdepth;
>>  	shost->max_lun = max_lun;
>>  	shost->max_id = max_targets;
>> -	shost->max_sectors = IBMVFC_MAX_SECTORS;
>> +	shost->max_sectors = max_sectors;
> 
> Would it make sense to check whether the user-provided max_sectors
> value is within some reasonable limits?

Agreed.  I'll follow up with an updated version.

Thanks,

Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCH v2] ibmvfc: Add max_sectors module parameter

[Brian King]

There are some scenarios that can occur, such as performing an
upgrade of the virtual I/O server, where the supported max transfer
of the backing device for an ibmvfc HBA can change. If the max
transfer of the backing device decreases, this can cause issues with
previously discovered LUNs. This patch accomplishes two things.
First, it changes the default ibmvfc max transfer value to 1MB.
This is generally supported by all backing devices, which should
mitigate this issue out of the box. Secondly, it adds a module
parameter, enabling a user to increase the max transfer value to
values that are larger than 1MB, as long as they have configured
these larger values on the virtual I/O server as well.

Signed-off-by: Brian King <brking@linux.ibm.com>
---
 drivers/scsi/ibmvscsi/ibmvfc.c | 17 ++++++++++++++---
 drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index a3d1013c8307..3349d321aa07 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -37,6 +37,7 @@ static unsigned int default_timeout = IBMVFC_DEFAULT_TIMEOUT;
 static u64 max_lun = IBMVFC_MAX_LUN;
 static unsigned int max_targets = IBMVFC_MAX_TARGETS;
 static unsigned int max_requests = IBMVFC_MAX_REQUESTS_DEFAULT;
+static u16 max_sectors = IBMVFC_MAX_SECTORS;
 static u16 scsi_qdepth = IBMVFC_SCSI_QDEPTH;
 static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;
 static unsigned int ibmvfc_debug = IBMVFC_DEBUG;
@@ -83,6 +84,9 @@ MODULE_PARM_DESC(default_timeout,
 module_param_named(max_requests, max_requests, uint, S_IRUGO);
 MODULE_PARM_DESC(max_requests, "Maximum requests for this adapter. "
 		 "[Default=" __stringify(IBMVFC_MAX_REQUESTS_DEFAULT) "]");
+module_param_named(max_sectors, max_sectors, ushort, S_IRUGO);
+MODULE_PARM_DESC(max_sectors, "Maximum sectors for this adapter. "
+		 "[Default=" __stringify(IBMVFC_MAX_SECTORS) "]");
 module_param_named(scsi_qdepth, scsi_qdepth, ushort, S_IRUGO);
 MODULE_PARM_DESC(scsi_qdepth, "Maximum scsi command depth per adapter queue. "
 		 "[Default=" __stringify(IBMVFC_SCSI_QDEPTH) "]");
@@ -1494,7 +1498,7 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
 	memset(login_info, 0, sizeof(*login_info));
 
 	login_info->ostype = cpu_to_be32(IBMVFC_OS_LINUX);
-	login_info->max_dma_len = cpu_to_be64(IBMVFC_MAX_SECTORS << 9);
+	login_info->max_dma_len = cpu_to_be64(max_sectors << 9);
 	login_info->max_payload = cpu_to_be32(sizeof(struct ibmvfc_fcp_cmd_iu));
 	login_info->max_response = cpu_to_be32(sizeof(struct ibmvfc_fcp_rsp));
 	login_info->partition_num = cpu_to_be32(vhost->partition_number);
@@ -5230,7 +5234,7 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event *evt)
 	}
 
 	vhost->logged_in = 1;
-	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len) >> 9), IBMVFC_MAX_SECTORS);
+	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len) >> 9), max_sectors);
 	dev_info(vhost->dev, "Host partition: %s, device: %s %s %s max sectors %u\n",
 		 rsp->partition_name, rsp->device_name, rsp->port_loc_code,
 		 rsp->drc_name, npiv_max_sectors);
@@ -6329,7 +6333,7 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 	shost->can_queue = scsi_qdepth;
 	shost->max_lun = max_lun;
 	shost->max_id = max_targets;
-	shost->max_sectors = IBMVFC_MAX_SECTORS;
+	shost->max_sectors = max_sectors;
 	shost->max_cmd_len = IBMVFC_MAX_CDB_LEN;
 	shost->unique_id = shost->host_no;
 	shost->nr_hw_queues = mq_enabled ? min(max_scsi_queues, nr_scsi_hw_queues) : 1;
@@ -6556,6 +6560,7 @@ static struct fc_function_template ibmvfc_transport_functions = {
  **/
 static int __init ibmvfc_module_init(void)
 {
+	int min_max_sectors = PAGE_SIZE >> 9;
 	int rc;
 
 	if (!firmware_has_feature(FW_FEATURE_VIO))
@@ -6564,6 +6569,12 @@ static int __init ibmvfc_module_init(void)
 	printk(KERN_INFO IBMVFC_NAME": IBM Virtual Fibre Channel Driver version: %s %s\n",
 	       IBMVFC_DRIVER_VERSION, IBMVFC_DRIVER_DATE);
 
+	if (max_sectors < min_max_sectors) {
+		printk(KERN_ERR IBMVFC_NAME": max_sectors must be at least %d.\n",
+			min_max_sectors);
+		max_sectors = min_max_sectors;
+	}
+
 	ibmvfc_transport_template = fc_attach_transport(&ibmvfc_transport_functions);
 	if (!ibmvfc_transport_template)
 		return -ENOMEM;
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 745ad5ac7251..c73ed2314ad0 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -32,7 +32,7 @@
 #define IBMVFC_DEBUG			0
 #define IBMVFC_MAX_TARGETS		1024
 #define IBMVFC_MAX_LUN			0xffffffff
-#define IBMVFC_MAX_SECTORS		0xffffu
+#define IBMVFC_MAX_SECTORS		2048
 #define IBMVFC_MAX_DISC_THREADS	4
 #define IBMVFC_TGT_MEMPOOL_SZ		64
 #define IBMVFC_MAX_CMDS_PER_LUN	64
-- 
2.43.5

Re: [PATCH v2] ibmvfc: Add max_sectors module parameter

[Martin Wilck]

On Fri, 2024-08-30 at 15:42 -0500, Brian King wrote:
> There are some scenarios that can occur, such as performing an
> upgrade of the virtual I/O server, where the supported max transfer
> of the backing device for an ibmvfc HBA can change. If the max
> transfer of the backing device decreases, this can cause issues with
> previously discovered LUNs. This patch accomplishes two things.
> First, it changes the default ibmvfc max transfer value to 1MB.
> This is generally supported by all backing devices, which should
> mitigate this issue out of the box. Secondly, it adds a module
> parameter, enabling a user to increase the max transfer value to
> values that are larger than 1MB, as long as they have configured
> these larger values on the virtual I/O server as well.
> 
> Signed-off-by: Brian King <brking@linux.ibm.com>
> ---
>  drivers/scsi/ibmvscsi/ibmvfc.c | 17 ++++++++++++++---
>  drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c
> b/drivers/scsi/ibmvscsi/ibmvfc.c
> index a3d1013c8307..3349d321aa07 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -37,6 +37,7 @@ static unsigned int default_timeout =
> IBMVFC_DEFAULT_TIMEOUT;
>  static u64 max_lun = IBMVFC_MAX_LUN;
>  static unsigned int max_targets = IBMVFC_MAX_TARGETS;
>  static unsigned int max_requests = IBMVFC_MAX_REQUESTS_DEFAULT;
> +static u16 max_sectors = IBMVFC_MAX_SECTORS;

Am I understanding correctly that the maximum supported value for
max_sectors is USHRT_MAX, and you're ensuring that indirectly by using
an u16 type?

If yes, I think this would justify a comment in the code.

Regards,
Martin

>  static u16 scsi_qdepth = IBMVFC_SCSI_QDEPTH;
>  static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;
>  static unsigned int ibmvfc_debug = IBMVFC_DEBUG;
> @@ -83,6 +84,9 @@ MODULE_PARM_DESC(default_timeout,
>  module_param_named(max_requests, max_requests, uint, S_IRUGO);
>  MODULE_PARM_DESC(max_requests, "Maximum requests for this adapter. "
>  		 "[Default="
> __stringify(IBMVFC_MAX_REQUESTS_DEFAULT) "]");
> +module_param_named(max_sectors, max_sectors, ushort, S_IRUGO);
> +MODULE_PARM_DESC(max_sectors, "Maximum sectors for this adapter. "
> +		 "[Default=" __stringify(IBMVFC_MAX_SECTORS) "]");
>  module_param_named(scsi_qdepth, scsi_qdepth, ushort, S_IRUGO);
>  MODULE_PARM_DESC(scsi_qdepth, "Maximum scsi command depth per
> adapter queue. "
>  		 "[Default=" __stringify(IBMVFC_SCSI_QDEPTH) "]");
> @@ -1494,7 +1498,7 @@ static void ibmvfc_set_login_info(struct
> ibmvfc_host *vhost)
>  	memset(login_info, 0, sizeof(*login_info));
>  
>  	login_info->ostype = cpu_to_be32(IBMVFC_OS_LINUX);
> -	login_info->max_dma_len = cpu_to_be64(IBMVFC_MAX_SECTORS <<
> 9);
> +	login_info->max_dma_len = cpu_to_be64(max_sectors << 9);
>  	login_info->max_payload = cpu_to_be32(sizeof(struct
> ibmvfc_fcp_cmd_iu));
>  	login_info->max_response = cpu_to_be32(sizeof(struct
> ibmvfc_fcp_rsp));
>  	login_info->partition_num = cpu_to_be32(vhost-
> >partition_number);
> @@ -5230,7 +5234,7 @@ static void ibmvfc_npiv_login_done(struct
> ibmvfc_event *evt)
>  	}
>  
>  	vhost->logged_in = 1;
> -	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len)
> >> 9), IBMVFC_MAX_SECTORS);
> +	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len)
> >> 9), max_sectors);
>  	dev_info(vhost->dev, "Host partition: %s, device: %s %s %s
> max sectors %u\n",
>  		 rsp->partition_name, rsp->device_name, rsp-
> >port_loc_code,
>  		 rsp->drc_name, npiv_max_sectors);
> @@ -6329,7 +6333,7 @@ static int ibmvfc_probe(struct vio_dev *vdev,
> const struct vio_device_id *id)
>  	shost->can_queue = scsi_qdepth;
>  	shost->max_lun = max_lun;
>  	shost->max_id = max_targets;
> -	shost->max_sectors = IBMVFC_MAX_SECTORS;
> +	shost->max_sectors = max_sectors;
>  	shost->max_cmd_len = IBMVFC_MAX_CDB_LEN;
>  	shost->unique_id = shost->host_no;
>  	shost->nr_hw_queues = mq_enabled ? min(max_scsi_queues,
> nr_scsi_hw_queues) : 1;
> @@ -6556,6 +6560,7 @@ static struct fc_function_template
> ibmvfc_transport_functions = {
>   **/
>  static int __init ibmvfc_module_init(void)
>  {
> +	int min_max_sectors = PAGE_SIZE >> 9;
>  	int rc;
>  
>  	if (!firmware_has_feature(FW_FEATURE_VIO))
> @@ -6564,6 +6569,12 @@ static int __init ibmvfc_module_init(void)
>  	printk(KERN_INFO IBMVFC_NAME": IBM Virtual Fibre Channel
> Driver version: %s %s\n",
>  	       IBMVFC_DRIVER_VERSION, IBMVFC_DRIVER_DATE);
>  
> +	if (max_sectors < min_max_sectors) {
> +		printk(KERN_ERR IBMVFC_NAME": max_sectors must be at
> least %d.\n",
> +			min_max_sectors);
> +		max_sectors = min_max_sectors;
> +	}
> +
>  	ibmvfc_transport_template =
> fc_attach_transport(&ibmvfc_transport_functions);
>  	if (!ibmvfc_transport_template)
>  		return -ENOMEM;
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h
> b/drivers/scsi/ibmvscsi/ibmvfc.h
> index 745ad5ac7251..c73ed2314ad0 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.h
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.h
> @@ -32,7 +32,7 @@
>  #define IBMVFC_DEBUG			0
>  #define IBMVFC_MAX_TARGETS		1024
>  #define IBMVFC_MAX_LUN			0xffffffff
> -#define IBMVFC_MAX_SECTORS		0xffffu
> +#define IBMVFC_MAX_SECTORS		2048
>  #define IBMVFC_MAX_DISC_THREADS	4
>  #define IBMVFC_TGT_MEMPOOL_SZ		64
>  #define IBMVFC_MAX_CMDS_PER_LUN	64

[PATCH v3] ibmvfc: Add max_sectors module parameter

[Brian King]

There are some scenarios that can occur, such as performing an
upgrade of the virtual I/O server, where the supported max transfer
of the backing device for an ibmvfc HBA can change. If the max
transfer of the backing device decreases, this can cause issues with
previously discovered LUNs. This patch accomplishes two things.
First, it changes the default ibmvfc max transfer value to 1MB.
This is generally supported by all backing devices, which should
mitigate this issue out of the box. Secondly, it adds a module
parameter, enabling a user to increase the max transfer value to
values that are larger than 1MB, as long as they have configured
these larger values on the virtual I/O server as well.

Signed-off-by: Brian King <brking@linux.ibm.com>
---
 drivers/scsi/ibmvscsi/ibmvfc.c | 19 ++++++++++++++++---
 drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index a3d1013c8307..c668701bf671 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -37,6 +37,7 @@ static unsigned int default_timeout = IBMVFC_DEFAULT_TIMEOUT;
 static u64 max_lun = IBMVFC_MAX_LUN;
 static unsigned int max_targets = IBMVFC_MAX_TARGETS;
 static unsigned int max_requests = IBMVFC_MAX_REQUESTS_DEFAULT;
+static u16 max_sectors = IBMVFC_MAX_SECTORS;
 static u16 scsi_qdepth = IBMVFC_SCSI_QDEPTH;
 static unsigned int disc_threads = IBMVFC_MAX_DISC_THREADS;
 static unsigned int ibmvfc_debug = IBMVFC_DEBUG;
@@ -83,6 +84,9 @@ MODULE_PARM_DESC(default_timeout,
 module_param_named(max_requests, max_requests, uint, S_IRUGO);
 MODULE_PARM_DESC(max_requests, "Maximum requests for this adapter. "
 		 "[Default=" __stringify(IBMVFC_MAX_REQUESTS_DEFAULT) "]");
+module_param_named(max_sectors, max_sectors, ushort, S_IRUGO);
+MODULE_PARM_DESC(max_sectors, "Maximum sectors for this adapter. "
+		 "[Default=" __stringify(IBMVFC_MAX_SECTORS) "]");
 module_param_named(scsi_qdepth, scsi_qdepth, ushort, S_IRUGO);
 MODULE_PARM_DESC(scsi_qdepth, "Maximum scsi command depth per adapter queue. "
 		 "[Default=" __stringify(IBMVFC_SCSI_QDEPTH) "]");
@@ -1494,7 +1498,7 @@ static void ibmvfc_set_login_info(struct ibmvfc_host *vhost)
 	memset(login_info, 0, sizeof(*login_info));
 
 	login_info->ostype = cpu_to_be32(IBMVFC_OS_LINUX);
-	login_info->max_dma_len = cpu_to_be64(IBMVFC_MAX_SECTORS << 9);
+	login_info->max_dma_len = cpu_to_be64(max_sectors << 9);
 	login_info->max_payload = cpu_to_be32(sizeof(struct ibmvfc_fcp_cmd_iu));
 	login_info->max_response = cpu_to_be32(sizeof(struct ibmvfc_fcp_rsp));
 	login_info->partition_num = cpu_to_be32(vhost->partition_number);
@@ -5230,7 +5234,7 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event *evt)
 	}
 
 	vhost->logged_in = 1;
-	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len) >> 9), IBMVFC_MAX_SECTORS);
+	npiv_max_sectors = min((uint)(be64_to_cpu(rsp->max_dma_len) >> 9), max_sectors);
 	dev_info(vhost->dev, "Host partition: %s, device: %s %s %s max sectors %u\n",
 		 rsp->partition_name, rsp->device_name, rsp->port_loc_code,
 		 rsp->drc_name, npiv_max_sectors);
@@ -6329,7 +6333,7 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 	shost->can_queue = scsi_qdepth;
 	shost->max_lun = max_lun;
 	shost->max_id = max_targets;
-	shost->max_sectors = IBMVFC_MAX_SECTORS;
+	shost->max_sectors = max_sectors;
 	shost->max_cmd_len = IBMVFC_MAX_CDB_LEN;
 	shost->unique_id = shost->host_no;
 	shost->nr_hw_queues = mq_enabled ? min(max_scsi_queues, nr_scsi_hw_queues) : 1;
@@ -6556,6 +6560,7 @@ static struct fc_function_template ibmvfc_transport_functions = {
  **/
 static int __init ibmvfc_module_init(void)
 {
+	int min_max_sectors = PAGE_SIZE >> 9;
 	int rc;
 
 	if (!firmware_has_feature(FW_FEATURE_VIO))
@@ -6564,6 +6569,14 @@ static int __init ibmvfc_module_init(void)
 	printk(KERN_INFO IBMVFC_NAME": IBM Virtual Fibre Channel Driver version: %s %s\n",
 	       IBMVFC_DRIVER_VERSION, IBMVFC_DRIVER_DATE);
 
+	/* Range check the max_sectors module parameter. The upper bounds
+	   is implicity checked since the parameter is a ushort */
+	if (max_sectors < min_max_sectors) {
+		printk(KERN_ERR IBMVFC_NAME": max_sectors must be at least %d.\n",
+			min_max_sectors);
+		max_sectors = min_max_sectors;
+	}
+
 	ibmvfc_transport_template = fc_attach_transport(&ibmvfc_transport_functions);
 	if (!ibmvfc_transport_template)
 		return -ENOMEM;
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 745ad5ac7251..c73ed2314ad0 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -32,7 +32,7 @@
 #define IBMVFC_DEBUG			0
 #define IBMVFC_MAX_TARGETS		1024
 #define IBMVFC_MAX_LUN			0xffffffff
-#define IBMVFC_MAX_SECTORS		0xffffu
+#define IBMVFC_MAX_SECTORS		2048
 #define IBMVFC_MAX_DISC_THREADS	4
 #define IBMVFC_TGT_MEMPOOL_SZ		64
 #define IBMVFC_MAX_CMDS_PER_LUN	64
-- 
2.43.5

Re: [PATCH v3] ibmvfc: Add max_sectors module parameter

[Martin Wilck]

On Tue, 2024-09-03 at 08:47 -0500, Brian King wrote:
> There are some scenarios that can occur, such as performing an
> upgrade of the virtual I/O server, where the supported max transfer
> of the backing device for an ibmvfc HBA can change. If the max
> transfer of the backing device decreases, this can cause issues with
> previously discovered LUNs. This patch accomplishes two things.
> First, it changes the default ibmvfc max transfer value to 1MB.
> This is generally supported by all backing devices, which should
> mitigate this issue out of the box. Secondly, it adds a module
> parameter, enabling a user to increase the max transfer value to
> values that are larger than 1MB, as long as they have configured
> these larger values on the virtual I/O server as well.
> 
> Signed-off-by: Brian King <brking@linux.ibm.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: [PATCH v3] ibmvfc: Add max_sectors module parameter

[Martin Wilck]

On Tue, 2024-09-03 at 16:42 +0200, Martin Wilck wrote:
> On Tue, 2024-09-03 at 08:47 -0500, Brian King wrote:
> > There are some scenarios that can occur, such as performing an
> > upgrade of the virtual I/O server, where the supported max transfer
> > of the backing device for an ibmvfc HBA can change. If the max
> > transfer of the backing device decreases, this can cause issues
> > with
> > previously discovered LUNs. This patch accomplishes two things.
> > First, it changes the default ibmvfc max transfer value to 1MB.
> > This is generally supported by all backing devices, which should
> > mitigate this issue out of the box. Secondly, it adds a module
> > parameter, enabling a user to increase the max transfer value to
> > values that are larger than 1MB, as long as they have configured
> > these larger values on the virtual I/O server as well.
> > 
> > Signed-off-by: Brian King <brking@linux.ibm.com>
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>

Martin P., do you need another review to merge this patch?

Martin W.

Re: [PATCH v3] ibmvfc: Add max_sectors module parameter

[Martin K. Petersen]

Brian,

> There are some scenarios that can occur, such as performing an upgrade
> of the virtual I/O server, where the supported max transfer of the
> backing device for an ibmvfc HBA can change. If the max transfer of
> the backing device decreases, this can cause issues with previously
> discovered LUNs. This patch accomplishes two things. First, it changes
> the default ibmvfc max transfer value to 1MB. This is generally
> supported by all backing devices, which should mitigate this issue out
> of the box. Secondly, it adds a module parameter, enabling a user to
> increase the max transfer value to values that are larger than 1MB, as
> long as they have configured these larger values on the virtual I/O
> server as well.

Applied to 6.12/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3] ibmvfc: Add max_sectors module parameter

[Martin K. Petersen]

On Tue, 03 Sep 2024 08:47:09 -0500, Brian King wrote:

> There are some scenarios that can occur, such as performing an
> upgrade of the virtual I/O server, where the supported max transfer
> of the backing device for an ibmvfc HBA can change. If the max
> transfer of the backing device decreases, this can cause issues with
> previously discovered LUNs. This patch accomplishes two things.
> First, it changes the default ibmvfc max transfer value to 1MB.
> This is generally supported by all backing devices, which should
> mitigate this issue out of the box. Secondly, it adds a module
> parameter, enabling a user to increase the max transfer value to
> values that are larger than 1MB, as long as they have configured
> these larger values on the virtual I/O server as well.
> 
> [...]

Applied to 6.12/scsi-queue, thanks!

[1/1] ibmvfc: Add max_sectors module parameter
      https://git.kernel.org/mkp/scsi/c/e36840069454

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] ibmvfc: Add max_sectors module parameter

[Hannes Reinecke]

On 7/30/24 19:51, Brian King wrote:
> There are some scenarios that can occur, such as performing an
> upgrade of the virtual I/O server, where the supported max transfer
> of the backing device for an ibmvfc HBA can change. If the max
> transfer of the backing device decreases, this can cause issues with
> previously discovered LUNs. This patch accomplishes two things.
> First, it changes the default ibmvfc max transfer value to 1MB.
> This is generally supported by all backing devices, which should
> mitigate this issue out of the box. Secondly, it adds a module
> parameter, enabling a user to increase the max transfer value to
> values that are larger than 1MB, as long as they have configured
> these larger values on the virtual I/O server as well.
> 
> Signed-off-by: Brian King <brking@linux.ibm.com>
> ---
>   drivers/scsi/ibmvscsi/ibmvfc.c | 10 +++++++---
>   drivers/scsi/ibmvscsi/ibmvfc.h |  2 +-
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich