[PATCH v2] libata: export host controller number thru /sys

[PATCH v2] libata: export host controller number thru /sys

[David Milburn]

As low-level drivers register their host controller(s), keep track
of the number of controllers and export thru /sys in a <host.port>
format so that udev can better match up port numbers with a 
specific controller.

# pwd
/sys/devices/pci0000:00
# find . -name 'ata*' -print

(2nd controller with port multiplier attached)

./0000:00:1e.0/0000:05:01.0/ata2.7
./0000:00:1e.0/0000:05:01.0/ata2.7/link7/dev7.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.0/dev7.0.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.0/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.1/dev7.1.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.1/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.2/dev7.2.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.2/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.3/dev7.3.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.3/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.4/dev7.4.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.4/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.5/dev7.5.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.5/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.6/dev7.6.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.6/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.7/dev7.7.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.7/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.8/dev7.8.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.8/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.9/dev7.9.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.9/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/ata_port
./0000:00:1e.0/0000:05:01.0/ata2.7/ata_port/ata2.7
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.10/dev7.10.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.10/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.11/dev7.11.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.11/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.12/dev7.12.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.12/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.13/dev7.13.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.13/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.14/dev7.14.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.7/link7.14/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.8
./0000:00:1e.0/0000:05:01.0/ata2.8/link8/dev8.0/ata_device
./0000:00:1e.0/0000:05:01.0/ata2.8/link8/ata_link
./0000:00:1e.0/0000:05:01.0/ata2.8/ata_port
./0000:00:1e.0/0000:05:01.0/ata2.8/ata_port/ata2.8

(1st controller)

./0000:00:1f.2/ata1.1
./0000:00:1f.2/ata1.1/link1/dev1.0/ata_device
./0000:00:1f.2/ata1.1/link1/ata_link
./0000:00:1f.2/ata1.1/ata_port
./0000:00:1f.2/ata1.1/ata_port/ata1.1
./0000:00:1f.2/ata1.2
./0000:00:1f.2/ata1.2/link2/dev2.0/ata_device
./0000:00:1f.2/ata1.2/link2/ata_link
./0000:00:1f.2/ata1.2/ata_port
./0000:00:1f.2/ata1.2/ata_port/ata1.2
./0000:00:1f.2/ata1.3
./0000:00:1f.2/ata1.3/link3/dev3.0/ata_device
./0000:00:1f.2/ata1.3/link3/ata_link
./0000:00:1f.2/ata1.3/ata_port
./0000:00:1f.2/ata1.3/ata_port/ata1.3
./0000:00:1f.2/ata1.4
./0000:00:1f.2/ata1.4/link4/dev4.0/ata_device
./0000:00:1f.2/ata1.4/link4/ata_link
./0000:00:1f.2/ata1.4/ata_port
./0000:00:1f.2/ata1.4/ata_port/ata1.4
./0000:00:1f.2/ata1.5
./0000:00:1f.2/ata1.5/link5/dev5.0/ata_device
./0000:00:1f.2/ata1.5/link5/ata_link
./0000:00:1f.2/ata1.5/ata_port
./0000:00:1f.2/ata1.5/ata_port/ata1.5
./0000:00:1f.2/ata1.6
./0000:00:1f.2/ata1.6/link6/dev6.0/ata_device
./0000:00:1f.2/ata1.6/link6/ata_link
./0000:00:1f.2/ata1.6/ata_port
./0000:00:1f.2/ata1.6/ata_port/ata1.6

Signed-off-by: David Milburn <dmilburn@redhat.com>
---
changes from v1->v2:

Fengguang Wu reported a sparse warning for host_print_id, declared as extern in drivers/ata/libata.h

drivers/ata/libata-core.c:102:10: sparse: symbol 'host_print_id' was not declared. Should it be static?

 drivers/ata/libata-core.c      |    4 ++++
 drivers/ata/libata-transport.c |    2 +-
 drivers/ata/libata.h           |    1 +
 include/linux/libata.h         |    1 +
 4 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 46cd3f4..275941b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -99,6 +99,7 @@ static void ata_dev_xfermask(struct ata_device *dev);
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
 
 atomic_t ata_print_id = ATOMIC_INIT(0);
