Re: [PATCH 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()

[Johannes Thumshirn <jthumshirn@suse.de>]

On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> The function storvsc_channel_init() repeatedly interacts with the host to
> extract various channel properties. Refactor this code to eliminate code
> repetition.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> Tested-by: Alex Ng <alexng@microsoft.com>
> ---
>  drivers/scsi/storvsc_drv.c |  155 ++++++++++++++++----------------------------
>  1 files changed, 57 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index b94d471..6f18e94 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -721,29 +721,16 @@ static void cache_wwn(struct storvsc_device *stor_device,
>  	}
>  }
>  
> -static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +static int storvsc_execute_vstor_op(struct hv_device *device,
> +				    struct storvsc_cmd_request *request,
> +				    bool status_check)
>  {
> -	struct storvsc_device *stor_device;
> -	struct storvsc_cmd_request *request;
>  	struct vstor_packet *vstor_packet;
> -	int ret, t, i;
> -	int max_chns;
> -	bool process_sub_channels = false;
> -
> -	stor_device = get_out_stor_device(device);
> -	if (!stor_device)
> -		return -ENODEV;
> +	int ret, t;
>  
> -	request = &stor_device->init_request;
>  	vstor_packet = &request->vstor_packet;
>  
> -	/*
> -	 * Now, initiate the vsc/vsp initialization protocol on the open
> -	 * channel
> -	 */
> -	memset(request, 0, sizeof(struct storvsc_cmd_request));
>  	init_completion(&request->wait_event);
> -	vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
>  	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>  
>  	ret = vmbus_sendpacket(device->channel, vstor_packet,
> @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>  			       VM_PKT_DATA_INBAND,
>  			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>  	if (ret != 0)
> -		goto cleanup;
> +		goto done;
>  
>  	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
>  	if (t == 0) {
>  		ret = -ETIMEDOUT;
> -		goto cleanup;
> +		goto done;
>  	}
>  
> +	if (!status_check)
> +		goto done;
> +
>  	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
>  	    vstor_packet->status != 0) {
>  		ret = -EINVAL;
> -		goto cleanup;
> +		goto done;
>  	}
>  
> +done:
> +	return ret;
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +{
> +	struct storvsc_device *stor_device;
> +	struct storvsc_cmd_request *request;
> +	struct vstor_packet *vstor_packet;
> +	int ret, i;
> +	int max_chns;
> +	bool process_sub_channels = false;
> +
> +	stor_device = get_out_stor_device(device);
> +	if (!stor_device)
> +		return -ENODEV;
> +
> +	request = &stor_device->init_request;
> +	vstor_packet = &request->vstor_packet;
> +
> +	/*
> +	 * Now, initiate the vsc/vsp initialization protocol on the open
> +	 * channel
> +	 */
> +	memset(request, 0, sizeof(struct storvsc_cmd_request));
> +	vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> +	ret = storvsc_execute_vstor_op(device, request, true);
> +	if (ret)
> +		goto cleanup;
> +
> +	/*
> +	 * Query host supported protocol version.
> +	 */
>  
>  	for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
>  		/* reuse the packet for version range supported */
>  		memset(vstor_packet, 0, sizeof(struct vstor_packet));
>  		vstor_packet->operation =
>  			VSTOR_OPERATION_QUERY_PROTOCOL_VERSION;
> -		vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>  
>  		vstor_packet->version.major_minor =
>  			vmstor_protocols[i].protocol_version;
> @@ -783,20 +805,9 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>  		 */
>  		vstor_packet->version.revision = 0;
>  
> -		ret = vmbus_sendpacket(device->channel, vstor_packet,
> -			       (sizeof(struct vstor_packet) -
> -				vmscsi_size_delta),
> -			       (unsigned long)request,
> -			       VM_PKT_DATA_INBAND,
> -			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -		if (ret != 0)
> -			goto cleanup;
> -
> -		t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> -		if (t == 0) {
> -			ret = -ETIMEDOUT;
> +		ret = storvsc_execute_vstor_op(device, request, false);
> +		if (ret)
>  			goto cleanup;
> -		}
>  
>  		if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO) {
>  			ret = -EINVAL;
> @@ -822,32 +833,14 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>  		goto cleanup;
>  	}
>  
> -
> +	/*
> +	 * Query channel properties.
> +	 */
>  	memset(vstor_packet, 0, sizeof(struct vstor_packet));
>  	vstor_packet->operation = VSTOR_OPERATION_QUERY_PROPERTIES;
> -	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> -	ret = vmbus_sendpacket(device->channel, vstor_packet,
> -			       (sizeof(struct vstor_packet) -
> -				vmscsi_size_delta),
> -			       (unsigned long)request,
> -			       VM_PKT_DATA_INBAND,
> -			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> -	if (ret != 0)
> -		goto cleanup;
> -
> -	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> -	if (t == 0) {
> -		ret = -ETIMEDOUT;
> -		goto cleanup;
> -	}
> -
> -	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> -	    vstor_packet->status != 0) {
> -		ret = -EINVAL;
> +	ret = storvsc_execute_vstor_op(device, request, true);
> +	if (ret)
>  		goto cleanup;
> -	}
>  
>  	/*
>  	 * Check to see if multi-channel support is there.
> @@ -866,30 +859,15 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>  	if (!is_fc)
>  		goto done;
>  
> +	/*
> +	 * For FC devices retrieve FC HBA data.
> +	 */
>  	memset(vstor_packet, 0, sizeof(struct vstor_packet));
>  	vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
> -	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>  
> -	ret = vmbus_sendpacket(device->channel, vstor_packet,
> -			       (sizeof(struct vstor_packet) -
> -			       vmscsi_size_delta),
> -			       (unsigned long)request,
> -			       VM_PKT_DATA_INBAND,
> -			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> -	if (ret != 0)
> -		goto cleanup;
> -
> -	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> -	if (t == 0) {
> -		ret = -ETIMEDOUT;
> -		goto cleanup;
> -	}
> -
> -	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> -	    vstor_packet->status != 0)
> +	ret = storvsc_execute_vstor_op(device, request, true);
> +	if (ret)
>  		goto cleanup;
> -
>  	/*
>  	 * Cache the currently active port and node ww names.
>  	 */
> @@ -899,29 +877,10 @@ done:
>  
>  	memset(vstor_packet, 0, sizeof(struct vstor_packet));
>  	vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
> -	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>  
> -	ret = vmbus_sendpacket(device->channel, vstor_packet,
> -			       (sizeof(struct vstor_packet) -
> -				vmscsi_size_delta),
> -			       (unsigned long)request,
> -			       VM_PKT_DATA_INBAND,
> -			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> -	if (ret != 0)
> -		goto cleanup;
> -
> -	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> -	if (t == 0) {
> -		ret = -ETIMEDOUT;
> -		goto cleanup;
> -	}
> -
> -	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> -	    vstor_packet->status != 0) {
> -		ret = -EINVAL;
> +	ret = storvsc_execute_vstor_op(device, request, true);
> +	if (ret)
>  		goto cleanup;
> -	}
>  
>  	if (process_sub_channels)
>  		handle_multichannel_storage(device, max_chns);
> -- 
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850