RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V

[KY Srinivasan]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Sunday, March 20, 2016 11:59 AM
> To: 'James Bottomley' <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>; Martin
> K. Petersen <martin.petersen@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> ohering@xxxxxxxx; jbottomley@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx;
> apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx;
> hare@xxxxxxx
> Subject: RE: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> Hyper-V
> 
> 
> 
> > -----Original Message-----
> > From: James Bottomley
> [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx]
> > Sent: Friday, March 18, 2016 3:41 PM
> > To: KY Srinivasan <kys@xxxxxxxxxxxxx>; Martin K. Petersen
> > <martin.petersen@xxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> > ohering@xxxxxxxx; jbottomley@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx;
> > apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx;
> > hare@xxxxxxx
> > Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on
> > Hyper-V
> >
> > On Thu, 2016-03-17 at 00:01 +0000, KY Srinivasan wrote:
> > > The only attributes I would be interested are:
> > > 1) node name
> > > 2) port name
> > >
> > > Ideally, if this can show under /sys/class/fc_host/hostx/port_name
> > > and node_name,
> > > it will be ideal since all user scripts can work.
> >
> > OK, like this?
> 
> Yes; thank you very much James. Looking at the patch though, it may be an
> overkill considering how much of the code is duplicated. The current fc
> transport
> class does give us the flexibility to control the attributes we want to surface
> (fc_function_template). In any case I will test this code and get back to you
> soon.

Today I got a chance to test this patch. Looks like all the state is not getting
properly set in this new transport class. I am hitting this NULL pointer reference fault in
get_device_parent(). Looks like the device class is not properly setup for
this transport class. class_dir_create_and_add() is not called for this class
and so the glue_dirs is NULL.   I fixed the issue:

1) You will need to call the transport_class_register() for this new transport class in
fc_transport_init()
2) We cannot use fc_host as the name in this new class since the standard fc transport already
Uses that name. I changed the name to get this to work. This will create a new directory under /sys/class.
So my original goal of  being compatible with existing scripts that expect to find the information under
/sys/class/fc_host will not be met.

Regards,

K. Y


