----- Original Message -----
> From: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> To: "Fam Zheng" <famz@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx, "Paolo Bonzini" <pbonzini@xxxxxxxxxx>, linux-scsi@xxxxxxxxxxxxxxx, "James E.J.
> Bottomley" <jejb@xxxxxxxxxxxxxxxxxx>, "Jason Wang" <jasowang@xxxxxxxxxx>, "Martin K. Petersen"
> <martin.petersen@xxxxxxxxxx>, stefanha@xxxxxxxxxx, virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Sent: Thursday, January 26, 2017 11:06:27 PM
> Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host
>
> On Thu, Jan 26, 2017 at 11:41:09AM +0800, Fam Zheng wrote:
> > This implements the VIRTIO_SCSI_F_FC_HOST feature by reading the config
> > fields and presenting them as sysfs fc_host attributes. The config
> > change handler is added here because primary_active will toggle during
> > migration.
>
> Looks like there's active discussion on virtio tc mailing list.
> It's ok to post patches meanwhile but best as RFC,
> and repost after controversy is resolved.
Discussion on the TC mailing list was not about the merit of the feature,
only about the timing of the vote.
Paolo
>
> >
> > Signed-off-by: Fam Zheng <famz@xxxxxxxxxx>
> > ---
> > drivers/scsi/virtio_scsi.c | 60
> > +++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> > index ec91bd0..1bb330c 100644
> > --- a/drivers/scsi/virtio_scsi.c
> > +++ b/drivers/scsi/virtio_scsi.c
> > @@ -28,6 +28,7 @@
> > #include <scsi/scsi_device.h>
> > #include <scsi/scsi_cmnd.h>
> > #include <scsi/scsi_tcq.h>
> > +#include <scsi/scsi_transport_fc.h>
> > #include <linux/seqlock.h>
> >
> > #define VIRTIO_SCSI_MEMPOOL_SZ 64
> > @@ -795,6 +796,15 @@ static struct scsi_host_template
> > virtscsi_host_template_multi = {
> > .track_queue_depth = 1,
> > };
> >
> > +static struct fc_function_template virtscsi_fc_template = {
> > + .show_host_node_name = 1,
> > + .show_host_port_name = 1,
> > + .show_host_port_type = 1,
> > + .show_host_port_state = 1,
> > +};
> > +
> > +static struct scsi_transport_template *virtscsi_fc_transport_template;
> > +
> > #define virtscsi_config_get(vdev, fld) \
> > ({ \
> > typeof(((struct virtio_scsi_config *)0)->fld) __val; \
> > @@ -956,15 +966,42 @@ static int virtscsi_init(struct virtio_device *vdev,
> > return err;
> > }
> >
> > +static void virtscsi_update_fc_host_attrs(struct virtio_device *vdev)
> > +{
> > + struct Scsi_Host *shost = vdev->priv;
> > + u8 node_name[8], port_name[8];
> > +
> > + if (virtscsi_config_get(vdev, primary_active)) {
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, primary_wwnn),
> > + &node_name, 8);
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, primary_wwpn),
> > + &port_name, 8);
> > + } else {
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, secondary_wwnn),
> > + &node_name, 8);
> > + virtio_cread_bytes(vdev,
> > + offsetof(struct virtio_scsi_config, secondary_wwpn),
> > + &port_name, 8);
> > + }
>
> This is racy, isn't it? You need to wrap this in a generation check
> otherwise read can race with primary_active changing.
> And you might want a wrapper to virtio_cread_bytes that does not
> include generation check.
>
> > + fc_host_node_name(shost) = wwn_to_u64(node_name);
> > + fc_host_port_name(shost) = wwn_to_u64(port_name);
> > + fc_host_port_type(shost) = FC_PORTTYPE_NPORT;
> > + fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
> > +}
> > +
> > static int virtscsi_probe(struct virtio_device *vdev)
> > {
> > - struct Scsi_Host *shost;
> > + struct Scsi_Host *shost = NULL;
> > struct virtio_scsi *vscsi;
> > int err;
> > u32 sg_elems, num_targets;
> > u32 cmd_per_lun;
> > u32 num_queues;
> > struct scsi_host_template *hostt;
> > + bool fc_host_enabled;
> >
> > if (!vdev->config->get) {
> > dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > @@ -987,6 +1024,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
> > if (!shost)
> > return -ENOMEM;
> >
> > + fc_host_enabled = virtio_has_feature(vdev, VIRTIO_SCSI_F_FC_HOST);
> > + if (fc_host_enabled)
> > + shost->transportt = virtscsi_fc_transport_template;
> > sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
> > shost->sg_tablesize = sg_elems;
> > vscsi = shost_priv(shost);
> > @@ -1032,6 +1072,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
> > if (err)
> > goto scsi_add_host_failed;
> >
> > + if (fc_host_enabled)
> > + virtscsi_update_fc_host_attrs(vdev);
> > +
> > virtio_device_ready(vdev);
> >
> > if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
> > @@ -1098,6 +1141,11 @@ static int virtscsi_restore(struct virtio_device
> > *vdev)
> > }
> > #endif
> >
> > +static void virtscsi_config_changed(struct virtio_device *vdev)
> > +{
>
> This is called unconditionally here, will access
> invalid config fields if feature is off.
> In fact this is wasting an MSIX vector when feature is not
> negotiated. No easy way not to, but best document this
> in a code comment.
>
> > + virtscsi_update_fc_host_attrs(vdev);
> > +}
> > +
> > static struct virtio_device_id id_table[] = {
> > { VIRTIO_ID_SCSI, VIRTIO_DEV_ANY_ID },
> > { 0 },
> > @@ -1109,6 +1157,7 @@ static unsigned int features[] = {
> > #ifdef CONFIG_BLK_DEV_INTEGRITY
> > VIRTIO_SCSI_F_T10_PI,
> > #endif
> > + VIRTIO_SCSI_F_FC_HOST,
> > };
> >
> > static struct virtio_driver virtio_scsi_driver = {
> > @@ -1123,12 +1172,20 @@ static struct virtio_driver virtio_scsi_driver = {
> > .restore = virtscsi_restore,
> > #endif
> > .remove = virtscsi_remove,
> > + .config_changed = virtscsi_config_changed,
> > };
> >
> > static int __init init(void)
> > {
> > int ret = -ENOMEM;
> >
> > + virtscsi_fc_transport_template =
> > + fc_attach_transport(&virtscsi_fc_template);
> > + if (!virtscsi_fc_transport_template) {
> > + pr_err("fc_attach_transport() failed\n");
> > + goto error;
> > + }
> > +
> > virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
> > if (!virtscsi_cmd_cache) {
> > pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
> > @@ -1176,6 +1233,7 @@ static int __init init(void)
> >
> > static void __exit fini(void)
> > {
> > + fc_release_transport(virtscsi_fc_transport_template);
> > unregister_virtio_driver(&virtio_scsi_driver);
> > cpuhp_remove_multi_state(virtioscsi_online);
> > cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);
> > --
> > 2.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
