[PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
KY Srinivasan
kys at microsoft.com
Fri Dec 18 17:13:14 UTC 2015
> -----Original Message-----
> From: Hannes Reinecke [mailto:hare at suse.de]
> Sent: Friday, December 18, 2015 12:49 AM
> To: KY Srinivasan <kys at microsoft.com>; gregkh at linuxfoundation.org; linux-
> kernel at vger.kernel.org; devel at linuxdriverproject.org; ohering at suse.com;
> jbottomley at parallels.com; hch at infradead.org; linux-scsi at vger.kernel.org;
> apw at canonical.com; vkuznets at redhat.com; jasowang at redhat.com;
> martin.petersen at oracle.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 at microsoft.com>
> > Reviewed-by: Long Li <longli at microsoft.com>
> > Tested-by: Alex Ng <alexng at microsoft.com>
> > ---
> > V2: Fixed error paths - Dan Carpenter <dan.carpenter at oracle.com>
> > V3: Fixed build issues reported by kbuild test robot <lkp at intel.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 at suse.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)
More information about the devel
mailing list