> 
> Regards,
> 
> K. Y
> 
> 
> >
> > From 7af7c428e7e04ddcc87fda12d6571e3dff8ae024 Mon Sep 17 00:00:00
> > 2001
> > From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > Date: Fri, 18 Mar 2016 15:35:45 -0700
> > Subject: scsi_transport_fc: introduce lightweight class for virtualization
> >  systems
> >
> > The FC transport class is very heavily tilted towards helping things
> > which operate a fabric (as it should be).  However, there seems to be
> > a need for a lightweight version for use in virtual systems that
> > simply want to show pass through FC information without making any use
> > of the heavyweight functions.  This is an attempt to give them what
> > they want: the lightweight class has no vports or rports and only two
> > host attributes.  Essentially, it's designed for the HV storvsc
> > driver, but if other virtualizataion systems have similar problems, we
> > can add more attributes.
> >
> > Signed-off-by: James Bottomley <jejb@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/scsi/scsi_transport_fc.c | 94
> > ++++++++++++++++++++++++++++++++++++++++
> >  include/scsi/scsi_transport_fc.h |  3 ++
> >  2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/scsi/scsi_transport_fc.c
> b/drivers/scsi/scsi_transport_fc.c
> > index 8a88226..a9fcb4d 100644
> > --- a/drivers/scsi/scsi_transport_fc.c
> > +++ b/drivers/scsi/scsi_transport_fc.c
> > @@ -351,6 +351,27 @@ struct fc_internal {
> >
> >  #define to_fc_internal(tmpl)	container_of(tmpl, struct fc_internal, t)
> >
> > +#define FC_LW_HOST_NUM_ATTRS 	2
> > +struct fc_lw_internal {
> > +	struct scsi_transport_template t;
> > +	struct fc_function_template *f;
> > +
> > +	/*
> > +	 * For attributes : each object has :
> > +	 *   An array of the actual attributes structures
> > +	 *   An array of null-terminated pointers to the attribute
> > +	 *     structures - used for mid-layer interaction.
> > +	 *
> > +	 * The attribute containers for the starget and host are are
> > +	 * part of the midlayer. As the remote port is specific to the
> > +	 * fc transport, we must provide the attribute container.
> > +	 */
> > +	struct device_attribute
> > private_host_attrs[FC_LW_HOST_NUM_ATTRS];
> > +	struct device_attribute *host_attrs[FC_LW_HOST_NUM_ATTRS + 1];
> > +};
> > +
> > +#define to_fc_lw_internal(tmpl)	container_of(tmpl, struct
> > fc_lw_internal, t)
> > +
> >  static int fc_target_setup(struct transport_container *tc, struct device
> *dev,
> >  			   struct device *cdev)
> >  {
> > @@ -472,6 +493,12 @@ static int fc_host_remove(struct
> transport_container
> > *tc, struct device *dev,
> >  	return 0;
> >  }
> >
> > +static DECLARE_TRANSPORT_CLASS(fc_lw_host_class,
> > +			       "fc_host",
> > +			       NULL,
> > +			       NULL,
> > +			       NULL);
> > +
> >  static DECLARE_TRANSPORT_CLASS(fc_host_class,
> >  			       "fc_host",
> >  			       fc_host_setup,
> > @@ -1968,6 +1995,25 @@ static int fc_host_match(struct
> > attribute_container *cont,
> >  	return &i->t.host_attrs.ac == cont;
> >  }
> >
> > +static int fc_lw_host_match(struct attribute_container *cont,
> > +			  struct device *dev)
> > +{
> > +	struct Scsi_Host *shost;
> > +	struct fc_lw_internal *i;
> > +
> > +	if (!scsi_is_host_device(dev))
> > +		return 0;
> > +
> > +	shost = dev_to_shost(dev);
> > +	if (!shost->transportt  || shost->transportt->host_attrs.ac.class
> > +	    != &fc_lw_host_class.class)
> > +		return 0;
> > +
> > +	i = to_fc_lw_internal(shost->transportt);
> > +
> > +	return &i->t.host_attrs.ac == cont;
> > +}
> > +
> >  static int fc_target_match(struct attribute_container *cont,
> >  			    struct device *dev)
> >  {
> > @@ -2171,6 +2217,54 @@ static int fc_it_nexus_response(struct Scsi_Host
> > *shost, u64 nexus, int result)
> >  	return i->f->it_nexus_response(shost, nexus, result);
> >  }
> >
> > +/**
> > + * fc_attach_lw_transport - light weight attach function
> > + * @ft:		function template for optional attributes
> > + *
> > + * This attach function is to be used only for virtual FC emulators
> > + * which do not have a physical fabric underneath them and thus only
> > + * need a few attributes and no helper functions
> > + */
> > +struct scsi_transport_template *
> > +fc_lw_attach_transport(struct fc_function_template *ft)
> > +{
> > +	int count;
> > +	struct fc_lw_internal *i = kzalloc(sizeof(struct fc_lw_internal),
> > +					   GFP_KERNEL);
> > +
> > +	if (unlikely(!i))
> > +		return NULL;
> > +
> > +	i->t.host_attrs.ac.attrs = &i->host_attrs[0];
> > +	i->t.host_attrs.ac.class = &fc_lw_host_class.class;
> > +	i->t.host_attrs.ac.match = fc_lw_host_match;
> > +	i->t.host_size = sizeof(struct fc_host_attrs);
> > +	transport_container_register(&i->t.host_attrs);
> > +
> > +	i->f = ft;
> > +
> > +	count = 0;
> > +	SETUP_HOST_ATTRIBUTE_RD(node_name);
> > +	SETUP_HOST_ATTRIBUTE_RD(port_name);
> > +
> > +	BUG_ON(count > FC_HOST_NUM_ATTRS);
> > +
> > +	i->host_attrs[count] = NULL;
> > +
> > +	return &i->t;
> > +}
> > +EXPORT_SYMBOL(fc_lw_attach_transport);
> > +
> > +void fc_lw_release_transport(struct scsi_transport_template *t)
> > +{
> > +	struct fc_lw_internal *i = to_fc_lw_internal(t);
> > +
> > +	transport_container_unregister(&i->t.host_attrs);
> > +
> > +	kfree(i);
> > +}
> > +EXPORT_SYMBOL(fc_lw_release_transport);
> > +
> >  struct scsi_transport_template *
> >  fc_attach_transport(struct fc_function_template *ft)
> >  {
> > diff --git a/include/scsi/scsi_transport_fc.h
> b/include/scsi/scsi_transport_fc.h
> > index 784bc2c..b0a9a64 100644
> > --- a/include/scsi/scsi_transport_fc.h
> > +++ b/include/scsi/scsi_transport_fc.h
> > @@ -835,6 +835,9 @@ fc_vport_set_state(struct fc_vport *vport, enum
> > fc_vport_state new_state)
> >  	vport->vport_state = new_state;
> >  }
> >
> > +struct scsi_transport_template *fc_lw_attach_transport(
> > +			struct fc_function_template *);
> > +void fc_lw_release_transport(struct scsi_transport_template *);
> >  struct scsi_transport_template *fc_attach_transport(
> >  			struct fc_function_template *);
> >  void fc_release_transport(struct scsi_transport_template *);
> > --
> > 2.6.2