| Message ID | 20130121154909.GA5750@dhcp-10-15-1-70.hsv.redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
<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
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
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
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
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
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