Re: Should isci create its own device class?

[Dan Williams <dan.j.williams@intel.com>]

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);
 	}