Transport Attributes -- attempt#2

Quoted 4 lines

Scan we replacte the PSCSI/pscsi in identifiers with SPI/spi as used in the t10 standards documents please? This is a bit shorter and the correct name. In user-visible string Parallel SCSI is fine with me, but maybe it should rather be

Quoted 4 lines

This is wrong. Currently the option are bool and you'd build them into the kernel even if the scsi code is modular. Either use scsi_mod-* or make them their own modules using tristate (I still think that's overkill just for the attributes, but jejb seems to like it)

Quoted 2 lines

Out of curiosity: Why do we need _another_ classdev. Can't you reuse the current one?

Please put stubs in the header instead of ifdefs in the code. (And 1:0 for jejb, with the moular approach this would just go away :))

Quoted 1 line

Yikes!

Quoted 1 line

no need to cast here, this is not C++ (fortunately..)

Quoted 1 line

this is just obsfucation, please kill the macro.

Quoted 1 line

same comments as above

Quoted 1 line

	void			*transport_attr_values;

Quoted 1 line

should just go to scsi_device.h

Quoted 3 lines

this is callback, thus the inline doesn't make sense at all. just give spi and fc their own copy - that scales better for possible future additions anyway

Quoted 2 lines

this is internal to the scsi midlayer, thus should go to scsi_priv.h (or be static to scsi_sysfs.c as suggested above)

Quoted 1 line

doesn't seem to exist at all..

Quoted 2 lines

Please set them _directrly_ in the host template.
Currently you're writing to the host template for every found adapter..

Quoted 1 line

should be struct scsi_device *sdev

Quoted 5 lines

Okay, this is ugly as hell, as every driver needs to duplicate this verbosely. Better have a scsi_set_{spi,fc}_transport function that can be called in module_init and then does not only set the calls and attributes (so they don't need to exposed anymore but also a default constructor for the attributes.

Quoted 4 lines

I don't think the _destroy_transport_attrs call makes sense, just kfree the transport_attrs field in the core code uncondititionally. If you plan more complex destructors for some reason put a destructor in the host teplate, similar to the constructore mentioned above.

drivers/scsi/Kconfig +2 -0

+config SCSI_PSCSI_ATTRS
+	bool "Parallel SCSI Transport Attributes"

Quoted 5 lines

Quoted 5 lines

Scan we replacte the PSCSI/pscsi in identifiers with SPI/spi as used in the t10 standards documents please? This is a bit shorter and the correct name. In user-visible string Parallel SCSI is fine with me, but maybe it should rather be

bool "Parallel SCSI (SPI) Transport Attributes"

drivers/scsi/Makefile +2 -0

+obj-$(CONFIG_SCSI_PSCSI_ATTRS)	+= scsi_transport_pscsi.o
+obj-$(CONFIG_SCSI_FC_ATTRS)	+= scsi_transport_fc.o

Quoted 5 lines

Quoted 5 lines

This is wrong. Currently the option are bool and you'd build them into the kernel even if the scsi code is modular. Either use scsi_mod-* or make them their own modules using tristate (I still think that's overkill just for the attributes, but jejb seems to like it)

drivers/scsi/scsi_scan.c +7 -0

 		snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
+			 "%d:%d:%d:%d", sdev->host->host_no,
+			 sdev->channel, sdev->id, sdev->lun);
+
+		class_device_initialize(&sdev->transport_classdev);
+		sdev->transport_classdev.dev = &sdev->sdev_gendev;
+		sdev->transport_classdev.class = sdev->host->hostt->transport_class;
+		snprintf(sdev->transport_classdev.class_id, BUS_ID_SIZE,

Quoted 3 lines

Quoted 3 lines

Out of curiosity: Why do we need _another_ classdev. Can't you reuse the current one?

drivers/scsi/scsi_sysfs.c +14 -0

+	error = class_register(&sdev_class);
+	if (error)
+		goto undo_bus;
+
+#ifdef CONFIG_SCSI_PSCSI_ATTRS
+	error = scsi_pscsi_transport_init();
+	if (error)
+		goto undo_sdev;
+#endif
+#ifdef CONFIG_SCSI_FC_ATTRS
+	error = scsi_fc_transport_init();
+	if (error)
+		goto undo_all;
+#endif

Quoted 2 lines

Please put stubs in the header instead of ifdefs in the code. (And 1:0 for jejb, with the moular approach this would just go away :))

