Re: [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available

[Bart Van Assche <bvanassche@xxxxxxx>]

On 9/16/19 8:24 PM, Honggang LI wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 2eadb038b257..d8dee5900c08 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -76,6 +76,7 @@ static bool register_always = true;
>   static bool never_register;
>   static int topspin_workarounds = 1;
>   static uint32_t srp_opt_max_it_iu_size;
> +static unsigned int srp_max_imm_data;

Each SCSI host can represent a connection to another SRP target, and the 
max_it_iu_size parameter can differ per target. So I think this variable 
should be moved into struct srp_target_port instead of being global.

>   module_param(srp_sg_tablesize, uint, 0444);
>   MODULE_PARM_DESC(srp_sg_tablesize, "Deprecated name for cmd_sg_entries");
> @@ -138,9 +139,9 @@ module_param_named(use_imm_data, srp_use_imm_data, bool, 0644);
>   MODULE_PARM_DESC(use_imm_data,
>   		 "Whether or not to request permission to use immediate data during SRP login.");
>   
> -static unsigned int srp_max_imm_data = 8 * 1024;
> -module_param_named(max_imm_data, srp_max_imm_data, uint, 0644);
> -MODULE_PARM_DESC(max_imm_data, "Maximum immediate data size.");
> +static unsigned int srp_default_max_imm_data = 8 * 1024;
> +module_param_named(max_imm_data, srp_default_max_imm_data, uint, 0644);
> +MODULE_PARM_DESC(max_imm_data, "Default maximum immediate data size.");

This description might confuse users. How about keeping the name of the 
variable and the module parameter and changing its description into the 
following?

"Maximum immediate data size if max_it_iu_size has not been specified."

>   
>   static unsigned ch_count;
>   module_param(ch_count, uint, 0444);
> @@ -1369,9 +1370,19 @@ static uint32_t srp_max_it_iu_len(int cmd_sg_cnt, bool use_imm_data)
>   		sizeof(struct srp_indirect_buf) +
>   		cmd_sg_cnt * sizeof(struct srp_direct_buf);
>   
> -	if (use_imm_data)
> -		max_iu_len = max(max_iu_len, SRP_IMM_DATA_OFFSET +
> -				 srp_max_imm_data);
> +	if (use_imm_data) {
> +		if (srp_opt_max_it_iu_size == 0) {
> +			srp_max_imm_data = srp_default_max_imm_data;
> +			max_iu_len = max(max_iu_len,
> +			   SRP_IMM_DATA_OFFSET + srp_max_imm_data);
> +		} else {
> +			srp_max_imm_data =
> +			   srp_opt_max_it_iu_size - SRP_IMM_DATA_OFFSET;

Please use as much of a line as possible. I think the recommended style 
in the Linux kernel is as follows:

			srp_max_imm_data = srp_opt_max_it_iu_size -
				SRP_IMM_DATA_OFFSET;

> @@ -3829,6 +3840,8 @@ static ssize_t srp_create_target(struct device *dev,
>   	if (ret < 0)
>   		goto put;
>   
> +	srp_opt_max_it_iu_size = 0;
> +

Static variables that are not initialized explicitly are initialized to 
zero. Since this initialization is redundant, please remove it.

Thanks,

Bart.