[PATCH 0/4] scsi: storvsc: Properly support FC hosts

[PATCH 0/4] scsi: storvsc: Properly support FC hosts

[K. Y. Srinivasan]

Properly support FC hosts. Additional cleanup patches are also
included.

K. Y. Srinivasan (4):
  scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
  scsi: storvsc: Properly support Fibre Channel devices
  scsi: storvsc: Refactor the code in storvsc_channel_init()
  scsi: storvsc: Tighten up the interrupt path

 drivers/scsi/storvsc_drv.c |  235 ++++++++++++++++++++++++++------------------
 1 files changed, 138 insertions(+), 97 deletions(-)

-- 
1.7.4.1

[PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet

[K. Y. Srinivasan]

The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in Linux match
the Window's definition.

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 |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index c41f674..00bb4bd 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -92,9 +92,8 @@ enum vstor_packet_operation {
  */
 
 struct hv_fc_wwn_packet {
-	bool	primary_active;
-	u8	reserved1;
-	u8	reserved2;
+	u8	primary_active;
+	u8	reserved1[3];
 	u8	primary_port_wwn[8];
 	u8	primary_node_wwn[8];
 	u8	secondary_port_wwn[8];
-- 
1.7.4.1

[PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices

[K. Y. Srinivasan]

For FC devices managed by this driver, atttach the appropriate transport
template. This will allow us to create the appropriate sysfs files for
these devices. With this we can publish the wwn for both the port and the node.

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 |  100 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 00bb4bd..b94d471 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -41,6 +41,7 @@
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_dbg.h>
+#include <scsi/scsi_transport_fc.h>
 
 /*
  * All wire protocol details (storage protocol between the guest and the host)
@@ -397,6 +398,7 @@ static int storvsc_timeout = 180;
 
 static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
 
+static struct scsi_transport_template *fc_transport_template;
 
 static void storvsc_on_channel_callback(void *context);
 
@@ -456,6 +458,11 @@ struct storvsc_device {
 	/* Used for vsc/vsp channel reset process */
 	struct storvsc_cmd_request init_request;
 	struct storvsc_cmd_request reset_request;
+	/*
+	 * Currently active port and node names for FC devices.
+	 */
+	u64 node_name;
+	u64 port_name;
 };
 
 struct hv_host_device {
@@ -695,7 +702,26 @@ static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
 	vmbus_are_subchannels_present(device->channel);
 }
 
-static int storvsc_channel_init(struct hv_device *device)
+static void cache_wwn(struct storvsc_device *stor_device,
+		      struct vstor_packet *vstor_packet)
+{
+	/*
+	 * Cache the currently active port and node ww names.
+	 */
+	if (vstor_packet->wwn_packet.primary_active) {
+		stor_device->node_name =
+			wwn_to_u64(vstor_packet->wwn_packet.primary_node_wwn);
+		stor_device->port_name =
+			wwn_to_u64(vstor_packet->wwn_packet.primary_port_wwn);
+	} else {
+		stor_device->node_name =
+			wwn_to_u64(vstor_packet->wwn_packet.secondary_node_wwn);
+		stor_device->port_name =
+			wwn_to_u64(vstor_packet->wwn_packet.secondary_port_wwn);
+	}
+}
+
+static int storvsc_channel_init(struct hv_device *device, bool is_fc)
 {
 	struct storvsc_device *stor_device;
 	struct storvsc_cmd_request *request;
@@ -837,6 +863,40 @@ static int storvsc_channel_init(struct hv_device *device)
 	stor_device->max_transfer_bytes =
 		vstor_packet->storage_channel_properties.max_transfer_bytes;
 
+	if (!is_fc)
+		goto done;
+
+	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)
+		goto cleanup;
+
+	/*
+	 * Cache the currently active port and node ww names.
+	 */
+	cache_wwn(stor_device, vstor_packet);
+
+done:
+
 	memset(vstor_packet, 0, sizeof(struct vstor_packet));
 	vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
 	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
@@ -1076,6 +1136,12 @@ static void storvsc_on_receive(struct hv_device *device,
 		schedule_work(&work->work);
 		break;
 
+	case VSTOR_OPERATION_FCHBA_DATA:
+		stor_device = get_in_stor_device(device);
+		cache_wwn(stor_device, vstor_packet);
+		fc_host_node_name(stor_device->host) = stor_device->node_name;
+		fc_host_port_name(stor_device->host) = stor_device->port_name;
+		break;
 	default:
 		break;
 	}
@@ -1131,7 +1197,8 @@ static void storvsc_on_channel_callback(void *context)
 	return;
 }
 
-static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
+static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
+				  bool is_fc)
 {
 	struct vmstorage_channel_properties props;
 	int ret;
@@ -1148,7 +1215,7 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
 	if (ret != 0)
 		return ret;
 
-	ret = storvsc_channel_init(device);
+	ret = storvsc_channel_init(device, is_fc);
 
 	return ret;
 }
@@ -1573,6 +1640,7 @@ static int storvsc_probe(struct hv_device *device,
 	struct Scsi_Host *host;
 	struct hv_host_device *host_dev;
 	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
+	bool is_fc = ((dev_id->driver_data == SFC_GUID) ? true : false);
 	int target = 0;
 	struct storvsc_device *stor_device;
 	int max_luns_per_target;
@@ -1630,7 +1698,7 @@ static int storvsc_probe(struct hv_device *device,
 	hv_set_drvdata(device, stor_device);
 
 	stor_device->port_number = host->host_no;
-	ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
+	ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size, is_fc);
 	if (ret)
 		goto err_out1;
 
@@ -1642,6 +1710,7 @@ static int storvsc_probe(struct hv_device *device,
 		host->max_lun = STORVSC_FC_MAX_LUNS_PER_TARGET;
 		host->max_id = STORVSC_FC_MAX_TARGETS;
 		host->max_channel = STORVSC_FC_MAX_CHANNELS - 1;
+		host->transportt = fc_transport_template;
 		break;
 
 	case SCSI_GUID:
@@ -1681,6 +1750,10 @@ static int storvsc_probe(struct hv_device *device,
 			goto err_out2;
 		}
 	}
+	if (host->transportt == fc_transport_template) {
+		fc_host_node_name(host) = stor_device->node_name;
+		fc_host_port_name(host) = stor_device->port_name;
+	}
 	return 0;
 
 err_out2:
@@ -1706,6 +1779,8 @@ static int storvsc_remove(struct hv_device *dev)
 	struct storvsc_device *stor_device = hv_get_drvdata(dev);
 	struct Scsi_Host *host = stor_device->host;
 
+	if (host->transportt == fc_transport_template)
+		fc_remove_host(host);
 	scsi_remove_host(host);
 	storvsc_dev_remove(dev);
 	scsi_host_put(host);
@@ -1720,8 +1795,14 @@ static struct hv_driver storvsc_drv = {
 	.remove = storvsc_remove,
 };
 
+static struct fc_function_template fc_transport_functions = {
+	.show_host_node_name = 1,
+	.show_host_port_name = 1,
+};
+
 static int __init storvsc_drv_init(void)
 {
+	int ret;
 
 	/*
 	 * Divide the ring buffer data size (which is 1 page less
@@ -1736,12 +1817,21 @@ static int __init storvsc_drv_init(void)
 		vmscsi_size_delta,
 		sizeof(u64)));
 
-	return vmbus_driver_register(&storvsc_drv);
+	fc_transport_template = fc_attach_transport(&fc_transport_functions);
+	if (!fc_transport_template)
+		return -ENODEV;
+
+	ret = vmbus_driver_register(&storvsc_drv);
+	if (ret)
+		fc_release_transport(fc_transport_template);
+
+	return ret;
 }
 
 static void __exit storvsc_drv_exit(void)
 {
 	vmbus_driver_unregister(&storvsc_drv);
+	fc_release_transport(fc_transport_template);
 }
 
 MODULE_LICENSE("GPL");
-- 
1.7.4.1

Re: [PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices

[Johannes Thumshirn]

On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> For FC devices managed by this driver, atttach the appropriate transport
> template. This will allow us to create the appropriate sysfs files for
> these devices. With this we can publish the wwn for both the port and the node.
> 
> 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 |  100 +++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 95 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 00bb4bd..b94d471 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -41,6 +41,7 @@
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_devinfo.h>
>  #include <scsi/scsi_dbg.h>
> +#include <scsi/scsi_transport_fc.h>
>  
>  /*
>   * All wire protocol details (storage protocol between the guest and the host)
> @@ -397,6 +398,7 @@ static int storvsc_timeout = 180;
>  
>  static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
>  
> +static struct scsi_transport_template *fc_transport_template;
>  
>  static void storvsc_on_channel_callback(void *context);
>  
> @@ -456,6 +458,11 @@ struct storvsc_device {
>  	/* Used for vsc/vsp channel reset process */
>  	struct storvsc_cmd_request init_request;
>  	struct storvsc_cmd_request reset_request;
> +	/*
> +	 * Currently active port and node names for FC devices.
> +	 */
> +	u64 node_name;
> +	u64 port_name;
>  };
>  
>  struct hv_host_device {
> @@ -695,7 +702,26 @@ static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
>  	vmbus_are_subchannels_present(device->channel);
>  }
>  
> -static int storvsc_channel_init(struct hv_device *device)
> +static void cache_wwn(struct storvsc_device *stor_device,
> +		      struct vstor_packet *vstor_packet)
> +{
> +	/*
> +	 * Cache the currently active port and node ww names.
> +	 */
> +	if (vstor_packet->wwn_packet.primary_active) {
> +		stor_device->node_name =
> +			wwn_to_u64(vstor_packet->wwn_packet.primary_node_wwn);
> +		stor_device->port_name =
> +			wwn_to_u64(vstor_packet->wwn_packet.primary_port_wwn);
> +	} else {
> +		stor_device->node_name =
> +			wwn_to_u64(vstor_packet->wwn_packet.secondary_node_wwn);
> +		stor_device->port_name =
> +			wwn_to_u64(vstor_packet->wwn_packet.secondary_port_wwn);
> +	}
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>  {
>  	struct storvsc_device *stor_device;
>  	struct storvsc_cmd_request *request;
> @@ -837,6 +863,40 @@ static int storvsc_channel_init(struct hv_device *device)
>  	stor_device->max_transfer_bytes =
>  		vstor_packet->storage_channel_properties.max_transfer_bytes;
>  
> +	if (!is_fc)
> +		goto done;
> +
> +	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)
> +		goto cleanup;
> +
> +	/*
> +	 * Cache the currently active port and node ww names.
> +	 */
> +	cache_wwn(stor_device, vstor_packet);
> +
> +done:
> +

That goto use is a bit weird. I see you did it because of the 80 chars limit
but 
if (is_fc) {
	[...]
}

is way more readable IMHO.

>  	memset(vstor_packet, 0, sizeof(struct vstor_packet));
>  	vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
>  	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> @@ -1076,6 +1136,12 @@ static void storvsc_on_receive(struct hv_device *device,
>  		schedule_work(&work->work);
>  		break;
>  
> +	case VSTOR_OPERATION_FCHBA_DATA:
> +		stor_device = get_in_stor_device(device);
> +		cache_wwn(stor_device, vstor_packet);
> +		fc_host_node_name(stor_device->host) = stor_device->node_name;
> +		fc_host_port_name(stor_device->host) = stor_device->port_name;
> +		break;
>  	default:
>  		break;
>  	}
> @@ -1131,7 +1197,8 @@ static void storvsc_on_channel_callback(void *context)
>  	return;
>  }
>  
> -static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
> +static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
> +				  bool is_fc)
>  {
>  	struct vmstorage_channel_properties props;
>  	int ret;
> @@ -1148,7 +1215,7 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
>  	if (ret != 0)
>  		return ret;
>  
> -	ret = storvsc_channel_init(device);
> +	ret = storvsc_channel_init(device, is_fc);
>  
>  	return ret;
>  }
> @@ -1573,6 +1640,7 @@ static int storvsc_probe(struct hv_device *device,
>  	struct Scsi_Host *host;
>  	struct hv_host_device *host_dev;
>  	bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
> +	bool is_fc = ((dev_id->driver_data == SFC_GUID) ? true : false);
>  	int target = 0;
>  	struct storvsc_device *stor_device;
>  	int max_luns_per_target;
> @@ -1630,7 +1698,7 @@ static int storvsc_probe(struct hv_device *device,
>  	hv_set_drvdata(device, stor_device);
>  
>  	stor_device->port_number = host->host_no;
> -	ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
> +	ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size, is_fc);
>  	if (ret)
>  		goto err_out1;
>  
> @@ -1642,6 +1710,7 @@ static int storvsc_probe(struct hv_device *device,
>  		host->max_lun = STORVSC_FC_MAX_LUNS_PER_TARGET;
>  		host->max_id = STORVSC_FC_MAX_TARGETS;
>  		host->max_channel = STORVSC_FC_MAX_CHANNELS - 1;
> +		host->transportt = fc_transport_template;
>  		break;
>  
>  	case SCSI_GUID:
> @@ -1681,6 +1750,10 @@ static int storvsc_probe(struct hv_device *device,
>  			goto err_out2;
>  		}
>  	}
> +	if (host->transportt == fc_transport_template) {
> +		fc_host_node_name(host) = stor_device->node_name;
> +		fc_host_port_name(host) = stor_device->port_name;
> +	}
>  	return 0;
>  
>  err_out2:
> @@ -1706,6 +1779,8 @@ static int storvsc_remove(struct hv_device *dev)
>  	struct storvsc_device *stor_device = hv_get_drvdata(dev);
>  	struct Scsi_Host *host = stor_device->host;
>  
> +	if (host->transportt == fc_transport_template)
> +		fc_remove_host(host);
>  	scsi_remove_host(host);
>  	storvsc_dev_remove(dev);
>  	scsi_host_put(host);
> @@ -1720,8 +1795,14 @@ static struct hv_driver storvsc_drv = {
>  	.remove = storvsc_remove,
>  };
>  
> +static struct fc_function_template fc_transport_functions = {
> +	.show_host_node_name = 1,
> +	.show_host_port_name = 1,
> +};
> +
>  static int __init storvsc_drv_init(void)
>  {
> +	int ret;
>  
>  	/*
>  	 * Divide the ring buffer data size (which is 1 page less
> @@ -1736,12 +1817,21 @@ static int __init storvsc_drv_init(void)
>  		vmscsi_size_delta,
>  		sizeof(u64)));
>  
> -	return vmbus_driver_register(&storvsc_drv);
> +	fc_transport_template = fc_attach_transport(&fc_transport_functions);
> +	if (!fc_transport_template)
> +		return -ENODEV;
> +
> +	ret = vmbus_driver_register(&storvsc_drv);
> +	if (ret)
> +		fc_release_transport(fc_transport_template);
> +
> +	return ret;
>  }
>  
>  static void __exit storvsc_drv_exit(void)
>  {
>  	vmbus_driver_unregister(&storvsc_drv);
> +	fc_release_transport(fc_transport_template);
>  }
>  
>  MODULE_LICENSE("GPL");
> -- 
> 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

-- 
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

Re: [PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices

[Dan Carpenter]

On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> +	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)
> +		goto cleanup;

"cleanup" is a misleading name because it doesn't clean up anything.
Do nothing gotos are a pain in the butt and they always introduce bugs.
For example, you appear to have forgotten to set the error code.  But
because it's a do-nothing goto it's ambiguous so perhaps returning
success was intended.

Empirically this style of coding causes bugs.  It does not prevent them.
It is a bad style if you believe in measuring, evidence and science.

regards,
dan carpenter

RE: [PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 11, 2015 2:25 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> martin.petersen@oracle.com
> Subject: Re: [PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices
> 
> On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> > +	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)
> > +		goto cleanup;
> 
> "cleanup" is a misleading name because it doesn't clean up anything.
> Do nothing gotos are a pain in the butt and they always introduce bugs.
> For example, you appear to have forgotten to set the error code.  But
> because it's a do-nothing goto it's ambiguous so perhaps returning
> success was intended.
> 
> Empirically this style of coding causes bugs.  It does not prevent them.
> It is a bad style if you believe in measuring, evidence and science.

Thanks Dan. I will fix this and resend.

K. Y

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

[K. Y. Srinivasan]

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

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

[Johannes Thumshirn]

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

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

[Dan Carpenter]

On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> @@ -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;

See?  This goto looks exactly the same as the earlier buggy goto but
it's actually correct.  Meanwhile if you just used an explicit
"return 0;" then it would be easy to understand.

I rant about this all the time but it's because it's bad deliberately.
It's normal to have bugs, but this deliberate stuff really I can't
understand it...

> +
>  	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;

10 lines earlier there is an explicit "return -ENODEV" so it's not as if
writing explicit returns will kill you.

regards,
dan carpenter

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

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 11, 2015 2:41 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> martin.petersen@oracle.com
> Subject: Re: [PATCH 3/4] scsi: storvsc: Refactor the code in
> storvsc_channel_init()
> 
> On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> > @@ -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;
> 
> See?  This goto looks exactly the same as the earlier buggy goto but
> it's actually correct.  Meanwhile if you just used an explicit
> "return 0;" then it would be easy to understand.
> 
> I rant about this all the time but it's because it's bad deliberately.
> It's normal to have bugs, but this deliberate stuff really I can't
> understand it...
> 
> > +
> >  	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;
> 
> 10 lines earlier there is an explicit "return -ENODEV" so it's not as if
> writing explicit returns will kill you.

Thanks Dan; I will cleanup the code and resend.

Regards,

K. Y

[PATCH 4/4] scsi: storvsc: Tighten up the interrupt path

[K. Y. Srinivasan]

On the interrupt path, we repeatedly establish the pointer to the
storvsc_device. Fix this.

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 |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 6f18e94..8ba9908 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -958,19 +958,16 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
 }
 
 
-static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
+static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request,
+				       struct storvsc_device *stor_dev)
 {
 	struct scsi_cmnd *scmnd = cmd_request->cmd;
-	struct hv_host_device *host_dev = shost_priv(scmnd->device->host);
 	struct scsi_sense_hdr sense_hdr;
 	struct vmscsi_request *vm_srb;
 	struct Scsi_Host *host;
-	struct storvsc_device *stor_dev;
-	struct hv_device *dev = host_dev->dev;
 	u32 payload_sz = cmd_request->payload_sz;
 	void *payload = cmd_request->payload;
 
-	stor_dev = get_in_stor_device(dev);
 	host = stor_dev->host;
 
 	vm_srb = &cmd_request->vstor_packet.vm_srb;
@@ -1000,14 +997,13 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
 		kfree(payload);
 }
 
-static void storvsc_on_io_completion(struct hv_device *device,
+static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 				  struct vstor_packet *vstor_packet,
 				  struct storvsc_cmd_request *request)
 {
-	struct storvsc_device *stor_device;
 	struct vstor_packet *stor_pkt;
+	struct hv_device *device = stor_device->device;
 
-	stor_device = hv_get_drvdata(device);
 	stor_pkt = &request->vstor_packet;
 
 	/*
@@ -1062,7 +1058,7 @@ static void storvsc_on_io_completion(struct hv_device *device,
 	stor_pkt->vm_srb.data_transfer_length =
 	vstor_packet->vm_srb.data_transfer_length;
 
-	storvsc_command_completion(request);
+	storvsc_command_completion(request, stor_device);
 
 	if (atomic_dec_and_test(&stor_device->num_outstanding_req) &&
 		stor_device->drain_notify)
@@ -1071,21 +1067,19 @@ static void storvsc_on_io_completion(struct hv_device *device,
 
 }
 
-static void storvsc_on_receive(struct hv_device *device,
+static void storvsc_on_receive(struct storvsc_device *stor_device,
 			     struct vstor_packet *vstor_packet,
 			     struct storvsc_cmd_request *request)
 {
 	struct storvsc_scan_work *work;
-	struct storvsc_device *stor_device;
 
 	switch (vstor_packet->operation) {
 	case VSTOR_OPERATION_COMPLETE_IO:
-		storvsc_on_io_completion(device, vstor_packet, request);
+		storvsc_on_io_completion(stor_device, vstor_packet, request);
 		break;
 
 	case VSTOR_OPERATION_REMOVE_DEVICE:
 	case VSTOR_OPERATION_ENUMERATE_BUS:
-		stor_device = get_in_stor_device(device);
 		work = kmalloc(sizeof(struct storvsc_scan_work), GFP_ATOMIC);
 		if (!work)
 			return;
@@ -1096,7 +1090,6 @@ static void storvsc_on_receive(struct hv_device *device,
 		break;
 
 	case VSTOR_OPERATION_FCHBA_DATA:
-		stor_device = get_in_stor_device(device);
 		cache_wwn(stor_device, vstor_packet);
 		fc_host_node_name(stor_device->host) = stor_device->node_name;
 		fc_host_port_name(stor_device->host) = stor_device->port_name;
@@ -1144,7 +1137,7 @@ static void storvsc_on_channel_callback(void *context)
 					vmscsi_size_delta));
 				complete(&request->wait_event);
 			} else {
-				storvsc_on_receive(device,
+				storvsc_on_receive(stor_device,
 						(struct vstor_packet *)packet,
 						request);
 			}
-- 
1.7.4.1

Re: [PATCH 4/4] scsi: storvsc: Tighten up the interrupt path

[Johannes Thumshirn]

On Thu, Dec 10, 2015 at 04:14:20PM -0800, K. Y. Srinivasan wrote:
> On the interrupt path, we repeatedly establish the pointer to the
> storvsc_device. Fix this.
> 
> 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 |   23 ++++++++---------------
>  1 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 6f18e94..8ba9908 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -958,19 +958,16 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
>  }
>  
>  
> -static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
> +static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request,
> +				       struct storvsc_device *stor_dev)
>  {
>  	struct scsi_cmnd *scmnd = cmd_request->cmd;
> -	struct hv_host_device *host_dev = shost_priv(scmnd->device->host);
>  	struct scsi_sense_hdr sense_hdr;
>  	struct vmscsi_request *vm_srb;
>  	struct Scsi_Host *host;
> -	struct storvsc_device *stor_dev;
> -	struct hv_device *dev = host_dev->dev;
>  	u32 payload_sz = cmd_request->payload_sz;
>  	void *payload = cmd_request->payload;
>  
> -	stor_dev = get_in_stor_device(dev);
>  	host = stor_dev->host;
>  
>  	vm_srb = &cmd_request->vstor_packet.vm_srb;
> @@ -1000,14 +997,13 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
>  		kfree(payload);
>  }
>  
> -static void storvsc_on_io_completion(struct hv_device *device,
> +static void storvsc_on_io_completion(struct storvsc_device *stor_device,
>  				  struct vstor_packet *vstor_packet,
>  				  struct storvsc_cmd_request *request)
>  {
> -	struct storvsc_device *stor_device;
>  	struct vstor_packet *stor_pkt;
> +	struct hv_device *device = stor_device->device;
>  
> -	stor_device = hv_get_drvdata(device);
>  	stor_pkt = &request->vstor_packet;
>  
>  	/*
> @@ -1062,7 +1058,7 @@ static void storvsc_on_io_completion(struct hv_device *device,
>  	stor_pkt->vm_srb.data_transfer_length =
>  	vstor_packet->vm_srb.data_transfer_length;
>  
> -	storvsc_command_completion(request);
> +	storvsc_command_completion(request, stor_device);
>  
>  	if (atomic_dec_and_test(&stor_device->num_outstanding_req) &&
>  		stor_device->drain_notify)
> @@ -1071,21 +1067,19 @@ static void storvsc_on_io_completion(struct hv_device *device,
>  
>  }
>  
> -static void storvsc_on_receive(struct hv_device *device,
> +static void storvsc_on_receive(struct storvsc_device *stor_device,
>  			     struct vstor_packet *vstor_packet,
>  			     struct storvsc_cmd_request *request)
>  {
>  	struct storvsc_scan_work *work;
> -	struct storvsc_device *stor_device;
>  
>  	switch (vstor_packet->operation) {
>  	case VSTOR_OPERATION_COMPLETE_IO:
> -		storvsc_on_io_completion(device, vstor_packet, request);
> +		storvsc_on_io_completion(stor_device, vstor_packet, request);
>  		break;
>  
>  	case VSTOR_OPERATION_REMOVE_DEVICE:
>  	case VSTOR_OPERATION_ENUMERATE_BUS:
> -		stor_device = get_in_stor_device(device);
>  		work = kmalloc(sizeof(struct storvsc_scan_work), GFP_ATOMIC);
>  		if (!work)
>  			return;
> @@ -1096,7 +1090,6 @@ static void storvsc_on_receive(struct hv_device *device,
>  		break;
>  
>  	case VSTOR_OPERATION_FCHBA_DATA:
> -		stor_device = get_in_stor_device(device);
>  		cache_wwn(stor_device, vstor_packet);
>  		fc_host_node_name(stor_device->host) = stor_device->node_name;
>  		fc_host_port_name(stor_device->host) = stor_device->port_name;
> @@ -1144,7 +1137,7 @@ static void storvsc_on_channel_callback(void *context)
>  					vmscsi_size_delta));
>  				complete(&request->wait_event);
>  			} else {
> -				storvsc_on_receive(device,
> +				storvsc_on_receive(stor_device,
>  						(struct vstor_packet *)packet,
>  						request);
>  			}
> -- 
> 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

Re: [PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet

[Johannes Thumshirn]

On Thu, Dec 10, 2015 at 04:14:17PM -0800, K. Y. Srinivasan wrote:
> The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in Linux match
> the Window's definition.
> 
> 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 |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index c41f674..00bb4bd 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -92,9 +92,8 @@ enum vstor_packet_operation {
>   */
>  
>  struct hv_fc_wwn_packet {
> -	bool	primary_active;
> -	u8	reserved1;
> -	u8	reserved2;
> +	u8	primary_active;
> +	u8	reserved1[3];
>  	u8	primary_port_wwn[8];
>  	u8	primary_node_wwn[8];
>  	u8	secondary_port_wwn[8];
> -- 
> 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

Should this go through stable as well?
If yes:
Fixes: 8b612fa23 '[SCSI] storvsc: Update the storage protocol to win8 level'
Cc: stable@vger.kernel.org # v3.11+
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

RE: [PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet

[KY Srinivasan]

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn@suse.de]
> Sent: Friday, December 11, 2015 12:48 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org;
> apw@canonical.com; vkuznets@redhat.com; jasowang@redhat.com;
> martin.petersen@oracle.com
> Subject: Re: [PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the
> hv_fc_wwn_packet
> 
> On Thu, Dec 10, 2015 at 04:14:17PM -0800, K. Y. Srinivasan wrote:
> > The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in
> Linux match
> > the Window's definition.
> >
> > 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 |    5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index c41f674..00bb4bd 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -92,9 +92,8 @@ enum vstor_packet_operation {
> >   */
> >
> >  struct hv_fc_wwn_packet {
> > -	bool	primary_active;
> > -	u8	reserved1;
> > -	u8	reserved2;
> > +	u8	primary_active;
> > +	u8	reserved1[3];
> >  	u8	primary_port_wwn[8];
> >  	u8	primary_node_wwn[8];
> >  	u8	secondary_port_wwn[8];
> > --
> > 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
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.ker
> nel.org%2fmajordomo-
> info.html&data=01%7c01%7ckys%40microsoft.com%7cb6c3be0d1b474ac1374
> 908d30207c170%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=7hfMcn
> qW32KiJgr7JkXsC4P0CnnQvAearwu7LzyWaqM%3d
> 
> Should this go through stable as well?
> If yes:
> Fixes: 8b612fa23 '[SCSI] storvsc: Update the storage protocol to win8 level'
> Cc: stable@vger.kernel.org # v3.11+
This is the first time we are using this data structure and so I don't think we need
to port it back to stable.

Regards,

K. Y

> 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