From: David Milburn <dmilburn@redhat.com>
To: Gwendal Grignou <gwendal@google.com>
Cc: Kay Sievers <kay@vrfy.org>,
IDE/ATA development list <linux-ide@vger.kernel.org>,
Jeff Garzik <jgarzik@pobox.com>,
coughlan@redhat.com, fengguang.wu@intel.com
Subject: Re: [PATCH v2] libata: export host controller number thru /sys
Date: Tue, 22 Jan 2013 15:44:56 -0600 [thread overview]
Message-ID: <50FF0858.50504@redhat.com> (raw)
In-Reply-To: <CAMHSBOXWkZTyH18goC6iS-S7VAmCuHJ9Wwhzy6kB1m5RKwFEAA@mail.gmail.com>
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
next prev parent reply other threads:[~2013-01-22 21:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-15 22:07 [PATCH v2] libata: export host controller number thru /sys David Milburn
2013-01-16 1:01 ` Kay Sievers
2013-01-16 21:25 ` David Milburn
2013-01-16 22:56 ` Kay Sievers
2013-01-17 18:22 ` David Milburn
2013-01-20 2:57 ` Kay Sievers
2013-01-21 15:49 ` David Milburn
2013-01-22 12:27 ` Gwendal Grignou
2013-01-22 21:44 ` David Milburn [this message]
2013-01-23 4:29 ` Kay Sievers
2013-01-24 7:55 ` Gwendal Grignou
2013-01-24 9:57 ` Kay Sievers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50FF0858.50504@redhat.com \
--to=dmilburn@redhat.com \
--cc=coughlan@redhat.com \
--cc=fengguang.wu@intel.com \
--cc=gwendal@google.com \
--cc=jgarzik@pobox.com \
--cc=kay@vrfy.org \
--cc=linux-ide@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox