linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).