+atomic_t host_print_id = ATOMIC_INIT(0);
 
 struct ata_force_param {
 	const char	*name;
@@ -6097,6 +6098,9 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	for (i = host->n_ports; host->ports[i]; i++)
 		kfree(host->ports[i]);
 
+	/* track host controller */
+	host->host_id = atomic_inc_return(&host_print_id);
+
 	/* give ports names and add SCSI hosts */
 	for (i = 0; i < host->n_ports; i++)
 		host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index c04d393..61dca7a 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.%d", ap->host->host_id, ap->print_id);
 	transport_setup_device(dev);
 	error = device_add(dev);
 	if (error) {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 7148a58..97776fd 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -54,6 +54,7 @@ enum {
 };
 
 extern atomic_t ata_print_id;
+extern atomic_t host_print_id;
 extern int atapi_passthru16;
 extern int libata_fua;
 extern int libata_noacpi;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 649e5f8..7ae207e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -546,6 +546,7 @@ struct ata_host {
 	void			*private_data;
 	struct ata_port_operations *ops;
 	unsigned long		flags;
+	unsigned int            host_id; /* user visible host ID */
 
 	struct mutex		eh_mutex;
 	struct task_struct	*eh_owner;

Re: [PATCH v2] libata: export host controller number thru /sys

[Kay Sievers]

On Tue, Jan 15, 2013 at 11:07 PM, David Milburn <dmilburn@redhat.com> wrote:
> As low-level drivers register their host controller(s), keep track
> of the number of controllers and export thru /sys in a <host.port>
> format so that udev can better match up port numbers with a
> specific controller.

Hmm, I tried this and I don't think it will solve out problem. But I
might have missed something ...

This is host 2 with port "7", but it is really port 1:

> ./0000:00:1e.0/0000:05:01.0/ata2.7
> ./0000:00:1e.0/0000:05:01.0/ata2.7/link7/dev7.0/ata_device

This is host 1 with port "6":

> ./0000:00:1f.2/ata1.6/ata_port
> ./0000:00:1f.2/ata1.6/ata_port/ata1.6

The host number is meaningless regarding stable identifiers, as it
depends on probing/driver binding order.

Now, the port number is still *global* across all all controllers, and
therefore also not useful. We need *host-local* numbers, which share
nothing with other hosts, so the drivers and bus enumeration order can
change at any time without affecting any of the device numbers used
inside the host.

I have not much clue about port-multipliers, never seen them, but if
we want to support arbitrary stacking of them we will need to compose
a "chain of numbers" in the "stable identifier" out of the individual
port numbers they are connected to, very much like USB hub chaining
works. This means we will need every instance in the chain to start
with their own numbers again.

Any global counters used for device naming/enumeration are not useful
to identify devices from userspace.

Thanks,
Kay

Re: [PATCH v2] libata: export host controller number thru /sys

[David Milburn]

Kay Sievers wrote:
> On Tue, Jan 15, 2013 at 11:07 PM, David Milburn <dmilburn@redhat.com> wrote:
>> As low-level drivers register their host controller(s), keep track
>> of the number of controllers and export thru /sys in a <host.port>
>> format so that udev can better match up port numbers with a
>> specific controller.
> 
> Hmm, I tried this and I don't think it will solve out problem. But I
> might have missed something ...
> 
> This is host 2 with port "7", but it is really port 1:
> 
>> ./0000:00:1e.0/0000:05:01.0/ata2.7
>> ./0000:00:1e.0/0000:05:01.0/ata2.7/link7/dev7.0/ata_device
> 
> This is host 1 with port "6":
> 
>> ./0000:00:1f.2/ata1.6/ata_port
>> ./0000:00:1f.2/ata1.6/ata_port/ata1.6
> 
> The host number is meaningless regarding stable identifiers, as it
> depends on probing/driver binding order.
> 
> Now, the port number is still *global* across all all controllers, and
> therefore also not useful. We need *host-local* numbers, which share
> nothing with other hosts, so the drivers and bus enumeration order can
> change at any time without affecting any of the device numbers used
> inside the host.
> 
> I have not much clue about port-multipliers, never seen them, but if
> we want to support arbitrary stacking of them we will need to compose
> a "chain of numbers" in the "stable identifier" out of the individual
> port numbers they are connected to, very much like USB hub chaining
> works. This means we will need every instance in the chain to start
> with their own numbers again.
> 
> Any global counters used for device naming/enumeration are not useful
> to identify devices from userspace.

Hi Kay,

So, if we eliminated the global ata_print_id counter, then we would
need to check all the places ap->print_id is used and consider the host
controller and a local_print_id. So, if the above changed to 2.1, we
would expect ata2.1 to show up in dmesg during error recovery
instead of ata7, right?

There are other places (non-printk stuff) in libata that check the global
counter that would need to change, and libsas increments the global
counter when a SATA drive is present on a SAS controller.

Thanks,
David

> 
> Thanks,
> Kay

Re: [PATCH v2] libata: export host controller number thru /sys

[Kay Sievers]

On Wed, Jan 16, 2013 at 10:25 PM, David Milburn <dmilburn@redhat.com> wrote:

> So, if we eliminated the global ata_print_id counter, then we would
> need to check all the places ap->print_id is used and consider the host
> controller and a local_print_id. So, if the above changed to 2.1, we
> would expect ata2.1 to show up in dmesg during error recovery
> instead of ata7, right?
>
> There are other places (non-printk stuff) in libata that check the global
> counter that would need to change, and libsas increments the global
> counter when a SATA drive is present on a SAS controller.

Well, we just need a way to get some "stable" numbers out of /sys when
we start at the block device and walk up the chain of parent devices
and see the ata directory. It does not necessarily need to be the name
of the top ata%d directory, it could be a sysfs attribute somewhere
too, if that's easier.

If we we are going to support port by-path/ strings for block devices
connected through a port multiplier chain, I guess we will need an
arbitrary length string representing the concatenated chain of local
port numbers, containing stable port identifier numbers of every
device involved. None of the numbers it contains can therefore depend
on enumeration or loading order.

The "stable chain of port numbers" do not necessarily need to share
anything with the current global numbers to make the device name
unique. It would be easier to understand, but it's not a requirement.

Kay

Kay

Re: [PATCH v2] libata: export host controller number thru /sys

[David Milburn]

On Wed, Jan 16, 2013 at 11:56:13PM +0100, Kay Sievers wrote:
> On Wed, Jan 16, 2013 at 10:25 PM, David Milburn <dmilburn@redhat.com> wrote:
> 
> > So, if we eliminated the global ata_print_id counter, then we would
> > need to check all the places ap->print_id is used and consider the host
> > controller and a local_print_id. So, if the above changed to 2.1, we
> > would expect ata2.1 to show up in dmesg during error recovery
> > instead of ata7, right?
> >
> > There are other places (non-printk stuff) in libata that check the global
> > counter that would need to change, and libsas increments the global
> > counter when a SATA drive is present on a SAS controller.
> 
> Well, we just need a way to get some "stable" numbers out of /sys when
> we start at the block device and walk up the chain of parent devices
> and see the ata directory. It does not necessarily need to be the name
> of the top ata%d directory, it could be a sysfs attribute somewhere
> too, if that's easier.
> 
> If we we are going to support port by-path/ strings for block devices
> connected through a port multiplier chain, I guess we will need an
> arbitrary length string representing the concatenated chain of local
> port numbers, containing stable port identifier numbers of every
> device involved. None of the numbers it contains can therefore depend
> on enumeration or loading order.
> 
> The "stable chain of port numbers" do not necessarily need to share
> anything with the current global numbers to make the device name
> unique. It would be easier to understand, but it's not a requirement.

Ahh, OK.

Please test this patch.

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

Previously, local port would have been global port.

Thanks,
David

 drivers/ata/libata-core.c      |    6 ++++--
 drivers/ata/libata-transport.c |   10 +++++-----
 include/linux/libata.h         |    1 +
 3 files changed, 10 insertions(+), 7 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..7f6feaf 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) {
@@ -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);
         else
-		dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp);
+		dev_set_name(dev, "link%d.%d", ap->local_print_id, link->pmp);
 
 	transport_setup_device(dev);
 
@@ -638,9 +638,9 @@ static int ata_tdev_add(struct ata_device *ata_dev)
 	dev->parent = get_device(&link->tdev);
 	dev->release = ata_tdev_release;
 	if (ata_is_host_link(link))
-		dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
+		dev_set_name(dev, "dev%d.%d", ap->local_print_id,ata_dev->devno);
         else
-		dev_set_name(dev, "dev%d.%d.0", ap->print_id, link->pmp);
+		dev_set_name(dev, "dev%d.%d.0", ap->local_print_id, link->pmp);
 
 	transport_setup_device(dev);
 	error = device_add(dev);
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

> 
> Kay
> 
> Kay
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] libata: export host controller number thru /sys

