Re: [PATCH v2] libata: export host controller number thru /sys
From: Gwendal Grignou <hidden>
Date: 2013-01-22 12:27:46
<resent as text 2> Although your solution works, it breaks the current numbering. The X in <pci-path>/ataX/link<..> has changed meaning, and it is not obvious to an untrained eyes. If we are willing to change, could we reconsider the patches in "Insert ATA transport objects in SCSI syfs topology"? These patches also makes udev rules easier to write, given we have scsi host id available in the path. On Mon, Jan 21, 2013 at 7:49 AM, David Milburn [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On Sun, Jan 20, 2013 at 03:57:16AM +0100, Kay Sievers wrote:quoted
On Thu, Jan 17, 2013 at 7:22 PM, David Milburn [off-list ref] wrote:quoted
The port multiplier extends the ATA port with up to 15 additional ports (links), so with this patch ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1 ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link1/dev1.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link1/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link1.0/dev1.0.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link1.0/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link1.1/dev1.1.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link1.1/ata_link ata<localport>/link<localport>.<unique port mult port>/dev<localport>.<port mult port>.0/ata_device@@ -284,7 +284,7 @@ int ata_tport_add(struct device *parent, dev->parent = get_device(parent); dev->release = ata_tport_release; - dev_set_name(dev, "ata%d", ap->print_id); + dev_set_name(dev, "ata%d", ap->local_print_id);quoted
@@ -410,9 +410,9 @@ int ata_tlink_add(struct ata_link *link) dev->parent = get_device(&ap->tdev); dev->release = ata_tlink_release; if (ata_is_host_link(link)) - dev_set_name(dev, "link%d", ap->print_id); + dev_set_name(dev, "link%d", ap->local_print_id);Hmm, multiple controllers will clash with their names here now, right? The "ata%d" and "link%d..." devices all show up in /sys/class/ in the same directories and need a unique name there, now that we start at every controller with out own counter, right? We need to prefix the names with the global counter?So, would you be OK with this? Every link would be <unique global port>.<unique port multiplier port> combination with no clashes with above ata<local port> # find . -name 'ata*' -print ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1 ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7/dev7.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.0/dev7.0.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.0/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.1/dev7.1.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.1/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.2/dev7.2.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.2/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.3/dev7.3.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.3/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.4/dev7.4.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.4/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.5/dev7.5.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.5/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.6/dev7.6.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.6/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.7/dev7.7.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.7/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.8/dev7.8.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.8/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.9/dev7.9.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.9/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/ata_port ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.10/dev7.10.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.10/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.11/dev7.11.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.11/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.12/dev7.12.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.12/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.13/dev7.13.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.13/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.14/dev7.14.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata1/link7.14/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata2 ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata2/link8/dev8.0/ata_device ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata2/link8/ata_link ./pci0000:00/0000:00:1e.0/0000:05:01.0/ata2/ata_port ./pci0000:00/0000:00:1f.2/ata1 ./pci0000:00/0000:00:1f.2/ata1/link1/dev1.0/ata_device ./pci0000:00/0000:00:1f.2/ata1/link1/ata_link ./pci0000:00/0000:00:1f.2/ata1/ata_port ./pci0000:00/0000:00:1f.2/ata1/ata_port/ata1 ./pci0000:00/0000:00:1f.2/ata2 ./pci0000:00/0000:00:1f.2/ata2/link2/dev2.0/ata_device ./pci0000:00/0000:00:1f.2/ata2/link2/ata_link ./pci0000:00/0000:00:1f.2/ata2/ata_port ./pci0000:00/0000:00:1f.2/ata2/ata_port/ata2 ./pci0000:00/0000:00:1f.2/ata3 ./pci0000:00/0000:00:1f.2/ata3/link3/dev3.0/ata_device ./pci0000:00/0000:00:1f.2/ata3/link3/ata_link ./pci0000:00/0000:00:1f.2/ata3/ata_port ./pci0000:00/0000:00:1f.2/ata3/ata_port/ata3 ./pci0000:00/0000:00:1f.2/ata4 ./pci0000:00/0000:00:1f.2/ata4/link4/dev4.0/ata_device ./pci0000:00/0000:00:1f.2/ata4/link4/ata_link ./pci0000:00/0000:00:1f.2/ata4/ata_port ./pci0000:00/0000:00:1f.2/ata4/ata_port/ata4 ./pci0000:00/0000:00:1f.2/ata5 ./pci0000:00/0000:00:1f.2/ata5/link5/dev5.0/ata_device ./pci0000:00/0000:00:1f.2/ata5/link5/ata_link ./pci0000:00/0000:00:1f.2/ata5/ata_port ./pci0000:00/0000:00:1f.2/ata5/ata_port/ata5 ./pci0000:00/0000:00:1f.2/ata6 ./pci0000:00/0000:00:1f.2/ata6/link6/dev6.0/ata_device ./pci0000:00/0000:00:1f.2/ata6/link6/ata_link ./pci0000:00/0000:00:1f.2/ata6/ata_port ./pci0000:00/0000:00:1f.2/ata6/ata_port/ata6 Thanks, David drivers/ata/libata-core.c | 6 ++++-- drivers/ata/libata-transport.c | 2 +- include/linux/libata.h | 1 + 3 files changed, 6 insertions(+), 3 deletions(-)diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 9e8b99a..b225b87 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c@@ -5604,6 +5604,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host) ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN; ap->lock = &host->lock; ap->print_id = -1; + ap->local_print_id = -1; ap->host = host; ap->dev = host->dev;@@ -6094,9 +6095,10 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht) kfree(host->ports[i]); /* give ports names and add SCSI hosts */ - for (i = 0; i < host->n_ports; i++) + for (i = 0; i < host->n_ports; i++) { host->ports[i]->print_id = atomic_inc_return(&ata_print_id); - + host->ports[i]->local_print_id = i + 1; + } /* Create associated sysfs transport objects */ for (i = 0; i < host->n_ports; i++) {diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c index c04d393..dc5f838 100644 --- a/drivers/ata/libata-transport.c +++ b/drivers/ata/libata-transport.c@@ -284,7 +284,7 @@ int ata_tport_add(struct device *parent, dev->parent = get_device(parent); dev->release = ata_tport_release; - dev_set_name(dev, "ata%d", ap->print_id); + dev_set_name(dev, "ata%d", ap->local_print_id); transport_setup_device(dev); error = device_add(dev); if (error) {diff --git a/include/linux/libata.h b/include/linux/libata.h index 83ba0ab..5208532 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h@@ -742,6 +742,7 @@ struct ata_port { /* Flags that change dynamically, protected by ap->lock */ unsigned int pflags; /* ATA_PFLAG_xxx */ unsigned int print_id; /* user visible unique port ID */ + unsigned int local_print_id; /* host local port ID */ unsigned int port_no; /* 0 based port no. inside the host */ #ifdef CONFIG_ATA_SFF