+ out:

drivers/scsi/scsi_sysfs.c +5 -0

 	return error;
+
+	/* Prevent "unused label" warnings when either of the
+	 * transport attributes config options are turned off. */
+	goto undo_all;
+	goto undo_sdev;

Quoted 2 lines

Quoted 2 lines

Yikes!

+extern int scsi_add_transport_attributes(struct scsi_device *sdev);

Quoted 2 lines

Quoted 2 lines

this is internal to the scsi midlayer, thus should go to scsi_priv.h (or be static to scsi_sysfs.c as suggested above)

+extern int scsi_export_transport_attributes(struct scsi_device *sdev);

Quoted 1 line

Quoted 1 line

doesn't seem to exist at all..

drivers/scsi/qla1280.c +2 -0

+	host->hostt->transport_attrs = pscsi_transport_attrs;
+	host->hostt->transport_class = &pscsi_transport_class;

Quoted 3 lines

Quoted 3 lines

Please set them _directrly_ in the host template.
Currently you're writing to the host template for every found adapter..

drivers/scsi/qla1280.c +2 -0

+static int
+qla1280_slave_alloc(Scsi_Device *device)

Quoted 2 lines

Quoted 2 lines

should be struct scsi_device *sdev

drivers/scsi/qla1280.c +9 -0

+{
+	int error = 0;
+
+	error = pscsi_alloc_transport_attrs(device);
+	if (error)
+		return error;
+
+	return error;
+}

Quoted 6 lines

Quoted 6 lines

Okay, this is ugly as hell, as every driver needs to duplicate this verbosely. Better have a scsi_set_{spi,fc}_transport function that can be called in module_init and then does not only set the calls and attributes (so they don't need to exposed anymore but also a default constructor for the attributes.

drivers/scsi/qla1280.c +4 -0

+void qla1280_slave_destroy(Scsi_Device *device)
+{
+	pscsi_destroy_transport_attrs(device);
+}

Quoted 4 lines

Quoted 4 lines

I don't think the _destroy_transport_attrs call makes sense, just kfree the transport_attrs field in the core code uncondititionally. If you plan more complex destructors for some reason put a destructor in the host teplate, similar to the constructore mentioned above.

In fact after all the above suggestions it might be useful to have a struct scsi_transport_attribute instead of stuffing all that into the host template (sorry for guiding you into that direction). The most logical interface to set it would be to have a third argument for scsi_host_alloc, but we can't break that interface now, so we should probably add scsi_set_transport(struct Scsi_host *,
struct scsi_transport_template *), then and the driver will do in it's probe rountine, somewhere between scsi_host_alloc and scsi_add_host a scsi_set_transport(shost, &scsi_spi_template);

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Out of curiosity: Why do we need _another_ classdev. Can't you reuse the current one?

It guess we could re-use the scsi_device classdev. This would mean that all the attribute files would show up in /sys/class/scsi_device/w:x:y:z. We would have to export an attribute called "transport" to tell which transport the device is using. I'm agreeable to this solution. I don't much like having an fc_transport and pscsi_transport directory in /sys/class.

Here is round#3 of the patch. I think I've addressed all of the points that were brought up by Christoph last time.

Once again, three patches: core, qla1280 and qla2xxx. The core patch expects to be applied on top of the patch that I sent recently to linux-scsi:

drivers/scsi/scsi_transport_fc.c line 101

Quoted 6 lines · Martin Hicks

drivers/scsi/scsi_transport_fc.c new file +6 -0