[Kay Sievers]

On Thu, Jan 17, 2013 at 7:22 PM, David Milburn <dmilburn@redhat.com> wrote:

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

> @@ -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?

Thanks,
Kay

Re: [PATCH v2] libata: export host controller number thru /sys

[David Milburn]

On Sun, Jan 20, 2013 at 03:57:16AM +0100, Kay Sievers wrote:
> On Thu, Jan 17, 2013 at 7:22 PM, David Milburn <dmilburn@redhat.com> wrote:
> 
> > 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);
> 
> > @@ -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

Re: [PATCH v2] libata: export host controller number thru /sys

[Gwendal Grignou]

<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 <dmilburn@redhat.com> wrote:
> On Sun, Jan 20, 2013 at 03:57:16AM +0100, Kay Sievers wrote:
>> On Thu, Jan 17, 2013 at 7:22 PM, David Milburn <dmilburn@redhat.com> wrote:
>>
>> > 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);
>>
>> > @@ -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
>

Re: [PATCH v2] libata: export host controller number thru /sys

[David Milburn]

Gwendal Grignou wrote:
> <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.

Hi Gwendal,

I applied

http://marc.info/?l=linux-ide&m=134911579601940&w=2
http://marc.info/?l=linux-ide&m=134911580102060&w=2
http://marc.info/?l=linux-ide&m=134931181222435&w=2

