| Message ID | 20240618153537.2687621-10-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: > While the assignment of ap->print_id could have been moved to > ata_host_alloc(), let's simply move it to ata_port_alloc(). > > If you allocate a port, you want to give it a unique name that can be used > for printing. > > By moving the ap->print_id assignment to ata_port_alloc(), means that we > can also remove the ap->print_id assignment from ata_sas_port_alloc(). > > This will allow a LLD to use the ata_port_*() print functions before > ata_host_register() has been called. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/ata/libata-core.c | 6 ++---- > drivers/ata/libata-sata.c | 1 - > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 0dc6e18bf492..fb4835c2ba2d 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5463,7 +5463,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->print_id = atomic_inc_return(&ata_print_id); > ap->local_port_no = -1; > ap->host = host; > ap->dev = host->dev; > @@ -5903,10 +5903,8 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh > WARN_ON(host->ports[i]); > > /* give ports names and add SCSI hosts */ Nit: I am not sure this comment is still appropriate. Maybe remove it ? Otherwise, looks OK to me. Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > - for (i = 0; i < host->n_ports; i++) { > - host->ports[i]->print_id = atomic_inc_return(&ata_print_id); > + 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++) { > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index c564eac9d430..def0026188f7 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -1234,7 +1234,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host, > ap->flags |= port_info->flags; > ap->ops = port_info->port_ops; > ap->cbl = ATA_CBL_SATA; > - ap->print_id = atomic_inc_return(&ata_print_id); > > return ap; > }
On 6/19/24 00:35, Niklas Cassel wrote: > While the assignment of ap->print_id could have been moved to > ata_host_alloc(), let's simply move it to ata_port_alloc(). > > If you allocate a port, you want to give it a unique name that can be used > for printing. > > By moving the ap->print_id assignment to ata_port_alloc(), means that we > can also remove the ap->print_id assignment from ata_sas_port_alloc(). > > This will allow a LLD to use the ata_port_*() print functions before > ata_host_register() has been called. By the way, shouldn't we replace the ata_print_id atomic with an IDA ? static DEFINE_IDA(ata_ida); And then use: ap->print_id = ida_alloc(&ata_ida, GFP_KERNEL); ida_free(&ata_ida, ap->print_id); That would avoid the print IDs to constantly increase when doing e.g. rmmod ahci+modprobe ahci. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/ata/libata-core.c | 6 ++---- > drivers/ata/libata-sata.c | 1 - > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 0dc6e18bf492..fb4835c2ba2d 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5463,7 +5463,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->print_id = atomic_inc_return(&ata_print_id); > ap->local_port_no = -1; > ap->host = host; > ap->dev = host->dev; > @@ -5903,10 +5903,8 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh > WARN_ON(host->ports[i]); > > /* 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); > + 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++) { > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c > index c564eac9d430..def0026188f7 100644 > --- a/drivers/ata/libata-sata.c > +++ b/drivers/ata/libata-sata.c > @@ -1234,7 +1234,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host, > ap->flags |= port_info->flags; > ap->ops = port_info->port_ops; > ap->cbl = ATA_CBL_SATA; > - ap->print_id = atomic_inc_return(&ata_print_id); > > return ap; > }
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 0dc6e18bf492..fb4835c2ba2d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5463,7 +5463,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->print_id = atomic_inc_return(&ata_print_id); ap->local_port_no = -1; ap->host = host; ap->dev = host->dev; @@ -5903,10 +5903,8 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh WARN_ON(host->ports[i]); /* 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); + 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++) { diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index c564eac9d430..def0026188f7 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1234,7 +1234,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host, ap->flags |= port_info->flags; ap->ops = port_info->port_ops; ap->cbl = ATA_CBL_SATA; - ap->print_id = atomic_inc_return(&ata_print_id); return ap; }
While the assignment of ap->print_id could have been moved to ata_host_alloc(), let's simply move it to ata_port_alloc(). If you allocate a port, you want to give it a unique name that can be used for printing. By moving the ap->print_id assignment to ata_port_alloc(), means that we can also remove the ap->print_id assignment from ata_sas_port_alloc(). This will allow a LLD to use the ata_port_*() print functions before ata_host_register() has been called. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/ata/libata-core.c | 6 ++---- drivers/ata/libata-sata.c | 1 - 2 files changed, 2 insertions(+), 5 deletions(-)