* [PATCH 0/5] Assign the unique id used for printing earlier
@ 2024-06-18 15:35 Niklas Cassel
2024-06-18 15:35 ` [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports Niklas Cassel
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 15:35 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: Mika Westerberg, linux-ide
Hello all,
This series moves the assignment of ap->print_id, which is used as a
unique id for each port, earlier, such that we can use the ata_port_*
print functions even before the ata_host has been registered.
Kind regards,
Niklas
Niklas Cassel (5):
ata: libata-core: Remove support for decreasing the number of ports
ata: libata-scsi: Remove superfluous assignment in
ata_sas_port_alloc()
ata: libata: Assign print_id at port allocation time
ata: libata-scsi: Assign local_port_no at host allocation time
ata: ahci: Add debug print for external port
drivers/ata/ahci.c | 4 +++-
drivers/ata/libata-core.c | 33 ++++++++++++---------------------
drivers/ata/libata-sata.c | 2 --
3 files changed, 15 insertions(+), 24 deletions(-)
--
2.45.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports
2024-06-18 15:35 [PATCH 0/5] Assign the unique id used for printing earlier Niklas Cassel
@ 2024-06-18 15:35 ` Niklas Cassel
2024-06-18 16:40 ` Niklas Cassel
2024-06-18 15:35 ` [PATCH 2/5] ata: libata-scsi: Remove superfluous assignment in ata_sas_port_alloc() Niklas Cassel
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 15:35 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: Mika Westerberg, linux-ide
Commit f31871951b38 ("libata: separate out ata_host_alloc() and
ata_host_register()") added ata_host_alloc(), where the API allowed
a LLD to overallocate the number of ports supplied to ata_host_alloc(),
as long as the LLD decreased host->n_ports before calling
ata_host_register().
However, this functionally has never ever been used by a single LLD.
Because of the current API design, the assignment of ap->print_id is
deferred until registration time, which is bad, because that means that
the ata_port_*() print functions cannot be used by a LLD until after
registration time, which means that a LLD is forced to use a print
function that is non-port specific, even for a port specific error.
Remove the support for decreasing the number of ports, such that it will
be possible to assign ap->print_id earlier.
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
drivers/ata/libata-core.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e1bf8a19b3c8..0dc6e18bf492 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5541,24 +5541,19 @@ EXPORT_SYMBOL_GPL(ata_host_put);
/**
* ata_host_alloc - allocate and init basic ATA host resources
* @dev: generic device this host is associated with
- * @max_ports: maximum number of ATA ports associated with this host
+ * @n_ports: the number of ATA ports associated with this host
*
* Allocate and initialize basic ATA host resources. LLD calls
* this function to allocate a host, initializes it fully and
* attaches it using ata_host_register().
*
- * @max_ports ports are allocated and host->n_ports is
- * initialized to @max_ports. The caller is allowed to decrease
- * host->n_ports before calling ata_host_register(). The unused
- * ports will be automatically freed on registration.
- *
* RETURNS:
* Allocate ATA host on success, NULL on failure.
*
* LOCKING:
* Inherited from calling layer (may sleep).
*/
-struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
+struct ata_host *ata_host_alloc(struct device *dev, int n_ports)
{
struct ata_host *host;
size_t sz;
@@ -5566,7 +5561,7 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
void *dr;
/* alloc a container for our list of ATA ports (buses) */
- sz = sizeof(struct ata_host) + (max_ports + 1) * sizeof(void *);
+ sz = sizeof(struct ata_host) + (n_ports + 1) * sizeof(void *);
host = kzalloc(sz, GFP_KERNEL);
if (!host)
return NULL;
@@ -5584,11 +5579,11 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
spin_lock_init(&host->lock);
mutex_init(&host->eh_mutex);
host->dev = dev;
- host->n_ports = max_ports;
+ host->n_ports = n_ports;
kref_init(&host->kref);
/* allocate ports bound to this host */
- for (i = 0; i < max_ports; i++) {
+ for (i = 0; i < n_ports; i++) {
struct ata_port *ap;
ap = ata_port_alloc(host);
@@ -5899,12 +5894,13 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
return -EINVAL;
}
- /* Blow away unused ports. This happens when LLD can't
- * determine the exact number of ports to allocate at
- * allocation time.
+ /*
+ * For a driver using ata_host_register(), the ports are allocated by
+ * ata_host_alloc(), which also allocates the host->ports array.
+ * The number of array elements must match host->n_ports.
*/
for (i = host->n_ports; host->ports[i]; i++)
- kfree(host->ports[i]);
+ WARN_ON(host->ports[i]);
/* give ports names and add SCSI hosts */
for (i = 0; i < host->n_ports; i++) {
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/5] ata: libata-scsi: Remove superfluous assignment in ata_sas_port_alloc()
2024-06-18 15:35 [PATCH 0/5] Assign the unique id used for printing earlier Niklas Cassel
2024-06-18 15:35 ` [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports Niklas Cassel
@ 2024-06-18 15:35 ` Niklas Cassel
2024-06-19 3:47 ` Damien Le Moal
2024-06-18 15:35 ` [PATCH 3/5] ata: libata: Assign print_id at port allocation time Niklas Cassel
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 15:35 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: Mika Westerberg, linux-ide
ata_sas_port_alloc() calls ata_port_alloc() which already assigns ap->lock
so there is no need to ata_sas_port_alloc() to assign it again.
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
drivers/ata/libata-sata.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 9e047bf912b1..c564eac9d430 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1228,7 +1228,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
return NULL;
ap->port_no = 0;
- ap->lock = &host->lock;
ap->pio_mask = port_info->pio_mask;
ap->mwdma_mask = port_info->mwdma_mask;
ap->udma_mask = port_info->udma_mask;
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] ata: libata: Assign print_id at port allocation time
2024-06-18 15:35 [PATCH 0/5] Assign the unique id used for printing earlier Niklas Cassel
2024-06-18 15:35 ` [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports Niklas Cassel
2024-06-18 15:35 ` [PATCH 2/5] ata: libata-scsi: Remove superfluous assignment in ata_sas_port_alloc() Niklas Cassel
@ 2024-06-18 15:35 ` Niklas Cassel
2024-06-19 3:51 ` Damien Le Moal
2024-06-19 4:56 ` Damien Le Moal
2024-06-18 15:35 ` [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host " Niklas Cassel
2024-06-18 15:35 ` [PATCH 5/5] ata: ahci: Add debug print for external port Niklas Cassel
4 siblings, 2 replies; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 15:35 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: Mika Westerberg, linux-ide
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 */
- 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;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host allocation time
2024-06-18 15:35 [PATCH 0/5] Assign the unique id used for printing earlier Niklas Cassel
` (2 preceding siblings ...)
2024-06-18 15:35 ` [PATCH 3/5] ata: libata: Assign print_id at port allocation time Niklas Cassel
@ 2024-06-18 15:35 ` Niklas Cassel
2024-06-19 4:04 ` Damien Le Moal
2024-06-18 15:35 ` [PATCH 5/5] ata: ahci: Add debug print for external port Niklas Cassel
4 siblings, 1 reply; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 15:35 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: Mika Westerberg, linux-ide
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(-)
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]);
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] ata: ahci: Add debug print for external port
2024-06-18 15:35 [PATCH 0/5] Assign the unique id used for printing earlier Niklas Cassel
` (3 preceding siblings ...)
2024-06-18 15:35 ` [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host " Niklas Cassel
@ 2024-06-18 15:35 ` Niklas Cassel
2024-06-19 3:48 ` Damien Le Moal
4 siblings, 1 reply; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 15:35 UTC (permalink / raw)
To: Damien Le Moal, Niklas Cassel; +Cc: Mika Westerberg, linux-ide
Add a debug print that tells us if LPM is not getting enabled because the
port is external.
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
drivers/ata/ahci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 07d66d2c5f0d..d79e46a59df9 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1732,8 +1732,10 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
* Management Interaction in AHCI 1.3.1. Therefore, do not enable
* LPM if the port advertises itself as an external port.
*/
- if (ap->pflags & ATA_PFLAG_EXTERNAL)
+ if (ap->pflags & ATA_PFLAG_EXTERNAL) {
+ ata_port_dbg(ap, "external port, not enabling LPM\n");
return;
+ }
/* user modified policy via module param */
if (mobile_lpm_policy != -1) {
--
2.45.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports
2024-06-18 15:35 ` [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports Niklas Cassel
@ 2024-06-18 16:40 ` Niklas Cassel
0 siblings, 0 replies; 13+ messages in thread
From: Niklas Cassel @ 2024-06-18 16:40 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Mika Westerberg, linux-ide
On Tue, Jun 18, 2024 at 05:35:39PM +0200, Niklas Cassel wrote:
> -struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
> +struct ata_host *ata_host_alloc(struct device *dev, int n_ports)
I forgot to do the same rename of the parameter in the function
declaration in the header file, will fix in v2.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/5] ata: libata-scsi: Remove superfluous assignment in ata_sas_port_alloc()
2024-06-18 15:35 ` [PATCH 2/5] ata: libata-scsi: Remove superfluous assignment in ata_sas_port_alloc() Niklas Cassel
@ 2024-06-19 3:47 ` Damien Le Moal
0 siblings, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2024-06-19 3:47 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Mika Westerberg, linux-ide
On 6/19/24 00:35, Niklas Cassel wrote:
> ata_sas_port_alloc() calls ata_port_alloc() which already assigns ap->lock
> so there is no need to ata_sas_port_alloc() to assign it again.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> ---
> drivers/ata/libata-sata.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 9e047bf912b1..c564eac9d430 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1228,7 +1228,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
> return NULL;
>
> ap->port_no = 0;
> - ap->lock = &host->lock;
> ap->pio_mask = port_info->pio_mask;
> ap->mwdma_mask = port_info->mwdma_mask;
> ap->udma_mask = port_info->udma_mask;
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 5/5] ata: ahci: Add debug print for external port
2024-06-18 15:35 ` [PATCH 5/5] ata: ahci: Add debug print for external port Niklas Cassel
@ 2024-06-19 3:48 ` Damien Le Moal
0 siblings, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2024-06-19 3:48 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Mika Westerberg, linux-ide
On 6/19/24 00:35, Niklas Cassel wrote:
> Add a debug print that tells us if LPM is not getting enabled because the
> port is external.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ata: libata: Assign print_id at port allocation time
2024-06-18 15:35 ` [PATCH 3/5] ata: libata: Assign print_id at port allocation time Niklas Cassel
@ 2024-06-19 3:51 ` Damien Le Moal
2024-06-19 4:56 ` Damien Le Moal
1 sibling, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2024-06-19 3:51 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Mika Westerberg, linux-ide
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;
> }
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host allocation time
2024-06-18 15:35 ` [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host " Niklas Cassel
@ 2024-06-19 4:04 ` Damien Le Moal
2024-06-20 14:52 ` Niklas Cassel
0 siblings, 1 reply; 13+ messages in thread
From: Damien Le Moal @ 2024-06-19 4:04 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Mika Westerberg, linux-ide
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]);
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] ata: libata: Assign print_id at port allocation time
2024-06-18 15:35 ` [PATCH 3/5] ata: libata: Assign print_id at port allocation time Niklas Cassel
2024-06-19 3:51 ` Damien Le Moal
@ 2024-06-19 4:56 ` Damien Le Moal
1 sibling, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2024-06-19 4:56 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Mika Westerberg, linux-ide
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;
> }
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host allocation time
2024-06-19 4:04 ` Damien Le Moal
@ 2024-06-20 14:52 ` Niklas Cassel
0 siblings, 0 replies; 13+ messages in thread
From: Niklas Cassel @ 2024-06-20 14:52 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Mika Westerberg, linux-ide
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-06-20 14:52 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-18 15:35 [PATCH 0/5] Assign the unique id used for printing earlier Niklas Cassel
2024-06-18 15:35 ` [PATCH 1/5] ata: libata-core: Remove support for decreasing the number of ports Niklas Cassel
2024-06-18 16:40 ` Niklas Cassel
2024-06-18 15:35 ` [PATCH 2/5] ata: libata-scsi: Remove superfluous assignment in ata_sas_port_alloc() Niklas Cassel
2024-06-19 3:47 ` Damien Le Moal
2024-06-18 15:35 ` [PATCH 3/5] ata: libata: Assign print_id at port allocation time Niklas Cassel
2024-06-19 3:51 ` Damien Le Moal
2024-06-19 4:56 ` Damien Le Moal
2024-06-18 15:35 ` [PATCH 4/5] ata: libata-scsi: Assign local_port_no at host " Niklas Cassel
2024-06-19 4:04 ` Damien Le Moal
2024-06-20 14:52 ` Niklas Cassel
2024-06-18 15:35 ` [PATCH 5/5] ata: ahci: Add debug print for external port Niklas Cassel
2024-06-19 3:48 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).