# find . -name 'sd*' -print
./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd
./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd/sdd1
./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd/sdd2
./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde
./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde/sde1
./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde/sde2
./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda
./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda/sda1
./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda/sda2
./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb
./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb/sdb1
./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb/sdb2
./0000:00:1f.2/host4/port5/link5/dev5.0/target4:0:0/4:0:0:0/block/sdc
./0000:00:1f.2/host4/port5/link5/dev5.0/target4:0:0/4:0:0:0/block/sdc/sdc1

I think Kay still wants us to avoid using the global ap->print_id, he 
expects local
port numbers per controller, for instance, port7 is really port1.

Thanks,
David




> 
> On Mon, Jan 21, 2013 at 7:49 AM, David Milburn <dmilburn@redhat.com> wrote:
>> On Sun, Jan 20, 2013 at 03:57:16AM +0100, Kay Sievers wrote:
>>> On Thu, Jan 17, 2013 at 7:22 PM, David Milburn <dmilburn@redhat.com> wrote:
>>>
>>>> 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);
>>>
>>>> @@ -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
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] libata: export host controller number thru /sys

[Kay Sievers]

On Tue, Jan 22, 2013 at 10:44 PM, David Milburn <dmilburn@redhat.com> wrote:
> Gwendal Grignou wrote:
>>
>> <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.
>
> Hi Gwendal,
>
> I applied
>
> http://marc.info/?l=linux-ide&m=134911579601940&w=2
> http://marc.info/?l=linux-ide&m=134911580102060&w=2
> http://marc.info/?l=linux-ide&m=134931181222435&w=2
>
> # find . -name 'sd*' -print
> ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd
> ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd/sdd1
> ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd/sdd2
> ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde
> ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde/sde1
> ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde/sde2
> ./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda
> ./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda/sda1
> ./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda/sda2
> ./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb
> ./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb/sdb1
> ./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb/sdb2
> ./0000:00:1f.2/host4/port5/link5/dev5.0/target4:0:0/4:0:0:0/block/sdc
> ./0000:00:1f.2/host4/port5/link5/dev5.0/target4:0:0/4:0:0:0/block/sdc/sdc1
>
> I think Kay still wants us to avoid using the global ap->print_id, he
> expects local
> port numbers per controller, for instance, port7 is really port1.

Right, I need numbers which are entirely independent of driver probing
order. The should all start locally and span multiple independent
controllers/drivers/devices.

I kind of like it when the topology appears directly in the device hierarchy.

We could append the local number just as as .<local port> to the
existing number, or we dould add a sysfs attribute(file) to the
device, containing just the local port number.

Both should work to retrieve the information and compose a string out
of it, to create a by-path/ symlink, which will not change when the
multiple drivers are loaded in different orders, or if devices are
unplugged and re-plugged during runtime of the machine.

Thanks,
Kay

Re: [PATCH v2] libata: export host controller number thru /sys

[Gwendal Grignou]

I see. But this behavior """numbers which are entirely independent of
driver probing order.""" is not consistent with the rest of scsi
subsystem: for instance scsi_host are named using a global number as
well; sas object naming is completely dependent on the probing order.
The idea behind using the global number is to be able to link ata
objects to their pci object parent as they are named in dmesg, so that
we see error messages in dmesg we could locate the whole topology.

