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

login
register
mail settings
Submitter David Milburn
Date Jan. 21, 2013, 3:49 p.m.
Message ID <20130121154909.GA5750@dhcp-10-15-1-70.hsv.redhat.com>
Download mbox | patch
Permalink /patch/214189/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

David Milburn - Jan. 21, 2013, 3:49 p.m.
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(-)


--
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
Gwendal Grignou - Jan. 22, 2013, 12:27 p.m.
<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
>
--
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. 22, 2013, 9:44 p.m.
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

--
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. 23, 2013, 4:29 a.m.
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
--
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
Gwendal Grignou - Jan. 24, 2013, 7:55 a.m.
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
--
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. 24, 2013, 9:57 a.m.
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
--
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 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