[PATCH RFC 1/3] add fc transport events

[PATCH RFC 1/3] add fc transport events

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

01-add-host-transport-classdev.patch - adds the transport class_device to
the scsi_host structure.

  drivers/scsi/hosts.c     |   22 +++++++++++++++++++++-
  include/scsi/scsi_host.h |    4 ++++
  2 files changed, 25 insertions(+), 1 deletion(-)

[-- Attachment #2: 01-add-host-transport-classdev.patch --]
[-- Type: text/plain, Size: 2698 bytes --]

diff -arup linux-2.6.7-rc1/drivers/scsi/hosts.c linux-2.6.7-rc1-hotplug/drivers/scsi/hosts.c
--- linux-2.6.7-rc1/drivers/scsi/hosts.c	2004-05-26 22:47:18.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/drivers/scsi/hosts.c	2004-05-26 23:17:30.217567849 -0700
@@ -83,6 +83,8 @@ void scsi_remove_host(struct Scsi_Host *
 
 	set_bit(SHOST_DEL, &shost->shost_state);
 
+	if (shost->transportt->class)
+		class_device_unregister(&shost->transport_classdev);
 	class_device_unregister(&shost->shost_classdev);
 	device_del(&shost->shost_gendev);
 }
@@ -123,15 +125,28 @@ int scsi_add_host(struct Scsi_Host *shos
 	if (error)
 		goto out_del_gendev;
 
+	if (shost->transportt->class) {
+		shost->transport_classdev.class = shost->transportt->class;
+
+		error = class_device_add(&shost->transport_classdev);
+		if (error)
+			goto out_del_classdev;
+		/* take a reference for the transport_classdev; this
+		 * is released by the transport_class .release */
+		get_device(&shost->shost_gendev);
+	}
+
 	get_device(&shost->shost_gendev);
 
 	error = scsi_sysfs_add_host(shost);
 	if (error)
-		goto out_del_classdev;
+		goto out_del_transport;
 
 	scsi_proc_host_add(shost);
 	return error;
 
+ out_del_transport:
+	class_device_del(&shost->transport_classdev);
  out_del_classdev:
 	class_device_del(&shost->shost_classdev);
  out_del_gendev:
@@ -280,6 +295,11 @@ struct Scsi_Host *scsi_host_alloc(struct
 	snprintf(shost->shost_classdev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
 
+	class_device_initialize(&shost->transport_classdev);
+	shost->transport_classdev.dev = &shost->shost_gendev;
+	snprintf(shost->transport_classdev.class_id, BUS_ID_SIZE, "host%d",
+		  shost->host_no);
+
 	shost->eh_notify = &complete;
 	rval = kernel_thread(scsi_error_handler, shost, 0);
 	if (rval < 0)
diff -arup linux-2.6.7-rc1/include/scsi/scsi_host.h linux-2.6.7-rc1-hotplug/include/scsi/scsi_host.h
--- linux-2.6.7-rc1/include/scsi/scsi_host.h	2004-05-26 22:47:18.000000000 -0700
+++ linux-2.6.7-rc1-hotplug/include/scsi/scsi_host.h	2004-05-26 23:17:08.000000000 -0700
@@ -475,6 +475,7 @@ struct Scsi_Host {
 	/* ldm bits */
 	struct device		shost_gendev;
 	struct class_device	shost_classdev;
+	struct class_device	transport_classdev;
 
 	/*
 	 * List of hosts per template.
@@ -497,6 +498,9 @@ struct Scsi_Host {
 	container_of(d, struct Scsi_Host, shost_gendev)
 #define		class_to_shost(d)	\
 	container_of(d, struct Scsi_Host, shost_classdev)
+#define transport_class_to_shost(_class_dev) \
+	container_of(_class_dev, struct Scsi_Host, transport_classdev)
+
 
 extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int scsi_add_host(struct Scsi_Host *, struct device *);

Re: [PATCH RFC 1/3] add fc transport events

[James Bottomley]

On Thu, 2004-05-27 at 03:25, Mike Christie wrote:
> 01-add-host-transport-classdev.patch - adds the transport class_device to
> the scsi_host structure.

I've been thinking about this quite a bit, and I like the general idea,
I just have a quibble with the implementation.

The main problem is the adding of a kobject to complement your host
transport class (and the device transport class).  I don't think we need
to do this since, like the transport classes fall into either host or
device objects, so the events are similarly divided; thus, we should be
able to trigger the hotplug events through the host or device kobject
(that come with the embedded struct device) rather than adding another
kobject for the purpose.

What would be the disadvantage of doing what you propose this way
instead of using a separate kobject?

James

Re: [PATCH RFC 1/3] add fc transport events

[Mike Christie]

James Bottomley wrote:
> On Thu, 2004-05-27 at 03:25, Mike Christie wrote:
> 
>>01-add-host-transport-classdev.patch - adds the transport class_device to
>>the scsi_host structure.
> 
> 
> I've been thinking about this quite a bit, and I like the general idea,
> I just have a quibble with the implementation.
> 
> The main problem is the adding of a kobject to complement your host
> transport class (and the device transport class).  I don't think we need

I am not adding a host and a device transport class. I am structuring 
things so there is a single fc transport class.

> to do this since, like the transport classes fall into either host or
> device objects, so the events are similarly divided; thus, we should be
> able to trigger the hotplug events through the host or device kobject
> (that come with the embedded struct device) rather than adding another
> kobject for the purpose.

The kobject I added to the scsi_device replaced the class_device (and 
its kobject) we were previously using for the device oriented transport 
class. I did this only because I wanted the scsi device's parent to be 
the host. In my patch, I then added a class_device to the host becuase 
the host's parent was the "FC Class".

There is no technical argument why they couldn't be coded the way you 
described. My patch just has the FC Class, where under it the device, 
host and whatever objects arise are set up as parent/children to reflect 
how SCSI/FC and the kernel structures really were.

It does not make a difference to me. It is easier to code just having a 
fc_host_transport_class and a fc_device_transport_class. It seemed like 
a waste to add more classes and doing the symlinks when you can just 
restructure things though.


Mike

Re: [PATCH RFC 1/3] add fc transport events

[Mike Christie]

Mike Christie wrote:

> James Bottomley wrote:
> 
>> On Thu, 2004-05-27 at 03:25, Mike Christie wrote:
>>
>>> 01-add-host-transport-classdev.patch - adds the transport 
>>> class_device to
>>> the scsi_host structure.
>>
>>
>>
>> I've been thinking about this quite a bit, and I like the general idea,
>> I just have a quibble with the implementation.
>>
>> The main problem is the adding of a kobject to complement your host
>> transport class (and the device transport class).  I don't think we need
> 
> 
> I am not adding a host and a device transport class. I am structuring 
> things so there is a single fc transport class.
> 
>> to do this since, like the transport classes fall into either host or
>> device objects, so the events are similarly divided; thus, we should be
>> able to trigger the hotplug events through the host or device kobject
>> (that come with the embedded struct device) rather than adding another
>> kobject for the purpose.
> 
> 
> The kobject I added to the scsi_device replaced the class_device (and 
> its kobject) we were previously using for the device oriented transport 
> class. I did this only because I wanted the scsi device's parent to be 
> the host. In my patch, I then added a class_device to the host becuase 
> the host's parent was the "FC Class".

Oh yeah, with this type of setup the host and device hotplug events were 
all coming through the single FC class. Host events can be distinguished 
from device events by their DEVPATH (one ends in scsiN the other ends 
with W:X:Y:Z and has the scsiN prefixed to it).

> There is no technical argument why they couldn't be coded the way you 
> described. My patch just has the FC Class, where under it the device, 
> host and whatever objects arise are set up as parent/children to reflect 
> how SCSI/FC and the kernel structures really were.
> 
> It does not make a difference to me. It is easier to code just having a 
> fc_host_transport_class and a fc_device_transport_class. It seemed like 
> a waste to add more classes and doing the symlinks when you can just 
> restructure things though.
> 
> 
> Mike
> -
> 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
> 

Re: [PATCH RFC 1/3] add fc transport events

[James Bottomley]

On Sun, 2004-06-13 at 16:44, Mike Christie wrote:
> I am not adding a host and a device transport class. I am structuring 
> things so there is a single fc transport class.

I don't think that's such a good idea.  A class is supposed to represent
an interface on a device.  The host and scsi device should have separate
interfaces.  The only reason we don't have any host interfaces in the
transport classes is because no-one has yet had a reason to add one. 
However, since the loop status is definitely a host property, you
do...part of what's missing is an attribute showing the loop state.  SPI
has a similar need; the host property there is LVD or SE, and we might
be interested in transitions between them.

> The kobject I added to the scsi_device replaced the class_device (and 
> its kobject) we were previously using for the device oriented transport 
> class. I did this only because I wanted the scsi device's parent to be 
> the host. In my patch, I then added a class_device to the host becuase 
> the host's parent was the "FC Class".
> 
> There is no technical argument why they couldn't be coded the way you 
> described. My patch just has the FC Class, where under it the device, 
> host and whatever objects arise are set up as parent/children to reflect 
> how SCSI/FC and the kernel structures really were.
> 
> It does not make a difference to me. It is easier to code just having a 
> fc_host_transport_class and a fc_device_transport_class. It seemed like 
> a waste to add more classes and doing the symlinks when you can just 
> restructure things though.

Well do it as two separate classes then.  Kobjects and Ksets are a bit
of a minefield that I'd rather we didn't get into unless it's really,
really necessary.  The abstraction we care about is the interface, which
to the generic device stuff is the class.  If we stick to what we need,
we'll get broken by generic device changes less often...

James

Re: [PATCH RFC 1/3] add fc transport events

[Mike Christie]

James Bottomley wrote:

> On Sun, 2004-06-13 at 16:44, Mike Christie wrote:
> 
>>I am not adding a host and a device transport class. I am structuring 
>>things so there is a single fc transport class.
> 
> 
> I don't think that's such a good idea.  A class is supposed to represent
> an interface on a device.

Ok. I saw the class as representing a "thingy" not necessarily a device, 
becuase at one point the mainatiner had taken out the coupling of the 
"struct device" to the class_device which I thought this was for. For 
example the io scheduler or request queue could and probably should be a 
class if you wanted to abe able to hot swap the schedluer. In my case I 
was representing the transport itself as a class (the class_device 
should have gone into the fc_transport object and a transport_kobject 
should have been placed into the host though).

   The host and scsi device should have separate
> interfaces.  The only reason we don't have any host interfaces in the
> transport classes is because no-one has yet had a reason to add one. 
> However, since the loop status is definitely a host property, you
> do...part of what's missing is an attribute showing the loop state.  SPI
> has a similar need; the host property there is LVD or SE, and we might
> be interested in transitions between them.
> 
> 
>>The kobject I added to the scsi_device replaced the class_device (and 
>>its kobject) we were previously using for the device oriented transport 
>>class. I did this only because I wanted the scsi device's parent to be 
>>the host. In my patch, I then added a class_device to the host becuase 
>>the host's parent was the "FC Class".
>>
>>There is no technical argument why they couldn't be coded the way you 
>>described. My patch just has the FC Class, where under it the device, 
>>host and whatever objects arise are set up as parent/children to reflect 
>>how SCSI/FC and the kernel structures really were.
>>
>>It does not make a difference to me. It is easier to code just having a 
>>fc_host_transport_class and a fc_device_transport_class. It seemed like 
>>a waste to add more classes and doing the symlinks when you can just 
>>restructure things though.
> 
> 
> Well do it as two separate classes then.  Kobjects and Ksets are a bit
> of a minefield that I'd rather we didn't get into unless it's really,
> really necessary.  The abstraction we care about is the interface, which
> to the generic device stuff is the class.  If we stick to what we need,
> we'll get broken by generic device changes less often...
> 

Ok then.

Will this mean we will end up with device and host specific error 
handler issues that the classes will have to coordinate? For example, if 
I got a link down I know that cmds are going to be timing out for every 
device on the host, but depending on the setup if just the device's port 
went down I will not get a link down and may want to do something 
different. Or maybe not?

Mike

Re: [PATCH RFC 1/3] add fc transport events

[Douglas Gilbert]

James Bottomley wrote:
> On Sun, 2004-06-13 at 16:44, Mike Christie wrote:
> 
>>I am not adding a host and a device transport class. I am structuring 
>>things so there is a single fc transport class.
> 
> 
> I don't think that's such a good idea.  A class is supposed to represent
> an interface on a device.  The host and scsi device should have separate
> interfaces.  The only reason we don't have any host interfaces in the
> transport classes is because no-one has yet had a reason to add one. 
> However, since the loop status is definitely a host property, you
> do...part of what's missing is an attribute showing the loop state.  SPI
> has a similar need; the host property there is LVD or SE, and we might
> be interested in transitions between them.

There are 4 categories of "devices" that we might try
to address:
   a) host (PCI side and SCSI transport side)
   b) transport service delivery subsystem
   c) target device server
   d) logical unit

Only a, b + c are associated with the transport in question.
In category b could be FC switches and SAS expanders. In
category d could be sATA, SPI or SAS disks behind a FC
target. So the logical unit doesn't necessarily belong to
the same transport as the initiator (especially in iSCSI)
as noted above by James.

So the end point of the transport is category c and according
to SPC-3 a target device server is addressed via "well known"
logical units (see section 8). [The standard requirement for a
device server to respond to lun==0 for INQUIRY and REPORT LUNS
is a related hack that allows a SCSI disk to double as a
target device server and a lu.]

Is our transport class design flexible enough for this level
of complexity?

Doug Gilbert

Re: [PATCH RFC 1/3] add fc transport events

[James Bottomley]

On Sun, 2004-06-13 at 22:15, Douglas Gilbert wrote:
> There are 4 categories of "devices" that we might try
> to address:
>    a) host (PCI side and SCSI transport side)
>    b) transport service delivery subsystem
>    c) target device server
>    d) logical unit

a) is really two (which we currently have represented) with the bus side
device and the host device.

b,c and d are all scsi devices.

> Only a, b + c are associated with the transport in question.
> In category b could be FC switches and SAS expanders. In
> category d could be sATA, SPI or SAS disks behind a FC
> target. So the logical unit doesn't necessarily belong to
> the same transport as the initiator (especially in iSCSI)
> as noted above by James.

The idea of the transport class is that it be invisible to the
mid-layer.  The services it provides are either to the LLD or to the
user (via sysfs).

> So the end point of the transport is category c and according
> to SPC-3 a target device server is addressed via "well known"
> logical units (see section 8). [The standard requirement for a
> device server to respond to lun==0 for INQUIRY and REPORT LUNS
> is a related hack that allows a SCSI disk to double as a
> target device server and a lu.]
> 
> Is our transport class design flexible enough for this level
> of complexity?

Well, in the SAM 4 level system:

1. Device specific command sets
2. Shared command sets (all device types)
3. SCSI transport protocols
4. Interconnects

Our ULDs do 1, the mid-layer does 2 and I'm hoping to expand the
transport classes into 3 and 4.

Enumeration really isn't addressed by SAM, but we're already moving
towards having enumeration done from user level (i.e. removing simple
scanning from the mid-layer), so that should be easily able to do
complex topology probes.

James