The name of the object needs to contain <global port>.
If we want to add <local port> , to be consistent with sas sysfs, I
suggest <object_name><global port><separator><local port> syntax.
<separator> is ':' for scsi transport, but I used '.' already, so we
can continue with it.

Gwendal.


On Tue, Jan 22, 2013 at 8:29 PM, Kay Sievers <kay@vrfy.org> wrote:
>
> On Tue, Jan 22, 2013 at 10:44 PM, David Milburn <dmilburn@redhat.com> wrote:
> > Gwendal Grignou wrote:
> >>
> >> <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.
> >
> > Hi Gwendal,
> >
> > I applied
> >
> > http://marc.info/?l=linux-ide&m=134911579601940&w=2
> > http://marc.info/?l=linux-ide&m=134911580102060&w=2
> > http://marc.info/?l=linux-ide&m=134931181222435&w=2
> >
> > # find . -name 'sd*' -print
> > ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd
> > ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd/sdd1
> > ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.0/dev7.0.0/target6:0:0/6:0:0:0/block/sdd/sdd2
> > ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde
> > ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde/sde1
> > ./0000:00:1e.0/0000:05:01.0/host6/port7/link7.3/dev7.3.0/target6:3:0/6:3:0:0/block/sde/sde2
> > ./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda
> > ./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda/sda1
> > ./0000:00:1f.2/host1/port2/link2/dev2.0/target1:0:0/1:0:0:0/block/sda/sda2
> >
> > ./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb/sdb1
> > ./0000:00:1f.2/host3/port4/link4/dev4.0/target3:0:0/3:0:0:0/block/sdb/sdb2
> > ./0000:00:1f.2/host4/port5/link5/dev5.0/target4:0:0/4:0:0:0/block/sdc
> > ./0000:00:1f.2/host4/port5/link5/dev5.0/target4:0:0/4:0:0:0/block/sdc/sdc1
> >
> > I think Kay still wants us to avoid using the global ap->print_id, he
> > expects local
> > port numbers per controller, for instance, port7 is really port1.
>
> Right, I need numbers which are entirely independent of driver probing
> order. The should all start locally and span multiple independent
> controllers/drivers/devices.
>
> I kind of like it when the topology appears directly in the device hierarchy.
>
> We could append the local number just as as .<local port> to the
> existing number, or we dould add a sysfs attribute(file) to the
> device, containing just the local port number.
>
> Both should work to retrieve the information and compose a string out
> of it, to create a by-path/ symlink, which will not change when the
> multiple drivers are loaded in different orders, or if devices are
> unplugged and re-plugged during runtime of the machine.
>
> Thanks,
> Kay

Re: [PATCH v2] libata: export host controller number thru /sys

[Kay Sievers]

On Thu, Jan 24, 2013 at 8:55 AM, Gwendal Grignou <gwendal@google.com> wrote:
> I see. But this behavior """numbers which are entirely independent of
> driver probing order.""" is not consistent with the rest of scsi
> subsystem

Sure it is, in most interesting cases.

> for instance scsi_host are named using a global number as
> well; sas object naming is completely dependent on the probing order.

Right, but the host number is not interesting at all, it obviously
always depends on the order devices are probed. What's interesting is
lun, target and the like, and they are still just nice and local
numbers, not meaningless global counters.

> The idea behind using the global number is to be able to link ata
> objects to their pci object parent as they are named in dmesg, so that
> we see error messages in dmesg we could locate the whole topology.

Which is absolutely no reason to use global counters for things that
have a well-defined meaning like a device's port number. Nothing here
is limited to a single number,we have strings not numbers. :)
ATA can surely do what USB, PCI, everybody else, even SCSI itself
does, and append the real and meaningful numbers to the parent name,
and all will be meaningful and will be unique.

> The name of the object needs to contain <global port>.
> If we want to add <local port> , to be consistent with sas sysfs, I
> suggest <object_name><global port><separator><local port> syntax.
> <separator> is ':' for scsi transport, but I used '.' already, so we
> can continue with it.

Sure, we need to add something that makes the name unique in the
class, but we should also export the real names and not only the
artificial and meaningless counters. There is a good reason SCSI
repeats the "real" and meaningful numbers in the LUN 0:0:0:0 device
name, and ATA should just do something like that too. :)

Kay