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

login
register
mail settings
Submitter David Milburn
Date Jan. 15, 2013, 10:07 p.m.
Message ID <1358287662-24016-1-git-send-email-dmilburn@redhat.com>
Download mbox | patch
Permalink /patch/212315/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

David Milburn - Jan. 15, 2013, 10:07 p.m.
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(-)

--
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
Kay Sievers - Jan. 16, 2013, 1:01 a.m.
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
--
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
David Milburn - Jan. 16, 2013, 9:25 p.m.
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



--
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
Kay Sievers - Jan. 16, 2013, 10:56 p.m.
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
--
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

Patch

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;