[PATCH v3 0/9] ata,libsas: Assign the unique id used for printing earlier

[PATCH v3 0/9] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel]

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.

While the patch series was orginally meant to simply assign a unique
id used for printing earlier (ap->print_id), it has since grown to
also include cleanups related to ata_port_alloc() (since ap->print_id
is now assigned in ata_port_alloc()).


Kind regards,
Niklas


Changes since v2:
-Sent out patches that were strict bug fixes as a separate series.
-Rebased patch series.
-Picked up tags.
-Fixed minor typos.
-Fixed problem reported by kernel test robot, by using a local variable to
 check for errors.
-Removed existing super ugly code that iterates past host->n_ports (which
 relied on the ata_host_alloc() allocating n_ports+1 rather than n_ports),
 as suggested by Hannes.
-Now when we don't have the ugly code that iterates past host->n_ports,
 change ata_host_alloc(() to only allocate n_ports, as the ugly workaround
 of allocating an n_ports+1 ports is no longer needed (as it was only
 needed for the feature that this patch series removes).


Link to v2:
https://lore.kernel.org/linux-ide/20240626180031.4050226-15-cassel@kernel.org/

Link to v1:
https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/


Niklas Cassel (9):
  ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()
  ata: libata: Remove unused function declaration for ata_scsi_detect()
  ata: libata-core: Remove support for decreasing the number of ports
  ata: libata-sata: Remove superfluous assignment in
    ata_sas_port_alloc()
  ata: libata-core: Remove local_port_no struct member
  ata: libata: Assign print_id at port allocation time
  ata: libata-core: Reuse available ata_port print_ids
  ata,scsi: Remove useless ata_sas_port_alloc() wrapper
  ata: ahci: Add debug print for external port

 drivers/ata/ahci.c                 |  4 ++-
 drivers/ata/libata-core.c          | 41 +++++++++----------------
 drivers/ata/libata-sata.c          | 49 ------------------------------
 drivers/ata/libata-transport.c     |  5 ++-
 drivers/ata/libata-transport.h     |  3 --
 drivers/ata/libata.h               |  2 --
 drivers/scsi/libsas/sas_ata.c      | 12 ++++++--
 drivers/scsi/libsas/sas_discover.c |  2 +-
 include/linux/libata.h             | 11 +++----
 9 files changed, 36 insertions(+), 93 deletions(-)

-- 
2.45.2

[PATCH v3 1/9] ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()

[Niklas Cassel]

Remove useless wrappers ata_sas_tport_add() and ata_sas_tport_delete().

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-sata.c          | 12 ------------
 drivers/ata/libata-transport.c     |  2 ++
 drivers/ata/libata-transport.h     |  3 ---
 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_discover.c |  2 +-
 include/linux/libata.h             |  4 ++--
 6 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 9e047bf912b1..e7991595bfe5 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1241,18 +1241,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
 
-int ata_sas_tport_add(struct device *parent, struct ata_port *ap)
-{
-	return ata_tport_add(parent, ap);
-}
-EXPORT_SYMBOL_GPL(ata_sas_tport_add);
-
-void ata_sas_tport_delete(struct ata_port *ap)
-{
-	ata_tport_delete(ap);
-}
-EXPORT_SYMBOL_GPL(ata_sas_tport_delete);
-
 /**
  *	ata_sas_device_configure - Default device_configure routine for libata
  *				   devices
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 3e49a877500e..d24f201c0ab2 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -265,6 +265,7 @@ void ata_tport_delete(struct ata_port *ap)
 	transport_destroy_device(dev);
 	put_device(dev);
 }
+EXPORT_SYMBOL_GPL(ata_tport_delete);
 
 static const struct device_type ata_port_sas_type = {
 	.name = ATA_PORT_TYPE_NAME,
@@ -329,6 +330,7 @@ int ata_tport_add(struct device *parent,
 	put_device(dev);
 	return error;
 }
+EXPORT_SYMBOL_GPL(ata_tport_add);
 
 /**
  *     ata_port_classify - determine device type based on ATA-spec signature
diff --git a/drivers/ata/libata-transport.h b/drivers/ata/libata-transport.h
index 08a57fb9dc61..50cd2cbe8eea 100644
--- a/drivers/ata/libata-transport.h
+++ b/drivers/ata/libata-transport.h
@@ -8,9 +8,6 @@ extern struct scsi_transport_template *ata_scsi_transport_template;
 int ata_tlink_add(struct ata_link *link);
 void ata_tlink_delete(struct ata_link *link);
 
-int ata_tport_add(struct device *parent, struct ata_port *ap);
-void ata_tport_delete(struct ata_port *ap);
-
 struct scsi_transport_template *ata_attach_transport(void);
 void ata_release_transport(struct scsi_transport_template *t);
 
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index cbbe43d8ef87..ab4ddeea4909 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -608,7 +608,7 @@ int sas_ata_init(struct domain_device *found_dev)
 	ap->cbl = ATA_CBL_SATA;
 	ap->scsi_host = shost;
 
-	rc = ata_sas_tport_add(ata_host->dev, ap);
+	rc = ata_tport_add(ata_host->dev, ap);
 	if (rc)
 		goto free_port;
 
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 48d975c6dbf2..951bdc554a10 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -300,7 +300,7 @@ void sas_free_device(struct kref *kref)
 		kfree(dev->ex_dev.ex_phy);
 
 	if (dev_is_sata(dev) && dev->sata_dev.ap) {
-		ata_sas_tport_delete(dev->sata_dev.ap);
+		ata_tport_delete(dev->sata_dev.ap);
 		ata_port_free(dev->sata_dev.ap);
 		ata_host_put(dev->sata_dev.ata_host);
 		dev->sata_dev.ata_host = NULL;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7d3bd7c9664a..586f0116d1d7 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1250,8 +1250,8 @@ extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
 extern void ata_port_probe(struct ata_port *ap);
 extern void ata_port_free(struct ata_port *ap);
-extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
-extern void ata_sas_tport_delete(struct ata_port *ap);
+extern int ata_tport_add(struct device *parent, struct ata_port *ap);
+extern void ata_tport_delete(struct ata_port *ap);
 int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
 		struct ata_port *ap);
 extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap);
-- 
2.45.2

Re: [PATCH v3 1/9] ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()

[John Garry]

On 02/07/2024 17:07, Niklas Cassel wrote:
> Remove useless wrappers ata_sas_tport_add() and ata_sas_tport_delete().
> 
> Reviewed-by: Hannes Reinecke<hare@suse.de>
> Signed-off-by: Niklas Cassel<cassel@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

[PATCH v3 2/9] ata: libata: Remove unused function declaration for ata_scsi_detect()

[Niklas Cassel]

Remove unused function declaration for ata_scsi_detect().

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 include/linux/libata.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 586f0116d1d7..580971e11804 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1082,7 +1082,6 @@ extern int ata_host_activate(struct ata_host *host, int irq,
 			     const struct scsi_host_template *sht);
 extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *, struct ata_port_operations *);
-extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, unsigned int cmd,
 			  void __user *arg);
 #ifdef CONFIG_COMPAT
-- 
2.45.2

[PATCH v3 3/9] ata: libata-core: Remove support for decreasing the number of ports

[Niklas Cassel]

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.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 22 +++++-----------------
 include/linux/libata.h    |  2 +-
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 74b59b78d278..f0cce3fec902 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5547,24 +5547,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;
@@ -5572,7 +5567,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 * sizeof(void *);
 	host = kzalloc(sz, GFP_KERNEL);
 	if (!host)
 		return NULL;
@@ -5592,11 +5587,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);
@@ -5905,13 +5900,6 @@ 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 (i = host->n_ports; host->ports[i]; i++)
-		ata_port_free(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);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 580971e11804..b7c5d3f33368 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1069,7 +1069,7 @@ extern int sata_std_hardreset(struct ata_link *link, unsigned int *class,
 			      unsigned long deadline);
 extern void ata_std_postreset(struct ata_link *link, unsigned int *classes);
 
-extern struct ata_host *ata_host_alloc(struct device *dev, int max_ports);
+extern struct ata_host *ata_host_alloc(struct device *dev, int n_ports);
 extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 			const struct ata_port_info * const * ppi, int n_ports);
 extern void ata_host_get(struct ata_host *host);
-- 
2.45.2

[PATCH v3 4/9] ata: libata-sata: Remove superfluous assignment in ata_sas_port_alloc()

[Niklas Cassel]

ata_sas_port_alloc() calls ata_port_alloc() which already assigns ap->lock
so there is no need for ata_sas_port_alloc() to assign it again.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
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 e7991595bfe5..1a36a5d1d7bc 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

[PATCH v3 5/9] ata: libata-core: Remove local_port_no struct member

[Niklas Cassel]

ap->local_port_no is simply ap->port_no + 1.
Since ap->local_port_no can be derived from ap->port_no, there is no need
for the ap->local_port_no struct member, so remove ap->local_port_no.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c      | 5 +----
 drivers/ata/libata-transport.c | 3 ++-
 include/linux/libata.h         | 1 -
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f0cce3fec902..aff54651da65 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5463,7 +5463,6 @@ 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_port_no = -1;
 	ap->host = host;
 	ap->dev = host->dev;
 
@@ -5901,10 +5900,8 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
 	}
 
 	/* 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_port_no = 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 d24f201c0ab2..9e24c33388f9 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -217,7 +217,8 @@ static DEVICE_ATTR(name, S_IRUGO, show_ata_port_##name, NULL)
 
 ata_port_simple_attr(nr_pmp_links, nr_pmp_links, "%d\n", int);
 ata_port_simple_attr(stats.idle_irq, idle_irq, "%ld\n", unsigned long);
-ata_port_simple_attr(local_port_no, port_no, "%u\n", unsigned int);
+/* We want the port_no sysfs attibute to start at 1 (ap->port_no starts at 0) */
+ata_port_simple_attr(port_no + 1, port_no, "%u\n", unsigned int);
 
 static DECLARE_TRANSPORT_CLASS(ata_port_class,
 			       "ata_port", NULL, NULL, NULL);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b7c5d3f33368..84a7bfbac9fa 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -814,7 +814,6 @@ 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_port_no; /* host local port num */
 	unsigned int		port_no; /* 0 based port no. inside the host */
 
 #ifdef CONFIG_ATA_SFF
-- 
2.45.2

[PATCH v3 6/9] ata: libata: Assign print_id at port allocation time

[Niklas Cassel]

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.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 6 +-----
 drivers/ata/libata-sata.c | 1 -
 drivers/ata/libata.h      | 1 -
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index aff54651da65..f02c023ba89e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5462,7 +5462,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->host = host;
 	ap->dev = host->dev;
 
@@ -5899,10 +5899,6 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
 		return -EINVAL;
 	}
 
