Should isci create its own device class?

Should isci create its own device class?

[Dan Williams]

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:
> ../../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:
> ../../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

Re: Should isci create its own device class?

[James Bottomley]

On Tue, 2011-03-01 at 19:22 -0800, Dan Williams wrote:
> 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?

> Currently the sysfs path for its scsi_host objects is:
> > ../../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:
> > ../../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.

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

> ...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

Re: Should isci create its own device class?

[Dan Williams]

On Wed, 2011-03-02 at 04:49 -0800, James Bottomley wrote:
> On Tue, 2011-03-01 at 19:22 -0800, Dan Williams wrote:
> > 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.

> > Currently the sysfs path for its scsi_host objects is:
> > > ../../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:
> > > ../../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.
> 
> 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?

diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index 124f4c7..5f46124 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -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);
 	}