| Message ID | 20240618153537.2687621-11-cassel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Assign the unique id used for printing earlier | expand |
On 6/19/24 00:35, Niklas Cassel wrote: > Now when we have removed support for decreasing the number of ports, > the most logical thing is to assign local_port_no at the same location(s) > where we assign port_no. > > Note that we cannot move the local_port_no assignment to ata_port_alloc(), > because not all users of ata_port_alloc() assigns local_port_no > (i.e. ata_sas_port_alloc()). > > I have no idea what local_port_no is actually used for, but at least > there should be no functional changes caused by this commit. It used as the sysfs "port_no" attribute to show the port number relative to the adapter. E.g. on my test box which has 2 adapters: # find /sys -name port_no /sys/devices/pci0000:00/0000:00:17.0/ata1/ata_port/ata1/port_no /sys/devices/pci0000:00/0000:00:17.0/ata8/ata_port/ata8/port_no /sys/devices/pci0000:00/0000:00:17.0/ata6/ata_port/ata6/port_no /sys/devices/pci0000:00/0000:00:17.0/ata4/ata_port/ata4/port_no /sys/devices/pci0000:00/0000:00:17.0/ata2/ata_port/ata2/port_no /sys/devices/pci0000:00/0000:00:17.0/ata7/ata_port/ata7/port_no /sys/devices/pci0000:00/0000:00:17.0/ata5/ata_port/ata5/port_no /sys/devices/pci0000:00/0000:00:17.0/ata3/ata_port/ata3/port_no /sys/devices/pci0000:00/0000:00:1c.0/0000:01:00.0/ata9/ata_port/ata9/port_no /sys/devices/pci0000:00/0000:00:1c.0/0000:01:00.0/ata10/ata_port/ata10/port_no For the first adapter: # cat /sys/devices/pci0000:00/0000:00:17.0/ata1/ata_port/ata1/port_no 1 # cat /sys/devices/pci0000:00/0000:00:17.0/ata2/ata_port/ata2/port_no 2 ... And for the second adapter: # cat /sys/devices/pci0000:00/0000:00:1c.0/0000:01:00.0/ata9/ata_port/ata9/port_no 1 #cat /sys/devices/pci0000:00/0000:00:1c.0/0000:01:00.0/ata10/ata_port/ata10/port_no 2 So the confusion here is the naming: ap->print_id is an ID increasing for all adapters, ap->port_no is the index of the port in the host (adapter) array of ports and local_port_no is the same + 1... So I think we can get rid of local_port_no by simply rewriting: ata_port_simple_attr(local_port_no, port_no, "%u\n", unsigned int); in libata-transport.c. That will avoid this useless and confusing code. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/ata/libata-core.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index fb4835c2ba2d..ee1a75bc0466 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5591,6 +5591,7 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) > goto err_out; > > ap->port_no = i; > + ap->local_port_no = i + 1; > host->ports[i] = ap; > } > > @@ -5902,10 +5903,6 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh > for (i = host->n_ports; host->ports[i]; i++) > WARN_ON(host->ports[i]); > > - /* give ports names and add SCSI hosts */ > - for (i = 0; i < host->n_ports; i++) > - host->ports[i]->local_port_no = i + 1; > - > /* Create associated sysfs transport objects */ > for (i = 0; i < host->n_ports; i++) { > rc = ata_tport_add(host->dev,host->ports[i]);
On Wed, Jun 19, 2024 at 01:04:44PM +0900, Damien Le Moal wrote: > > So the confusion here is the naming: ap->print_id is an ID increasing for all > adapters, ap->port_no is the index of the port in the host (adapter) array of > ports and local_port_no is the same + 1... > > So I think we can get rid of local_port_no by simply rewriting: > > ata_port_simple_attr(local_port_no, port_no, "%u\n", unsigned int); > > in libata-transport.c. That will avoid this useless and confusing code. Right now, libsas ports has both port_no and local_port_no set to 0, so if we change common code to always print sysfs port_no attribute as ap->port_no + 1, the sysfs value for libsas ports will change, which would be a user visible change, but perhaps that is fine? I don't understand why sysfs port_no should start at 0 for libsas, but at 1 for other libata ports. Kind regards, Niklas
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index fb4835c2ba2d..ee1a75bc0466 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5591,6 +5591,7 @@ struct ata_host *ata_host_alloc(struct device *dev, int n_ports) goto err_out; ap->port_no = i; + ap->local_port_no = i + 1; host->ports[i] = ap; } @@ -5902,10 +5903,6 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh for (i = host->n_ports; host->ports[i]; i++) WARN_ON(host->ports[i]); - /* give ports names and add SCSI hosts */ - for (i = 0; i < host->n_ports; i++) - host->ports[i]->local_port_no = i + 1; - /* Create associated sysfs transport objects */ for (i = 0; i < host->n_ports; i++) { rc = ata_tport_add(host->dev,host->ports[i]);
Now when we have removed support for decreasing the number of ports, the most logical thing is to assign local_port_no at the same location(s) where we assign port_no. Note that we cannot move the local_port_no assignment to ata_port_alloc(), because not all users of ata_port_alloc() assigns local_port_no (i.e. ata_sas_port_alloc()). I have no idea what local_port_no is actually used for, but at least there should be no functional changes caused by this commit. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/ata/libata-core.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)