Message-ID: <BY2PR0301MB16543EB4E7BB5CF66FCF0071A0E10@BY2PR0301MB1654.namprd03.prod.outlook.com>
Date:	Fri, 18 Dec 2015 17:13:14 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	Hannes Reinecke <hare@...e.de>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"ohering@...e.com" <ohering@...e.com>,
	"jbottomley@...allels.com" <jbottomley@...allels.com>,
	"hch@...radead.org" <hch@...radead.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"apw@...onical.com" <apw@...onical.com>,
	"vkuznets@...hat.com" <vkuznets@...hat.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	"martin.petersen@...cle.com" <martin.petersen@...cle.com>
Subject: RE: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
 devices



> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@...e.de]
> Sent: Friday, December 18, 2015 12:49 AM
> To: KY Srinivasan <kys@...rosoft.com>; gregkh@...uxfoundation.org; linux-
> kernel@...r.kernel.org; devel@...uxdriverproject.org; ohering@...e.com;
> jbottomley@...allels.com; hch@...radead.org; linux-scsi@...r.kernel.org;
> apw@...onical.com; vkuznets@...hat.com; jasowang@...hat.com;
> martin.petersen@...cle.com
> Subject: Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
> devices
> 
> On 12/13/2015 09:28 PM, 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@...rosoft.com>
> > Reviewed-by: Long Li <longli@...rosoft.com>
> > Tested-by: Alex Ng <alexng@...rosoft.com>
> > ---
> > 	V2: Fixed error paths - Dan Carpenter <dan.carpenter@...cle.com>
> > 	V3: Fixed build issues reported by kbuild test robot <lkp@...el.com>
> >
> >   drivers/scsi/storvsc_drv.c |  181
> ++++++++++++++++++++++++++++++++-----------
> >   1 files changed, 134 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 00bb4bd..220b794 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,9 @@ static int storvsc_timeout = 180;
> >
> >   static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +static struct scsi_transport_template *fc_transport_template;
> > +#endif
> >
> >   static void storvsc_on_channel_callback(void *context);
> >
> > @@ -456,6 +460,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 +704,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;
> > @@ -727,19 +755,15 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >   			       VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> >   	if (ret != 0)
> > -		goto cleanup;
> > +		return ret;
> >
> >   	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > -	if (t == 0) {
> > -		ret = -ETIMEDOUT;
> > -		goto cleanup;
> > -	}
> > +	if (t == 0)
> > +		return -ETIMEDOUT;
> >
> >   	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > -	    vstor_packet->status != 0) {
> > -		ret = -EINVAL;
> > -		goto cleanup;
> > -	}
> > +	    vstor_packet->status != 0)
> > +		return -EINVAL;
> >
> >
> >   	for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> > @@ -764,18 +788,14 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >   			       VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> >   		if (ret != 0)
> > -			goto cleanup;
> > +			return ret;
> >
> >   		t = wait_for_completion_timeout(&request->wait_event,
> 5*HZ);
> > -		if (t == 0) {
> > -			ret = -ETIMEDOUT;
> > -			goto cleanup;
> > -		}
> > +		if (t == 0)
> > +			return -ETIMEDOUT;
> >
> > -		if (vstor_packet->operation !=
> VSTOR_OPERATION_COMPLETE_IO) {
> > -			ret = -EINVAL;
> > -			goto cleanup;
> > -		}
> > +		if (vstor_packet->operation !=
> VSTOR_OPERATION_COMPLETE_IO)
> > +			return -EINVAL;
> >
> >   		if (vstor_packet->status == 0) {
> >   			vmstor_proto_version =
> > @@ -791,10 +811,8 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >   		}
> >   	}
> >
> > -	if (vstor_packet->status != 0) {
> > -		ret = -EINVAL;
> > -		goto cleanup;
> > -	}
> > +	if (vstor_packet->status != 0)
> > +		return -EINVAL;
> >
> >
> >   	memset(vstor_packet, 0, sizeof(struct vstor_packet));
> > @@ -809,19 +827,15 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> >
> >   	if (ret != 0)
> > -		goto cleanup;
> > +		return ret;
> >
> >   	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > -	if (t == 0) {
> > -		ret = -ETIMEDOUT;
> > -		goto cleanup;
> > -	}
> > +	if (t == 0)
> > +		return -ETIMEDOUT;
> >
> >   	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > -	    vstor_packet->status != 0) {
> > -		ret = -EINVAL;
> > -		goto cleanup;
> > -	}
> > +	    vstor_packet->status != 0)
> > +		return -EINVAL;
> >
> >   	/*
> >   	 * Check to see if multi-channel support is there.
> > @@ -837,6 +851,38 @@ 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)
> > +		return ret;
> > +
> > +	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > +	if (t == 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > +	    vstor_packet->status != 0)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * 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;
> > @@ -849,25 +895,19 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> >
> >   	if (ret != 0)
> > -		goto cleanup;
> > +		return ret;
> >
> >   	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > -	if (t == 0) {
> > -		ret = -ETIMEDOUT;
> > -		goto cleanup;
> > -	}
> > +	if (t == 0)
> > +		return -ETIMEDOUT;
> >
> >   	if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > -	    vstor_packet->status != 0) {
> > -		ret = -EINVAL;
> > -		goto cleanup;
> > -	}
> > +	    vstor_packet->status != 0)
> > +		return -EINVAL;
> >
> >   	if (process_sub_channels)
> >   		handle_multichannel_storage(device, max_chns);
> >
> > -
> > -cleanup:
> >   	return ret;
> >   }
> >
> > @@ -1076,6 +1116,14 @@ 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);
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +		fc_host_node_name(stor_device->host) = stor_device-
> >node_name;
> > +		fc_host_port_name(stor_device->host) = stor_device-
> >port_name;
> > +#endif
> > +		break;
> >   	default:
> >   		break;
> >   	}
> > @@ -1131,7 +1179,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 +1197,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 +1622,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 +1680,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 +1692,9 @@ 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;
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +		host->transportt = fc_transport_template;
> > +#endif
> >   		break;
> >
> >   	case SCSI_GUID:
> > @@ -1681,6 +1734,12 @@ static int storvsc_probe(struct hv_device
> *device,
> >   			goto err_out2;
> >   		}
> >   	}
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +	if (host->transportt == fc_transport_template) {
> > +		fc_host_node_name(host) = stor_device->node_name;
> > +		fc_host_port_name(host) = stor_device->port_name;
> > +	}
> > +#endif
> >   	return 0;
> >
> >   err_out2:
> > @@ -1706,6 +1765,10 @@ static int storvsc_remove(struct hv_device
> *dev)
> >   	struct storvsc_device *stor_device = hv_get_drvdata(dev);
> >   	struct Scsi_Host *host = stor_device->host;
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +	if (host->transportt == fc_transport_template)
> > +		fc_remove_host(host);
> > +#endif
> >   	scsi_remove_host(host);
> >   	storvsc_dev_remove(dev);
> >   	scsi_host_put(host);
> > @@ -1720,8 +1783,16 @@ static struct hv_driver storvsc_drv = {
> >   	.remove = storvsc_remove,
> >   };
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +static struct fc_function_template fc_transport_functions = {
> > +	.show_host_node_name = 1,
> > +	.show_host_port_name = 1,
> > +};
> > +#endif
> > +
> >   static int __init storvsc_drv_init(void)
> >   {
> > +	int ret;
> >
> >   	/*
> >   	 * Divide the ring buffer data size (which is 1 page less
> > @@ -1736,12 +1807,28 @@ static int __init storvsc_drv_init(void)
> >   		vmscsi_size_delta,
> >   		sizeof(u64)));
> >
> > -	return vmbus_driver_register(&storvsc_drv);
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +	fc_transport_template =
> fc_attach_transport(&fc_transport_functions);
> > +	if (!fc_transport_template)
> > +		return -ENODEV;
> > +#endif
> > +
> > +	ret = vmbus_driver_register(&storvsc_drv);
> > +
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +	if (ret)
> > +		fc_release_transport(fc_transport_template);
> > +#endif
> > +
> > +	return ret;
> >   }
> >
> >   static void __exit storvsc_drv_exit(void)
> >   {
> >   	vmbus_driver_unregister(&storvsc_drv);
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +	fc_release_transport(fc_transport_template);
> > +#endif
> >   }
> >
> >   MODULE_LICENSE("GPL");
> >
> Well.
> This would _always_ attach the FC template to the storvsc driver,
> even if it not used. Typically one would be using a separate driver
> for that, but hey.
> 
> _However_: How should you handle FC attached devices if the FC
> transport template is _NOT_ selected?
> By rights one would expect the driver to not handle those devices;
> but looking at the code this doesn't happen.
> 
> What I would like to see is a clear separation here:
> - Disable FC disk handling if FC attributes are not configured
> - Add a module parameter allowing to disable FC attributes even if
> they are compiled in. Remember: this is a virtualized guest, and
> people might want so save kernel memory wherever they can. So always
> attaching to the fc transport template will make them very unhappy.
> Alternatively you could split out FC device handling into a separate
> driver, but seeing the diff that's probably overkill.

Hannes,
Another option might be to allocate the FC transport template in the probe call
when we are presented with a FC device (this would still be under the appropriate
config option). This way, when no FC device is configured for the guest, we don't
allocate FC transport template.

Regards,

K. Y 
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke		               zSeries & Storage
> hare@...e.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists