Skip to contentSkip to conversation

Should isci create its own device class?

DISCUSSION Dan Williams 2 participants 3 messages Mar 2, 2011

Subsystems (2)
SCSI Intel C600 series SAS controller driver
lore
Share & tools
lore

Dan Williams

author
#0 lore raw

For compatibility with other software raid environments an isci [1] device may contain up to two controller instances per pci device.

Currently the sysfs path for its scsi_host objects is:

Quoted 2 lines

../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/host10/scsi_host/host10 ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/host11/scsi_host/host11

But to reflect reality the pci device is actually the parent of two independent controller instances. With a mockup like:

Quoted 2 lines

../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/controller0/host10/scsi_host/host10 ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/controller1/host11/scsi_host/host11

I can see this being beneficial in a few ways:

1/ fix dev_printk() messages which right now give an ambiguous "isci 0000:03:00.0" prefix
2/ controller boundaries are visible via the sysfs path without need to to look at sas_addresses to determine the controller. 2/ if we ever wanted to support some per controller attributes between the global pci attributes and the scsi_host attributes.

...but would this confuse /dev/disk/by-path/ or any other unintended side effects?

--
Dan

[1]: http://marc.info/?l=linux-scsi&m=129703780424729&w=2

#1 lore raw
Quoted 2 lines · Dan Williams

For compatibility with other software raid environments an isci [1] device may contain up to two controller instances per pci device.

So every other SCSI card I've seen that's done this, the multiple controllers have been different PCI functions, and thus distinguishable at the PCI level ... why is isci different?

Quoted 9 lines

Currently the sysfs path for its scsi_host objects is:

Quoted 2 lines

../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/host10/scsi_host/host10 ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/host11/scsi_host/host11

But to reflect reality the pci device is actually the parent of two independent controller instances. With a mockup like:

Quoted 2 lines

../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/controller0/host10/scsi_host/host10 ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/controller1/host11/scsi_host/host11

Quoted 8 lines

I can see this being beneficial in a few ways:

1/ fix dev_printk() messages which right now give an ambiguous "isci 0000:03:00.0" prefix
2/ controller boundaries are visible via the sysfs path without need to to look at sas_addresses to determine the controller.
2/ if we ever wanted to support some per controller attributes between the global pci attributes and the scsi_host attributes.

So what's wrong with just using the hostX for this instead of inventing a new dummy level in the tree?

Quoted 2 lines

...but would this confuse /dev/disk/by-path/ or any other unintended side effects?

Yes and no ... yes as in this extra piece now has to be in the tree so any tool that actually parses by path could get confused, but no because all the usual links will update correctly.

James

author
#2 lore raw
Quoted 8 lines · James Bottomley
Quoted 2 lines · Dan Williams

For compatibility with other software raid environments an isci [1] device may contain up to two controller instances per pci device.

So every other SCSI card I've seen that's done this, the multiple controllers have been different PCI functions, and thus distinguishable at the PCI level ... why is isci different?

The existence of multiple SAS controllers is hidden behind a single "RAID controller" in environments where software RAID is "request" based and cannot span controllers with a single driver instance. Linux software RAID in contrast is bio based and controller spanning happens naturally.

Quoted 18 lines
Quoted 9 lines

Currently the sysfs path for its scsi_host objects is:

Quoted 2 lines

../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/host10/scsi_host/host10 ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/host11/scsi_host/host11

But to reflect reality the pci device is actually the parent of two independent controller instances. With a mockup like:

Quoted 2 lines

../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/controller0/host10/scsi_host/host10 ../../devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:08.0/0000:03:00.0/controller1/host11/scsi_host/host11

Quoted 8 lines

I can see this being beneficial in a few ways:

1/ fix dev_printk() messages which right now give an ambiguous "isci 0000:03:00.0" prefix
2/ controller boundaries are visible via the sysfs path without need to to look at sas_addresses to determine the controller.
2/ if we ever wanted to support some per controller attributes between the global pci attributes and the scsi_host attributes.

Quoted 3 lines

So what's wrong with just using the hostX for this instead of inventing a new dummy level in the tree?

Ok, I'm primarily concerned with userspace being able to map physical connectors back to sas_phy objects. I think an "isci_id" file at the hostX level would be sufficient, and cleaner in the case of single controller isci instances. It does not address the dev_printk() nit, but that is easy enough to workaround. Is the following acceptable?

drivers/scsi/isci/init.c +26 -4
index 124f4c7..5f46124 100644
@@ -237,12 +237,28 @@ static int isci_register_sas_ha(struct isci_host *isci_host)
 	return 0;
 }
 
-static void isci_unregister_sas_ha(struct isci_host *isci_host)
+static ssize_t isci_show_id(struct device *dev, struct device_attribute *attr, char *buf)
 {
+	struct Scsi_Host *shost = container_of(dev, typeof(*shost), shost_dev);
+	struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(shost);
+	struct isci_host *ihost = container_of(sas_ha, typeof(*ihost), sas_ha);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", ihost->id);
+}
+
+static DEVICE_ATTR(isci_id, S_IRUGO, isci_show_id, NULL);
+
+static void isci_unregister(struct isci_host *isci_host)
+{
+	struct Scsi_Host *shost;
+
 	if (!isci_host)
 		return;
 
-	sas_unregister_ha(&(isci_host->sas_ha));
+	shost = isci_host->shost;
+	device_remove_file(&shost->shost_dev, &dev_attr_isci_id);
+
+	sas_unregister_ha(&isci_host->sas_ha);
 
 	sas_remove_host(isci_host->shost);
 	scsi_remove_host(isci_host->shost);

@@ -413,8 +429,14 @@ static struct isci_host *isci_host_alloc(struct pci_dev *pdev, int id)
 	if (err)
 		goto err_shost_remove;
 
+	err = device_create_file(&shost->shost_dev, &dev_attr_isci_id);
+	if (err)
+		goto err_unregister_ha;
+
 	return isci_host;
 
+ err_unregister_ha:
+	sas_unregister_ha(&(isci_host->sas_ha));
  err_shost_remove:
 	scsi_remove_host(shost);
  err_shost:

@@ -516,7 +538,7 @@ static int __devinit isci_pci_probe(struct pci_dev *pdev, const struct pci_devic
 
  err_host_alloc:
 	for_each_isci_host(isci_host, pdev)
-		isci_unregister_sas_ha(isci_host);
+		isci_unregister(isci_host);
 	return err;
 }
 

@@ -525,7 +547,7 @@ static void __devexit isci_pci_remove(struct pci_dev *pdev)
 	struct isci_host *isci_host;
 
 	for_each_isci_host(isci_host, pdev) {
-		isci_unregister_sas_ha(isci_host);
+		isci_unregister(isci_host);
 		isci_host_deinit(isci_host);
 		scic_controller_disable_interrupts(isci_host->core_controller);
 	}