-	/* 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);
-
 	/* Create associated sysfs transport objects  */
 	for (i = 0; i < host->n_ports; i++) {
 		rc = ata_tport_add(host->dev,host->ports[i]);
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 1a36a5d1d7bc..b602247604dc 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.h b/drivers/ata/libata.h
index 38ce13b55474..5ea194ae8a8b 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -32,7 +32,6 @@ enum {
 
 #define ATA_PORT_TYPE_NAME	"ata_port"
 
-extern atomic_t ata_print_id;
 extern int atapi_passthru16;
 extern int libata_fua;
 extern int libata_noacpi;
-- 
2.45.2

[PATCH v3 7/9] ata: libata-core: Reuse available ata_port print_ids

[Niklas Cassel]

Currently, the ata_port print_ids are increased indefinitely, even when
there are lower ids available.

E.g. on first boot you will have ata1-ata6 assigned.
After a rmmod + modprobe, you will instead have ata7-ata12 assigned.

Move to use the ida_alloc() API, such that print_ids will get reused.
This means that even after a rmmod + modprobe, the ports will be assigned
print_ids ata1-ata6.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f02c023ba89e..5031064834be 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -86,7 +86,7 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
 
-atomic_t ata_print_id = ATOMIC_INIT(0);
+static DEFINE_IDA(ata_ida);
 
 #ifdef CONFIG_ATA_FORCE
 struct ata_force_param {
@@ -5455,6 +5455,7 @@ int sata_link_init_spd(struct ata_link *link)
 struct ata_port *ata_port_alloc(struct ata_host *host)
 {
 	struct ata_port *ap;
+	int id;
 
 	ap = kzalloc(sizeof(*ap), GFP_KERNEL);
 	if (!ap)
@@ -5462,7 +5463,12 @@ 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 = atomic_inc_return(&ata_print_id);
+	id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL);
+	if (id < 0) {
+		kfree(ap);
+		return NULL;
+	}
+	ap->print_id = id;
 	ap->host = host;
 	ap->dev = host->dev;
 
@@ -5496,6 +5502,7 @@ void ata_port_free(struct ata_port *ap)
 	kfree(ap->pmp_link);
 	kfree(ap->slave_link);
 	kfree(ap->ncq_sense_buf);
+	ida_free(&ata_ida, ap->print_id);
 	kfree(ap);
 }
 EXPORT_SYMBOL_GPL(ata_port_free);
-- 
2.45.2

[PATCH v3 8/9] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Niklas Cassel]

Now when the ap->print_id assignment has moved to ata_port_alloc(),
we can remove the useless ata_sas_port_alloc() wrapper.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c     |  1 +
 drivers/ata/libata-sata.c     | 35 -----------------------------------
 drivers/ata/libata.h          |  1 -
 drivers/scsi/libsas/sas_ata.c | 10 ++++++++--
 include/linux/libata.h        |  3 +--
 5 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5031064834be..22e7b09c93b1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5493,6 +5493,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 
 	return ap;
 }