+
+/* the FiberChannel Tranport Attributes: */
+fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr(port_id, "0x%x\n");
+

Purely aesthetics : PortIDs are typically viewed as three hex bytes padded with zeros, i.e. 010203. Perhaps this may be more appropriate:

	fc_transport_rd_attr(port_id, "0x%06x\n");

[snip]

drivers/scsi/qla2xxx/qla_os.c line 1893

Quoted 6 lines

drivers/scsi/qla2xxx/qla_os.c +6 -0

+	attrs = (struct fc_transport_attrs *)sdev->transport_attr_values;
+	list_for_each_entry(fc, &ha->fcports, list) {
+		if (fc->os_target_id == sdev->id) {
+			attrs->port_name = __be64_to_cpu(*(uint64_t *)fc->port_name);
+			attrs->node_name = __be64_to_cpu(*(uint64_t *)fc->node_name);
+			attrs->port_id = fc->d_id.b24;

This port_id assignment will not give you what you expect on big-endian machines. Given the previous port_id example value (010203), on BE machines, the attribute will read as 030201 when displayed. A more endian safe way if you want to continue with the pure-type usages (uint64_t/int) rather than byte arrays would be:

	attrs->port_id = fc->d_id.b.domain << 16 |
		fc->d_id.b.area << 8 |
		fc->d_id.b.al_pa;

drivers/scsi/scsi_transport_fc.c line 100

Quoted 4 lines

drivers/scsi/scsi_transport_fc.c new file +4 -0

+/* the FiberChannel Tranport Attributes: */
+fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
+fc_transport_rd_attr(port_id, "0x%x\n");

Purely aesthetics : PortIDs are typically viewed as three hex bytes padded with zeros, i.e. 010203. Perhaps this may be more appropriate:

        fc_transport_rd_attr(port_id, "0x%06x\n");

Hi,

Here is round#3 of the patch. I think I've addressed all of the points that were brought up by Christoph last time.

diff -Nru a/drivers/scsi/scsi_syms.c b/drivers/scsi/scsi_syms.c

drivers/scsi/scsi_syms.c +2 -0

@@ -21,6 +21,8 @@
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>
+#include <scsi/scsi_transport_spi.h>
+#include <scsi/scsi_transport_fc.h>
 #include <scsi/scsicam.h>
 #include "scsi.h"
 

diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c

drivers/scsi/scsi_sysfs.c +17 -5

@@ -13,6 +13,9 @@
 #include <linux/device.h>
 
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
+#include <scsi/scsi_transport_spi.h>
+#include <scsi/scsi_transport_fc.h>
 #include "scsi.h"
 
 #include "scsi_priv.h"

@@ -162,13 +165,31 @@
 	int error;
 
 	error = bus_register(&scsi_bus_type);
-	if (!error) {
-		error = class_register(&sdev_class);
-		if (error)
-			bus_unregister(&scsi_bus_type);
-	}
+	if (error)
+		return error;
 
+	error = class_register(&sdev_class);
+	if (error)
+		goto undo_bus;
+
+	error = scsi_spi_transport_init();
+	if (error)
+		goto undo_sdev;
+
+	error = scsi_fc_transport_init();
+	if (error)
+		goto undo_all;
+

+extern struct scsi_transport_template blank_transport_template;

drivers/scsi/Kconfig line 200

Quoted 2 lines

drivers/scsi/Kconfig +2 -0

+menu "SCSI Transport Attributes (EXPERIMENTAL)"
+	depends on EXPERIMENTAL && SCSI!=n

Do you really need the experimental flags? Actually this option is rather bogus anyway - with your patches posted the qlogic drivers won't compile anymore without those set. So either make it unconditional or let the drives select it.

drivers/scsi/Makefile line 140

Quoted 13 lines

drivers/scsi/Makefile +8 -1

+ifdef CONFIG_SCSI_SPI_ATTRS
+transport-objs			+= scsi_transport_spi.o
+endif
+ifdef CONFIG_SCSI_FC_ATTRS
+transport-objs			+= scsi_transport_fc.o
+endif
+
 scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o constants.o \
 				   scsicam.o scsi_error.o scsi_lib.o \
 				   scsi_scan.o scsi_syms.o scsi_sysfs.o \
-				   scsi_devinfo.o
+				   scsi_devinfo.o $(transport-objs)

This still looks quite overcomplicated, what about a simple:

scsi_mod-$(CONFIG_SCSI_SPI_ATTRS) += scsi_transport_spi.o scsi_mod-$(CONFIG_SCSI_FC_ATTRS) += scsi_transport_fc.o

drivers/scsi/scsi_sysfs.c line 542

Quoted 3 lines

drivers/scsi/scsi_sysfs.c +3 -0

+/* A blank transport template that is used in drivers that don't
+ * yet implement Transport Attributes */
+struct scsi_transport_template blank_transport_template = { NULL };

The zero-initialization is not needed.

drivers/scsi/scsi_transport_fc.c line 46

Quoted 4 lines

drivers/scsi/scsi_transport_fc.c new file +3 -0

+void scsi_sysfs_cleanup_fc_transport(struct scsi_device *sdev)
+{
+	kfree(sdev->transport_attr_values);

I think you'll have to do the kfree in the device's ->release, but maybe I got all that sysfs-foo wrong :)

include/scsi/scsi_transport.h line 37

Quoted 2 lines

include/scsi/scsi_transport.h new file +2 -0

+	/* Default values for the transport attributes */
+	void *default_attr_values;

Should this really be a void *?

Quoted 2 lines

+extern struct scsi_transport_template blank_transport_template;

Should be in scsi_priv.h as it's not exported, no?

I just submitted a new LLD to linux-scsi and Christoph suggested I use this patch for setting SCSI bus attributes. I looked through the patches and noticed that these are per device attributes. I need to be able to set per SCSI bus attributes. The attributes I would like to be able to set include: initiator ID, maximum bus speed, wide enabled/disabled, and device spinup delay. Would it make sense to create a scsi_bus class or something similar for these types of attributes?

For the IPR driver -

Is the spinup delay for the entire card, not per bus? If so it should be a scsi_host attribute set via shost_attrs. (No one uses shost_attrs, but its use is the same as sdev_attrs usage in 53c700.c)

And are the other values for the actual physical disks? If they are not visible to the host, then there is no bus visible to the host (i.e. scsi core can't see them).

Having the physical disk show up in scsi core as drives is nice for managing them, but if they are ready for writing it could be confusing, and potentially disasterous. We could do hacks to get different behaviour for the disks (so only sg shows up, or so sd shows up read only), like: add a flag in scsi_host; modify the INQUIRY type in the ipr driver; or modify the MODE SENSE page 0x3f (and/or page 0) result in the ipr driver so they show up read only (though on failure of a MODE SENSE they would become writable).

If they were visible to scsi core, the intiator ID (this_id) would be found in the scsi_host, but would need to be added as a sysfs attribute.

We could add a scsi_bus class, but we first need a bus/fabric struct that can include the class, and that in turn requires quite a bit more work to get it in the right place in the driver model tree (we would want a bus/fabric per host:channel pair).

As noted before, we also don't have a target, and the transport attributes (per the patch) show up as per LUN, even though they might be per target, or even per bus/fabric. IMO that is fine for now.

i.e. we have a sysfs tree like this:

`-- pcifoo

    `-- host14
        |-- host_attrs
        |-- 14:0:0:0
        |   `-- sdev_attrs
        `-- 14:0:0:1
            `-- sdev_attrs

But we need something like this:

`-- pcifoo

    `-- host14
        |-- host_attrs
        `-- 14:0
            |-- bus_fabric_attrs
            `-- 14:0:0
                |-- target_attrs
                |-- 14:0:0:0
                |   `-- sdev_attrs
                `-- 14:0:0:1
                    `-- sdev_attrs

And then driver model class or bus code can be used with the above as needed.