+EXPORT_SYMBOL_GPL(ata_port_alloc);
 
 void ata_port_free(struct ata_port *ap)
 {
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index b602247604dc..48660d445602 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1204,41 +1204,6 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
 }
 EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth);
 
-/**
- *	ata_sas_port_alloc - Allocate port for a SAS attached SATA device
- *	@host: ATA host container for all SAS ports
- *	@port_info: Information from low-level host driver
- *	@shost: SCSI host that the scsi device is attached to
- *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
- *
- *	RETURNS:
- *	ata_port pointer on success / NULL on failure.
- */
-
-struct ata_port *ata_sas_port_alloc(struct ata_host *host,
-				    struct ata_port_info *port_info,
-				    struct Scsi_Host *shost)
-{
-	struct ata_port *ap;
-
-	ap = ata_port_alloc(host);
-	if (!ap)
-		return NULL;
-
-	ap->port_no = 0;
-	ap->pio_mask = port_info->pio_mask;
-	ap->mwdma_mask = port_info->mwdma_mask;
-	ap->udma_mask = port_info->udma_mask;
-	ap->flags |= port_info->flags;
-	ap->ops = port_info->port_ops;
-	ap->cbl = ATA_CBL_SATA;
-
-	return ap;
-}
-EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
-
 /**
  *	ata_sas_device_configure - Default device_configure routine for libata
  *				   devices
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 5ea194ae8a8b..6abf265f626e 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -81,7 +81,6 @@ extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp);
 extern int sata_link_init_spd(struct ata_link *link);
 extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
-extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern const char *sata_spd_string(unsigned int spd);
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 				      u8 page, void *buf, unsigned int sectors);
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index ab4ddeea4909..80299f517081 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -597,13 +597,19 @@ int sas_ata_init(struct domain_device *found_dev)
 
 	ata_host_init(ata_host, ha->dev, &sas_sata_ops);
 
-	ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
+	ap = ata_port_alloc(ata_host);
 	if (!ap) {
-		pr_err("ata_sas_port_alloc failed.\n");
+		pr_err("ata_port_alloc failed.\n");
 		rc = -ENODEV;
 		goto free_host;
 	}
 
+	ap->port_no = 0;
+	ap->pio_mask = sata_port_info.pio_mask;
+	ap->mwdma_mask = sata_port_info.mwdma_mask;
+	ap->udma_mask = sata_port_info.udma_mask;
+	ap->flags |= sata_port_info.flags;
+	ap->ops = sata_port_info.port_ops;
 	ap->private_data = found_dev;
 	ap->cbl = ATA_CBL_SATA;
 	ap->scsi_host = shost;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 84a7bfbac9fa..17394098bee9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1244,9 +1244,8 @@ extern int sata_link_debounce(struct ata_link *link,
 extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			     bool spm_wakeup);
 extern int ata_slave_link_init(struct ata_port *ap);
-extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
-					   struct ata_port_info *, struct Scsi_Host *);
 extern void ata_port_probe(struct ata_port *ap);
+extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern void ata_port_free(struct ata_port *ap);
 extern int ata_tport_add(struct device *parent, struct ata_port *ap);
 extern void ata_tport_delete(struct ata_port *ap);
-- 
2.45.2

Re: [PATCH v3 8/9] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[John Garry]

On 02/07/2024 17:08, Niklas Cassel wrote:
> Now when the ap->print_id assignment has moved to ata_port_alloc(),
> we can remove the useless ata_sas_port_alloc() wrapper.

nit: I don't know why you say it is useless. It is used today, so has 
some use, right?

I'd be more inclined to say that it is only used in one location, so can 
be inlined there.

> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c     |  1 +
>   drivers/ata/libata-sata.c     | 35 -----------------------------------
>   drivers/ata/libata.h          |  1 -
>   drivers/scsi/libsas/sas_ata.c | 10 ++++++++--
>   include/linux/libata.h        |  3 +--
>   5 files changed, 10 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 5031064834be..22e7b09c93b1 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5493,6 +5493,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>   
>   	return ap;
>   }
> +EXPORT_SYMBOL_GPL(ata_port_alloc);
>   
>   void ata_port_free(struct ata_port *ap)
>   {
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b602247604dc..48660d445602 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1204,41 +1204,6 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
>   }
>   EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth);
>   
> -/**
> - *	ata_sas_port_alloc - Allocate port for a SAS attached SATA device
> - *	@host: ATA host container for all SAS ports
> - *	@port_info: Information from low-level host driver
> - *	@shost: SCSI host that the scsi device is attached to
> - *
> - *	LOCKING:
> - *	PCI/etc. bus probe sem.
> - *
> - *	RETURNS:
> - *	ata_port pointer on success / NULL on failure.
> - */
> -
> -struct ata_port *ata_sas_port_alloc(struct ata_host *host,
> -				    struct ata_port_info *port_info,
> -				    struct Scsi_Host *shost)
> -{
> -	struct ata_port *ap;
> -
> -	ap = ata_port_alloc(host);
> -	if (!ap)
> -		return NULL;
> -
> -	ap->port_no = 0;
> -	ap->pio_mask = port_info->pio_mask;
> -	ap->mwdma_mask = port_info->mwdma_mask;
> -	ap->udma_mask = port_info->udma_mask;
> -	ap->flags |= port_info->flags;
> -	ap->ops = port_info->port_ops;
> -	ap->cbl = ATA_CBL_SATA;
> -
> -	return ap;
> -}
> -EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
> -
>   /**
>    *	ata_sas_device_configure - Default device_configure routine for libata
>    *				   devices
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 5ea194ae8a8b..6abf265f626e 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -81,7 +81,6 @@ extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp);
>   extern int sata_link_init_spd(struct ata_link *link);
>   extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
>   extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
> -extern struct ata_port *ata_port_alloc(struct ata_host *host);
>   extern const char *sata_spd_string(unsigned int spd);
>   extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>   				      u8 page, void *buf, unsigned int sectors);
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index ab4ddeea4909..80299f517081 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -597,13 +597,19 @@ int sas_ata_init(struct domain_device *found_dev)
>   
>   	ata_host_init(ata_host, ha->dev, &sas_sata_ops);
>   
> -	ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
> +	ap = ata_port_alloc(ata_host);
>   	if (!ap) {
> -		pr_err("ata_sas_port_alloc failed.\n");
> +		pr_err("ata_port_alloc failed.\n");

nit: Are these really useful prints? AFAICS, ata_port_alloc() can only 
fail due to ENOMEM and we generally don't print ENOMEM errors in 
drivers. I know that we change the error code, below, but still I doubt 
its value.

>   		rc = -ENODEV;
>   		goto free_host;
>   	}
>   
> +	ap->port_no = 0;
> +	ap->pio_mask = sata_port_info.pio_mask;

Why do we even have sata_port_info now, if we are not passing a complete 
structure? I mean, why not:

	ap->pio_mask = ATA_PI04;

> +	ap->mwdma_mask = sata_port_info.mwdma_mask;
> +	ap->udma_mask = sata_port_info.udma_mask;
> +	ap->flags |= sata_port_info.flags;
> +	ap->ops = sata_port_info.port_ops;
>   	ap->private_data = found_dev;
>   	ap->cbl = ATA_CBL_SATA;
>   	ap->scsi_host = shost;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 84a7bfbac9fa..17394098bee9 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1244,9 +1244,8 @@ extern int sata_link_debounce(struct ata_link *link,
>   extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>   			     bool spm_wakeup);
>   extern int ata_slave_link_init(struct ata_port *ap);
> -extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> -					   struct ata_port_info *, struct Scsi_Host *);
>   extern void ata_port_probe(struct ata_port *ap);
> +extern struct ata_port *ata_port_alloc(struct ata_host *host);
>   extern void ata_port_free(struct ata_port *ap);
>   extern int ata_tport_add(struct device *parent, struct ata_port *ap);
>   extern void ata_tport_delete(struct ata_port *ap);

Re: [PATCH v3 8/9] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Niklas Cassel]

On Wed, Jul 03, 2024 at 11:20:51AM +0100, John Garry wrote:
> On 02/07/2024 17:08, Niklas Cassel wrote:
> > Now when the ap->print_id assignment has moved to ata_port_alloc(),
> > we can remove the useless ata_sas_port_alloc() wrapper.
> 
> nit: I don't know why you say it is useless. It is used today, so has some
> use, right?
> 
> I'd be more inclined to say that it is only used in one location, so can be
> inlined there.

Sure, I will clarify the commit message.
(I will also clarify the commit message for the other commit that also
says 'remove useless wrappers'.)


> 
> > 
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >   drivers/ata/libata-core.c     |  1 +
> >   drivers/ata/libata-sata.c     | 35 -----------------------------------
> >   drivers/ata/libata.h          |  1 -
> >   drivers/scsi/libsas/sas_ata.c | 10 ++++++++--
> >   include/linux/libata.h        |  3 +--
> >   5 files changed, 10 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 5031064834be..22e7b09c93b1 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5493,6 +5493,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
> >   	return ap;
> >   }
> > +EXPORT_SYMBOL_GPL(ata_port_alloc);
> >   void ata_port_free(struct ata_port *ap)
> >   {
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index b602247604dc..48660d445602 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -1204,41 +1204,6 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
> >   }
> >   EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth);
> > -/**
> > - *	ata_sas_port_alloc - Allocate port for a SAS attached SATA device
> > - *	@host: ATA host container for all SAS ports
> > - *	@port_info: Information from low-level host driver
> > - *	@shost: SCSI host that the scsi device is attached to
> > - *
> > - *	LOCKING:
> > - *	PCI/etc. bus probe sem.
> > - *
> > - *	RETURNS:
> > - *	ata_port pointer on success / NULL on failure.
> > - */
> > -
> > -struct ata_port *ata_sas_port_alloc(struct ata_host *host,
> > -				    struct ata_port_info *port_info,
> > -				    struct Scsi_Host *shost)
> > -{
> > -	struct ata_port *ap;
> > -
> > -	ap = ata_port_alloc(host);
> > -	if (!ap)
> > -		return NULL;
> > -
> > -	ap->port_no = 0;
> > -	ap->pio_mask = port_info->pio_mask;
> > -	ap->mwdma_mask = port_info->mwdma_mask;
> > -	ap->udma_mask = port_info->udma_mask;
> > -	ap->flags |= port_info->flags;
> > -	ap->ops = port_info->port_ops;
> > -	ap->cbl = ATA_CBL_SATA;
> > -
> > -	return ap;
> > -}
> > -EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
> > -
> >   /**
> >    *	ata_sas_device_configure - Default device_configure routine for libata
> >    *				   devices
> > diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> > index 5ea194ae8a8b..6abf265f626e 100644
> > --- a/drivers/ata/libata.h
> > +++ b/drivers/ata/libata.h
> > @@ -81,7 +81,6 @@ extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp);
> >   extern int sata_link_init_spd(struct ata_link *link);
> >   extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
> >   extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
> > -extern struct ata_port *ata_port_alloc(struct ata_host *host);
> >   extern const char *sata_spd_string(unsigned int spd);
> >   extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
> >   				      u8 page, void *buf, unsigned int sectors);
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index ab4ddeea4909..80299f517081 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -597,13 +597,19 @@ int sas_ata_init(struct domain_device *found_dev)
> >   	ata_host_init(ata_host, ha->dev, &sas_sata_ops);
> > -	ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
> > +	ap = ata_port_alloc(ata_host);
> >   	if (!ap) {
> > -		pr_err("ata_sas_port_alloc failed.\n");
> > +		pr_err("ata_port_alloc failed.\n");
> 
> nit: Are these really useful prints? AFAICS, ata_port_alloc() can only fail
> due to ENOMEM and we generally don't print ENOMEM errors in drivers. I know
> that we change the error code, below, but still I doubt its value.

ata_port_alloc() can also fail if there are no free IDs (ida_alloc_min()
returns -ENOSPC), so I will leave the print for now.


> 
> >   		rc = -ENODEV;
> >   		goto free_host;
> >   	}
> > +	ap->port_no = 0;
> > +	ap->pio_mask = sata_port_info.pio_mask;
> 
> Why do we even have sata_port_info now, if we are not passing a complete
> structure? I mean, why not:
> 
> 	ap->pio_mask = ATA_PI04;

Good point, I will remove the structure and perform the initialization
directly, like you are suggesting. This will remove even more lines :)


> 
> > +	ap->mwdma_mask = sata_port_info.mwdma_mask;
> > +	ap->udma_mask = sata_port_info.udma_mask;
> > +	ap->flags |= sata_port_info.flags;
> > +	ap->ops = sata_port_info.port_ops;
> >   	ap->private_data = found_dev;
> >   	ap->cbl = ATA_CBL_SATA;
> >   	ap->scsi_host = shost;
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 84a7bfbac9fa..17394098bee9 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1244,9 +1244,8 @@ extern int sata_link_debounce(struct ata_link *link,
> >   extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
> >   			     bool spm_wakeup);
> >   extern int ata_slave_link_init(struct ata_port *ap);
> > -extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> > -					   struct ata_port_info *, struct Scsi_Host *);
> >   extern void ata_port_probe(struct ata_port *ap);
> > +extern struct ata_port *ata_port_alloc(struct ata_host *host);
> >   extern void ata_port_free(struct ata_port *ap);
> >   extern int ata_tport_add(struct device *parent, struct ata_port *ap);
> >   extern void ata_tport_delete(struct ata_port *ap);
> 

[PATCH v3 9/9] ata: ahci: Add debug print for external port

[Niklas Cassel]

Add a debug print that tells us if LPM is not getting enabled because the
port is external.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
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 fc6fd583faf8..a05c17249448 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;
+	}
 
 	/* If no LPM states are supported by the HBA, do not bother with LPM */
 	if ((ap->host->flags & ATA_HOST_NO_PART) &&
-- 
2.45.2

Re: [PATCH v3 0/9] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel]

On Tue, Jul 02, 2024 at 06:07:56PM +0200, Niklas Cassel wrote:

(snip)

> Niklas Cassel (9):
>   ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()
>   ata: libata: Remove unused function declaration for ata_scsi_detect()
>   ata: libata-core: Remove support for decreasing the number of ports
>   ata: libata-sata: Remove superfluous assignment in
>     ata_sas_port_alloc()
>   ata: libata-core: Remove local_port_no struct member
>   ata: libata: Assign print_id at port allocation time
>   ata: libata-core: Reuse available ata_port print_ids
>   ata,scsi: Remove useless ata_sas_port_alloc() wrapper
>   ata: ahci: Add debug print for external port
> 
>  drivers/ata/ahci.c                 |  4 ++-
>  drivers/ata/libata-core.c          | 41 +++++++++----------------
>  drivers/ata/libata-sata.c          | 49 ------------------------------
>  drivers/ata/libata-transport.c     |  5 ++-
>  drivers/ata/libata-transport.h     |  3 --
>  drivers/ata/libata.h               |  2 --
>  drivers/scsi/libsas/sas_ata.c      | 12 ++++++--
>  drivers/scsi/libsas/sas_discover.c |  2 +-
>  include/linux/libata.h             | 11 +++----
>  9 files changed, 36 insertions(+), 93 deletions(-)

John,
could you please help out with reviews on the patches that touch libsas?
Would have to queue them latest end of the week in order for them to have
at least two weeks in -next.

Martin,
is it okay if we queue this whole series via the libata tree?
Just like the last series, the libsas changes are very small.


Kind regards,
Niklas