[PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel]

Hello all,

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

Patch 1-3 fixes incorrect cleanups in the error paths.
Patch 4,12 removes a useless libata wrappers only used for libsas.
Patch 5 introduces a ata_port_free(), in order to avoid duplicated code.
Patch 6 removes a unused function declaration in include/linux/libata.h.
Patch 7 remove support for decreasing the number of ports, as it is never
        used by any libata driver (including libsas and ipr).
Patch 8 removes a superfluous assignment in ata_sas_port_alloc().
Patch 9 removes the unnecessary local_port_no struct member in ata_port.
Patch 10 performs the ata_port print_id assignment earlier, so that the
         ata_port_* print functions can be used even before the ata_host
	 has been registered.
Patch 11 changes the print_id assignment to use an ida_alloc(), such that
         we will reuse IDs that are no longer in use, rather than keep
	 increasing the print_id forever.
Patch 13 adds a debug print in case the port is marked as external, this
         code runs before the ata_host has been registered, so it depends
	 on patch 10.


Martin, how do you want us to coordinate libsas changes?

You don't seem to have any libsas changes staged for 6.11 so far,
and the libsas changes in this series are quite isolated (and small),
so perhaps we can simply queue them via the libata tree?

Kind regards,
Niklas


Changes since v1:
-Added patches that fixes incorrect cleanups in the error paths.
-Added patches to remove useless libata wrappers only used for libsas.
-Added patch that introduces ata_port_free().
-Added patch that removes a unused function declaration in libata.h.
-Added patch that removes local_port_no (Damien).
-Added patch that assigns the print_id using ida_alloc() (Damien).
-Picked up tags.

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

Niklas Cassel (13):
  ata: libata-core: Fix null pointer dereference on error
  ata: libata-core: Fix double free on error
  ata: ahci: Clean up sysfs file on error
  ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()
  ata,scsi: libata-core: Add ata_port_free()
  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                 | 21 +++++++---
 drivers/ata/libata-core.c          | 66 ++++++++++++++++--------------
 drivers/ata/libata-sata.c          | 49 ----------------------
 drivers/ata/libata-transport.c     |  5 ++-
 drivers/ata/libata-transport.h     |  3 --
 drivers/ata/libata.h               |  1 -
 drivers/scsi/libsas/sas_ata.c      | 14 +++++--
 drivers/scsi/libsas/sas_discover.c |  4 +-
 include/linux/libata.h             | 12 +++---
 9 files changed, 71 insertions(+), 104 deletions(-)

-- 
2.45.2

[PATCH v2 01/13] ata: libata-core: Fix null pointer dereference on error

[Niklas Cassel]

If the ata_port_alloc() call in ata_host_alloc() fails,
ata_host_release() will get called.

However, the code in ata_host_release() tries to free ata_port struct
members unconditionally, which can lead to the following:

BUG: unable to handle page fault for address: 0000000000003990
PGD 0 P4D 0
Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 10 PID: 594 Comm: (udev-worker) Not tainted 6.10.0-rc5 #44
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
RIP: 0010:ata_host_release.cold+0x2f/0x6e [libata]
Code: e4 4d 63 f4 44 89 e2 48 c7 c6 90 ad 32 c0 48 c7 c7 d0 70 33 c0 49 83 c6 0e 41
RSP: 0018:ffffc90000ebb968 EFLAGS: 00010246
RAX: 0000000000000041 RBX: ffff88810fb52e78 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff88813b3218c0 RDI: ffff88813b3218c0
RBP: ffff88810fb52e40 R08: 0000000000000000 R09: 6c65725f74736f68
R10: ffffc90000ebb738 R11: 73692033203a746e R12: 0000000000000004
R13: 0000000000000000 R14: 0000000000000011 R15: 0000000000000006
FS:  00007f6cc55b9980(0000) GS:ffff88813b300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000003990 CR3: 00000001122a2000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 ? __die_body.cold+0x19/0x27
 ? page_fault_oops+0x15a/0x2f0
 ? exc_page_fault+0x7e/0x180
 ? asm_exc_page_fault+0x26/0x30
 ? ata_host_release.cold+0x2f/0x6e [libata]
 ? ata_host_release.cold+0x2f/0x6e [libata]
 release_nodes+0x35/0xb0
 devres_release_group+0x113/0x140
 ata_host_alloc+0xed/0x120 [libata]
 ata_host_alloc_pinfo+0x14/0xa0 [libata]
 ahci_init_one+0x6c9/0xd20 [ahci]

Do not access ata_port struct members unconditionally.

Fixes: 633273a3ed1c ("libata-pmp: hook PMP support and enable it")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e1bf8a19b3c8..88e32f638f33 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5518,10 +5518,12 @@ static void ata_host_release(struct kref *kref)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-		kfree(ap->pmp_link);
-		kfree(ap->slave_link);
-		kfree(ap->ncq_sense_buf);
-		kfree(ap);
+		if (ap) {
+			kfree(ap->pmp_link);
+			kfree(ap->slave_link);
+			kfree(ap->ncq_sense_buf);
+			kfree(ap);
+		}
 		host->ports[i] = NULL;
 	}
 	kfree(host);
-- 
2.45.2

Re: [PATCH v2 01/13] ata: libata-core: Fix null pointer dereference on error

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> If the ata_port_alloc() call in ata_host_alloc() fails,
> ata_host_release() will get called.
> 
> However, the code in ata_host_release() tries to free ata_port struct
> members unconditionally, which can lead to the following:
> 
> BUG: unable to handle page fault for address: 0000000000003990
> PGD 0 P4D 0
> Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 10 PID: 594 Comm: (udev-worker) Not tainted 6.10.0-rc5 #44
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> RIP: 0010:ata_host_release.cold+0x2f/0x6e [libata]
> Code: e4 4d 63 f4 44 89 e2 48 c7 c6 90 ad 32 c0 48 c7 c7 d0 70 33 c0 49 83 c6 0e 41
> RSP: 0018:ffffc90000ebb968 EFLAGS: 00010246
> RAX: 0000000000000041 RBX: ffff88810fb52e78 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff88813b3218c0 RDI: ffff88813b3218c0
> RBP: ffff88810fb52e40 R08: 0000000000000000 R09: 6c65725f74736f68
> R10: ffffc90000ebb738 R11: 73692033203a746e R12: 0000000000000004
> R13: 0000000000000000 R14: 0000000000000011 R15: 0000000000000006
> FS:  00007f6cc55b9980(0000) GS:ffff88813b300000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000003990 CR3: 00000001122a2000 CR4: 0000000000750ef0
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  ? __die_body.cold+0x19/0x27
>  ? page_fault_oops+0x15a/0x2f0
>  ? exc_page_fault+0x7e/0x180
>  ? asm_exc_page_fault+0x26/0x30
>  ? ata_host_release.cold+0x2f/0x6e [libata]
>  ? ata_host_release.cold+0x2f/0x6e [libata]
>  release_nodes+0x35/0xb0
>  devres_release_group+0x113/0x140
>  ata_host_alloc+0xed/0x120 [libata]
>  ata_host_alloc_pinfo+0x14/0xa0 [libata]
>  ahci_init_one+0x6c9/0xd20 [ahci]
> 
> Do not access ata_port struct members unconditionally.
> 
> Fixes: 633273a3ed1c ("libata-pmp: hook PMP support and enable it")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good, with a nit below. This should be queued as soon as possible as a
6.10 fix patch.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  drivers/ata/libata-core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e1bf8a19b3c8..88e32f638f33 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5518,10 +5518,12 @@ static void ata_host_release(struct kref *kref)
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
>  
> -		kfree(ap->pmp_link);
> -		kfree(ap->slave_link);
> -		kfree(ap->ncq_sense_buf);
> -		kfree(ap);
> +		if (ap) {
> +			kfree(ap->pmp_link);
> +			kfree(ap->slave_link);
> +			kfree(ap->ncq_sense_buf);
> +			kfree(ap);
> +		}
>  		host->ports[i] = NULL;

Nit: this line can go inside the if as well. Or even better: reverse the if
condition and continue to ignore NULL ports.

	for (i = 0; i < host->n_ports; i++) {
  		struct ata_port *ap = host->ports[i];

		if (!ap)
			continue;

		kfree(ap->pmp_link);
		kfree(ap->slave_link);
		kfree(ap->ncq_sense_buf);
		kfree(ap);
		host->ports[i] = NULL;
	}

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 01/13] ata: libata-core: Fix null pointer dereference on error

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> If the ata_port_alloc() call in ata_host_alloc() fails,
> ata_host_release() will get called.
> 
> However, the code in ata_host_release() tries to free ata_port struct
> members unconditionally, which can lead to the following:
> 
> BUG: unable to handle page fault for address: 0000000000003990
> PGD 0 P4D 0
> Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 10 PID: 594 Comm: (udev-worker) Not tainted 6.10.0-rc5 #44
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> RIP: 0010:ata_host_release.cold+0x2f/0x6e [libata]
> Code: e4 4d 63 f4 44 89 e2 48 c7 c6 90 ad 32 c0 48 c7 c7 d0 70 33 c0 49 83 c6 0e 41
> RSP: 0018:ffffc90000ebb968 EFLAGS: 00010246
> RAX: 0000000000000041 RBX: ffff88810fb52e78 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff88813b3218c0 RDI: ffff88813b3218c0
> RBP: ffff88810fb52e40 R08: 0000000000000000 R09: 6c65725f74736f68
> R10: ffffc90000ebb738 R11: 73692033203a746e R12: 0000000000000004
> R13: 0000000000000000 R14: 0000000000000011 R15: 0000000000000006
> FS:  00007f6cc55b9980(0000) GS:ffff88813b300000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000003990 CR3: 00000001122a2000 CR4: 0000000000750ef0
> PKRU: 55555554
> Call Trace:
>   <TASK>
>   ? __die_body.cold+0x19/0x27
>   ? page_fault_oops+0x15a/0x2f0
>   ? exc_page_fault+0x7e/0x180
>   ? asm_exc_page_fault+0x26/0x30
>   ? ata_host_release.cold+0x2f/0x6e [libata]
>   ? ata_host_release.cold+0x2f/0x6e [libata]
>   release_nodes+0x35/0xb0
>   devres_release_group+0x113/0x140
>   ata_host_alloc+0xed/0x120 [libata]
>   ata_host_alloc_pinfo+0x14/0xa0 [libata]
>   ahci_init_one+0x6c9/0xd20 [ahci]
> 
> Do not access ata_port struct members unconditionally.
> 
> Fixes: 633273a3ed1c ("libata-pmp: hook PMP support and enable it")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e1bf8a19b3c8..88e32f638f33 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5518,10 +5518,12 @@ static void ata_host_release(struct kref *kref)
>   	for (i = 0; i < host->n_ports; i++) {
>   		struct ata_port *ap = host->ports[i];
>   
> -		kfree(ap->pmp_link);
> -		kfree(ap->slave_link);
> -		kfree(ap->ncq_sense_buf);
> -		kfree(ap);
> +		if (ap) {
> +			kfree(ap->pmp_link);
> +			kfree(ap->slave_link);
> +			kfree(ap->ncq_sense_buf);
> +			kfree(ap);
> +		}
>   		host->ports[i] = NULL;
>   	}
>   	kfree(host);
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 02/13] ata: libata-core: Fix double free on error

[Niklas Cassel]

If e.g. the ata_port_alloc() call in ata_host_alloc() fails, we will jump
to the err_out label, which will call devres_release_group().
devres_release_group() will trigger a call to ata_host_release().
ata_host_release() calls kfree(host), so executing the kfree(host) in
ata_host_alloc() will lead to a double free:

kernel BUG at mm/slub.c:553!
Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 11 PID: 599 Comm: (udev-worker) Not tainted 6.10.0-rc5 #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
RIP: 0010:kfree+0x2cf/0x2f0
Code: 5d 41 5e 41 5f 5d e9 80 d6 ff ff 4d 89 f1 41 b8 01 00 00 00 48 89 d9 48 89 da
RSP: 0018:ffffc90000f377f0 EFLAGS: 00010246
RAX: ffff888112b1f2c0 RBX: ffff888112b1f2c0 RCX: ffff888112b1f320
RDX: 000000000000400b RSI: ffffffffc02c9de5 RDI: ffff888112b1f2c0
RBP: ffffc90000f37830 R08: 0000000000000000 R09: 0000000000000000
R10: ffffc90000f37610 R11: 617461203a736b6e R12: ffffea00044ac780
R13: ffff888100046400 R14: ffffffffc02c9de5 R15: 0000000000000006
FS:  00007f2f1cabe980(0000) GS:ffff88813b380000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f2f1c3acf75 CR3: 0000000111724000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 ? __die_body.cold+0x19/0x27
 ? die+0x2e/0x50
 ? do_trap+0xca/0x110
 ? do_error_trap+0x6a/0x90
 ? kfree+0x2cf/0x2f0
 ? exc_invalid_op+0x50/0x70
 ? kfree+0x2cf/0x2f0
 ? asm_exc_invalid_op+0x1a/0x20
 ? ata_host_alloc+0xf5/0x120 [libata]
 ? ata_host_alloc+0xf5/0x120 [libata]
 ? kfree+0x2cf/0x2f0
 ata_host_alloc+0xf5/0x120 [libata]
 ata_host_alloc_pinfo+0x14/0xa0 [libata]
 ahci_init_one+0x6c9/0xd20 [ahci]

Ensure that we will not call kfree(host) twice, by performing the kfree()
only if the devres_open_group() call failed.

Fixes: dafd6c496381 ("libata: ensure host is free'd on error exit paths")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 88e32f638f33..c916cbe3e099 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5573,8 +5573,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 	if (!host)
 		return NULL;
 
-	if (!devres_open_group(dev, NULL, GFP_KERNEL))
-		goto err_free;
+	if (!devres_open_group(dev, NULL, GFP_KERNEL)) {
+		kfree(host);
+		return NULL;
+	}
 
 	dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL);
 	if (!dr)
@@ -5606,8 +5608,6 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 
  err_out:
 	devres_release_group(dev, NULL);
- err_free:
-	kfree(host);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(ata_host_alloc);
-- 
2.45.2

Re: [PATCH v2 02/13] ata: libata-core: Fix double free on error

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> If e.g. the ata_port_alloc() call in ata_host_alloc() fails, we will jump
> to the err_out label, which will call devres_release_group().
> devres_release_group() will trigger a call to ata_host_release().
> ata_host_release() calls kfree(host), so executing the kfree(host) in
> ata_host_alloc() will lead to a double free:
> 
> kernel BUG at mm/slub.c:553!
> Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 11 PID: 599 Comm: (udev-worker) Not tainted 6.10.0-rc5 #47
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> RIP: 0010:kfree+0x2cf/0x2f0
> Code: 5d 41 5e 41 5f 5d e9 80 d6 ff ff 4d 89 f1 41 b8 01 00 00 00 48 89 d9 48 89 da
> RSP: 0018:ffffc90000f377f0 EFLAGS: 00010246
> RAX: ffff888112b1f2c0 RBX: ffff888112b1f2c0 RCX: ffff888112b1f320
> RDX: 000000000000400b RSI: ffffffffc02c9de5 RDI: ffff888112b1f2c0
> RBP: ffffc90000f37830 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffc90000f37610 R11: 617461203a736b6e R12: ffffea00044ac780
> R13: ffff888100046400 R14: ffffffffc02c9de5 R15: 0000000000000006
> FS:  00007f2f1cabe980(0000) GS:ffff88813b380000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f2f1c3acf75 CR3: 0000000111724000 CR4: 0000000000750ef0
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  ? __die_body.cold+0x19/0x27
>  ? die+0x2e/0x50
>  ? do_trap+0xca/0x110
>  ? do_error_trap+0x6a/0x90
>  ? kfree+0x2cf/0x2f0
>  ? exc_invalid_op+0x50/0x70
>  ? kfree+0x2cf/0x2f0
>  ? asm_exc_invalid_op+0x1a/0x20
>  ? ata_host_alloc+0xf5/0x120 [libata]
>  ? ata_host_alloc+0xf5/0x120 [libata]
>  ? kfree+0x2cf/0x2f0
>  ata_host_alloc+0xf5/0x120 [libata]
>  ata_host_alloc_pinfo+0x14/0xa0 [libata]
>  ahci_init_one+0x6c9/0xd20 [ahci]
> 
> Ensure that we will not call kfree(host) twice, by performing the kfree()
> only if the devres_open_group() call failed.
> 
> Fixes: dafd6c496381 ("libata: ensure host is free'd on error exit paths")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 02/13] ata: libata-core: Fix double free on error

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> If e.g. the ata_port_alloc() call in ata_host_alloc() fails, we will jump
> to the err_out label, which will call devres_release_group().
> devres_release_group() will trigger a call to ata_host_release().
> ata_host_release() calls kfree(host), so executing the kfree(host) in
> ata_host_alloc() will lead to a double free:
> 
> kernel BUG at mm/slub.c:553!
> Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 11 PID: 599 Comm: (udev-worker) Not tainted 6.10.0-rc5 #47
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> RIP: 0010:kfree+0x2cf/0x2f0
> Code: 5d 41 5e 41 5f 5d e9 80 d6 ff ff 4d 89 f1 41 b8 01 00 00 00 48 89 d9 48 89 da
> RSP: 0018:ffffc90000f377f0 EFLAGS: 00010246
> RAX: ffff888112b1f2c0 RBX: ffff888112b1f2c0 RCX: ffff888112b1f320
> RDX: 000000000000400b RSI: ffffffffc02c9de5 RDI: ffff888112b1f2c0
> RBP: ffffc90000f37830 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffc90000f37610 R11: 617461203a736b6e R12: ffffea00044ac780
> R13: ffff888100046400 R14: ffffffffc02c9de5 R15: 0000000000000006
> FS:  00007f2f1cabe980(0000) GS:ffff88813b380000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f2f1c3acf75 CR3: 0000000111724000 CR4: 0000000000750ef0
> PKRU: 55555554
> Call Trace:
>   <TASK>
>   ? __die_body.cold+0x19/0x27
>   ? die+0x2e/0x50
>   ? do_trap+0xca/0x110
>   ? do_error_trap+0x6a/0x90
>   ? kfree+0x2cf/0x2f0
>   ? exc_invalid_op+0x50/0x70
>   ? kfree+0x2cf/0x2f0
>   ? asm_exc_invalid_op+0x1a/0x20
>   ? ata_host_alloc+0xf5/0x120 [libata]
>   ? ata_host_alloc+0xf5/0x120 [libata]
>   ? kfree+0x2cf/0x2f0
>   ata_host_alloc+0xf5/0x120 [libata]
>   ata_host_alloc_pinfo+0x14/0xa0 [libata]
>   ahci_init_one+0x6c9/0xd20 [ahci]
> 
> Ensure that we will not call kfree(host) twice, by performing the kfree()
> only if the devres_open_group() call failed.
> 
> Fixes: dafd6c496381 ("libata: ensure host is free'd on error exit paths")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 88e32f638f33..c916cbe3e099 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5573,8 +5573,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
>   	if (!host)
>   		return NULL;
>   
> -	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> -		goto err_free;
> +	if (!devres_open_group(dev, NULL, GFP_KERNEL)) {
> +		kfree(host);
> +		return NULL;
> +	}
>   
>   	dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL);
>   	if (!dr)
> @@ -5606,8 +5608,6 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
>   
>    err_out:
>   	devres_release_group(dev, NULL);
> - err_free:
> -	kfree(host);
>   	return NULL;
>   }
>   EXPORT_SYMBOL_GPL(ata_host_alloc);
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 03/13] ata: ahci: Clean up sysfs file on error

[Niklas Cassel]

.probe() (ahci_init_one()) calls sysfs_add_file_to_group(), however,
if probe() fails after this call, we currently never call
sysfs_remove_file_from_group().

(The sysfs_remove_file_from_group() call in .remove() (ahci_remove_one())
does not help, as .remove() is not called on .probe() error.)

Thus, if probe() fails after the sysfs_add_file_to_group() call, we get:

sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:04.0/remapped_nvme'
CPU: 11 PID: 954 Comm: modprobe Not tainted 6.10.0-rc5 #43
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x5d/0x80
 sysfs_warn_dup.cold+0x17/0x23
 sysfs_add_file_mode_ns+0x11a/0x130
 sysfs_add_file_to_group+0x7e/0xc0
 ahci_init_one+0x31f/0xd40 [ahci]

Fixes: 894fba7f434a ("ata: ahci: Add sysfs attribute to show remapped NVMe device count")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 5eb38fbbbecd..fc6fd583faf8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1975,8 +1975,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
-	if (!host)
-		return -ENOMEM;
+	if (!host) {
+		rc = -ENOMEM;
+		goto err_rm_sysfs_file;
+	}
 	host->private_data = hpriv;
 
 	if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
@@ -2031,11 +2033,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* initialize adapter */
 	rc = ahci_configure_dma_masks(pdev, hpriv);
 	if (rc)
-		return rc;
+		goto err_rm_sysfs_file;
 
 	rc = ahci_pci_reset_controller(host);
 	if (rc)
-		return rc;
+		goto err_rm_sysfs_file;
 
 	ahci_pci_init_controller(host);
 	ahci_pci_print_info(host);
@@ -2044,10 +2046,15 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	rc = ahci_host_activate(host, &ahci_sht);
 	if (rc)
-		return rc;
+		goto err_rm_sysfs_file;
 
 	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
+
+err_rm_sysfs_file:
+	sysfs_remove_file_from_group(&pdev->dev.kobj,
+				     &dev_attr_remapped_nvme.attr, NULL);
+	return rc;
 }
 
 static void ahci_shutdown_one(struct pci_dev *pdev)
-- 
2.45.2

Re: [PATCH v2 03/13] ata: ahci: Clean up sysfs file on error

[Niklas Cassel]

On Wed, Jun 26, 2024 at 08:00:33PM +0200, Niklas Cassel wrote:
> .probe() (ahci_init_one()) calls sysfs_add_file_to_group(), however,
> if probe() fails after this call, we currently never call
> sysfs_remove_file_from_group().
> 
> (The sysfs_remove_file_from_group() call in .remove() (ahci_remove_one())
> does not help, as .remove() is not called on .probe() error.)
> 
> Thus, if probe() fails after the sysfs_add_file_to_group() call, we get:

Nit:
s/we get/the next time we insmod the module we will get/

Re: [PATCH v2 03/13] ata: ahci: Clean up sysfs file on error

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> .probe() (ahci_init_one()) calls sysfs_add_file_to_group(), however,
> if probe() fails after this call, we currently never call
> sysfs_remove_file_from_group().
> 
> (The sysfs_remove_file_from_group() call in .remove() (ahci_remove_one())
> does not help, as .remove() is not called on .probe() error.)
> 
> Thus, if probe() fails after the sysfs_add_file_to_group() call, we get:
> 
> sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:04.0/remapped_nvme'
> CPU: 11 PID: 954 Comm: modprobe Not tainted 6.10.0-rc5 #43
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x5d/0x80
>  sysfs_warn_dup.cold+0x17/0x23
>  sysfs_add_file_mode_ns+0x11a/0x130
>  sysfs_add_file_to_group+0x7e/0xc0
>  ahci_init_one+0x31f/0xd40 [ahci]
> 
> Fixes: 894fba7f434a ("ata: ahci: Add sysfs attribute to show remapped NVMe device count")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good. And I think same as patch 1 and 2: let's send this out as a 6.10 fix.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 03/13] ata: ahci: Clean up sysfs file on error

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> .probe() (ahci_init_one()) calls sysfs_add_file_to_group(), however,
> if probe() fails after this call, we currently never call
> sysfs_remove_file_from_group().
> 
> (The sysfs_remove_file_from_group() call in .remove() (ahci_remove_one())
> does not help, as .remove() is not called on .probe() error.)
> 
> Thus, if probe() fails after the sysfs_add_file_to_group() call, we get:
> 
> sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:04.0/remapped_nvme'
> CPU: 11 PID: 954 Comm: modprobe Not tainted 6.10.0-rc5 #43
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0x5d/0x80
>   sysfs_warn_dup.cold+0x17/0x23
>   sysfs_add_file_mode_ns+0x11a/0x130
>   sysfs_add_file_to_group+0x7e/0xc0
>   ahci_init_one+0x31f/0xd40 [ahci]
> 
> Fixes: 894fba7f434a ("ata: ahci: Add sysfs attribute to show remapped NVMe device count")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/ahci.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 


Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 04/13] ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()

[Niklas Cassel]

Remove useless wrappers ata_sas_tport_add() and ata_sas_tport_delete().

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 4c69fc63c119..1c2400c96ebd 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 destroy_port;
 
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 8fb7c41c0962..6e01ddec10c9 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);
 		kfree(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 13fb41d25da6..581e166615fa 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1249,8 +1249,8 @@ 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 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 v2 04/13] ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> Remove useless wrappers ata_sas_tport_add() and ata_sas_tport_delete().

I am not a big fan of this patch. I would prefer to keep everything in
libata-transport local to libata and have the exports done for what libsas needs
using the wrappers, even if some of them are very trivial...

That also allows keeping the naming consistent with the ata_sas_ prefix for
everything that libsas uses from libata.

John ? Thoughts ?

> 
> 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 4c69fc63c119..1c2400c96ebd 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 destroy_port;
>  
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 8fb7c41c0962..6e01ddec10c9 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);
>  		kfree(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 13fb41d25da6..581e166615fa 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1249,8 +1249,8 @@ 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 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);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 04/13] ata,scsi: Remove useless wrappers ata_sas_tport_{add,delete}()

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> Remove useless wrappers ata_sas_tport_add() and ata_sas_tport_delete().
> 
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 05/13] ata,scsi: libata-core: Add ata_port_free()

[Niklas Cassel]

Add a function, ata_port_free(), that is used to free a ata_port.
It makes sense to keep the code related to freeing a ata_port in its own
function, which will also free all the struct members of struct ata_port.

libsas is currently not freeing all the struct ata_port struct members,
e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c          | 19 +++++++++++++------
 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_discover.c |  2 +-
 include/linux/libata.h             |  1 +
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c916cbe3e099..591020ea8989 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5490,6 +5490,18 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	return ap;
 }
 
+void ata_port_free(struct ata_port *ap)
+{
+	if (!ap)
+		return;
+
+	kfree(ap->pmp_link);
+	kfree(ap->slave_link);
+	kfree(ap->ncq_sense_buf);
+	kfree(ap);
+}
+EXPORT_SYMBOL_GPL(ata_port_free);
+
 static void ata_devres_release(struct device *gendev, void *res)
 {
 	struct ata_host *host = dev_get_drvdata(gendev);
@@ -5518,12 +5530,7 @@ static void ata_host_release(struct kref *kref)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-		if (ap) {
-			kfree(ap->pmp_link);
-			kfree(ap->slave_link);
-			kfree(ap->ncq_sense_buf);
-			kfree(ap);
-		}
+		ata_port_free(ap);
 		host->ports[i] = NULL;
 	}
 	kfree(host);
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 1c2400c96ebd..e8987dce585f 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -618,7 +618,7 @@ int sas_ata_init(struct domain_device *found_dev)
 	return 0;
 
 destroy_port:
-	kfree(ap);
+	ata_port_free(ap);
 free_host:
 	ata_host_put(ata_host);
 	return rc;
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 6e01ddec10c9..951bdc554a10 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
 
 	if (dev_is_sata(dev) && dev->sata_dev.ap) {
 		ata_tport_delete(dev->sata_dev.ap);
-		kfree(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;
 		dev->sata_dev.ap = NULL;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 581e166615fa..586f0116d1d7 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1249,6 +1249,7 @@ 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 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);
 int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
-- 
2.45.2

Re: [PATCH v2 05/13] ata,scsi: libata-core: Add ata_port_free()

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> Add a function, ata_port_free(), that is used to free a ata_port.
> It makes sense to keep the code related to freeing a ata_port in its own
> function, which will also free all the struct members of struct ata_port.
> 
> libsas is currently not freeing all the struct ata_port struct members,
> e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).

This part should be a separate fix patch and sent out this cycle.

> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

> @@ -5518,12 +5530,7 @@ static void ata_host_release(struct kref *kref)
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
>  
> -		if (ap) {
> -			kfree(ap->pmp_link);
> -			kfree(ap->slave_link);
> -			kfree(ap->ncq_sense_buf);
> -			kfree(ap);
> -		}
> +		ata_port_free(ap);

The variable "ap" can be removed too.

>  		host->ports[i] = NULL;
>  	}
>  	kfree(host);

> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 6e01ddec10c9..951bdc554a10 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
>  
>  	if (dev_is_sata(dev) && dev->sata_dev.ap) {
>  		ata_tport_delete(dev->sata_dev.ap);
> -		kfree(dev->sata_dev.ap);
> +		ata_port_free(dev->sata_dev.ap);

Why not have this inside ata_tport_delete() ?

>  		ata_host_put(dev->sata_dev.ata_host);
>  		dev->sata_dev.ata_host = NULL;
>  		dev->sata_dev.ap = NULL;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 581e166615fa..586f0116d1d7 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1249,6 +1249,7 @@ 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 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);
>  int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 05/13] ata,scsi: libata-core: Add ata_port_free()

[Niklas Cassel]

On Thu, Jun 27, 2024 at 10:15:37AM +0900, Damien Le Moal wrote:
> On 6/27/24 03:00, Niklas Cassel wrote:
> > Add a function, ata_port_free(), that is used to free a ata_port.
> > It makes sense to keep the code related to freeing a ata_port in its own
> > function, which will also free all the struct members of struct ata_port.
> > 
> > libsas is currently not freeing all the struct ata_port struct members,
> > e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).
> 
> This part should be a separate fix patch and sent out this cycle.
> 
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> > @@ -5518,12 +5530,7 @@ static void ata_host_release(struct kref *kref)
> >  	for (i = 0; i < host->n_ports; i++) {
> >  		struct ata_port *ap = host->ports[i];
> >  
> > -		if (ap) {
> > -			kfree(ap->pmp_link);
> > -			kfree(ap->slave_link);
> > -			kfree(ap->ncq_sense_buf);
> > -			kfree(ap);
> > -		}
> > +		ata_port_free(ap);
> 
> The variable "ap" can be removed too.
> 
> >  		host->ports[i] = NULL;
> >  	}
> >  	kfree(host);
> 
> > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> > index 6e01ddec10c9..951bdc554a10 100644
> > --- a/drivers/scsi/libsas/sas_discover.c
> > +++ b/drivers/scsi/libsas/sas_discover.c
> > @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
> >  
> >  	if (dev_is_sata(dev) && dev->sata_dev.ap) {
> >  		ata_tport_delete(dev->sata_dev.ap);
> > -		kfree(dev->sata_dev.ap);
> > +		ata_port_free(dev->sata_dev.ap);
> 
> Why not have this inside ata_tport_delete() ?

In the current code, the allocating and freeing of the ports
are done separately from adding and deleting a transport.

The tport_del will be called by:
ahci_remove_one()

drivers/base/dd.c:__device_release_driver() will call
device_remove() which will call ahci_remove_one().

ahci_remove_one() (.remove()), will call ata_host_detach(), which calls
ata_port_detach(), which calls ata_tport_delete().

This will not free the port however. That is instead done even later, by:
ata_host_release() will be called even later:
drivers/base/dd.c:__device_release_driver() will call
device_unbind_cleanup(), which will call ata_host_release().

We could evaluate if we want to change this, since ALL libata driver do
call tport_add()/tport_delete(), but since this is a fix patch, I would
rather not do fundamental changes like that in this patch.


> 
> >  		ata_host_put(dev->sata_dev.ata_host);
> >  		dev->sata_dev.ata_host = NULL;
> >  		dev->sata_dev.ap = NULL;
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 581e166615fa..586f0116d1d7 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1249,6 +1249,7 @@ 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 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);
> >  int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

Re: [PATCH v2 05/13] ata,scsi: libata-core: Add ata_port_free()

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> Add a function, ata_port_free(), that is used to free a ata_port.
> It makes sense to keep the code related to freeing a ata_port in its own
> function, which will also free all the struct members of struct ata_port.
> 
> libsas is currently not freeing all the struct ata_port struct members,
> e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c          | 19 +++++++++++++------
>   drivers/scsi/libsas/sas_ata.c      |  2 +-
>   drivers/scsi/libsas/sas_discover.c |  2 +-
>   include/linux/libata.h             |  1 +
>   4 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index c916cbe3e099..591020ea8989 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5490,6 +5490,18 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>   	return ap;
>   }
>   
> +void ata_port_free(struct ata_port *ap)
> +{
> +	if (!ap)
> +		return;
> +
> +	kfree(ap->pmp_link);
> +	kfree(ap->slave_link);
> +	kfree(ap->ncq_sense_buf);
> +	kfree(ap);
> +}
> +EXPORT_SYMBOL_GPL(ata_port_free);
> +
>   static void ata_devres_release(struct device *gendev, void *res)
>   {
>   	struct ata_host *host = dev_get_drvdata(gendev);
> @@ -5518,12 +5530,7 @@ static void ata_host_release(struct kref *kref)
>   	for (i = 0; i < host->n_ports; i++) {
>   		struct ata_port *ap = host->ports[i];
>   
> -		if (ap) {
> -			kfree(ap->pmp_link);
> -			kfree(ap->slave_link);
> -			kfree(ap->ncq_sense_buf);
> -			kfree(ap);
> -		}
> +		ata_port_free(ap);
>   		host->ports[i] = NULL;
>   	}
>   	kfree(host);

Can't you squish this into the first patch?

Otherwise looks good.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 06/13] ata: libata: Remove unused function declaration for ata_scsi_detect()

[Niklas Cassel]

Remove unsed function declaration for ata_scsi_detect().

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

Re: [PATCH v2 06/13] ata: libata: Remove unused function declaration for ata_scsi_detect()

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> Remove unsed function declaration for ata_scsi_detect().

s/unsed/unused

With that:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 06/13] ata: libata: Remove unused function declaration for ata_scsi_detect()

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> Remove unsed function declaration for ata_scsi_detect().
> 
> 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

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 07/13] 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.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 24 ++++++++++--------------
 include/linux/libata.h    |  2 +-
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 591020ea8989..a213a9c0d0a5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5550,24 +5550,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;
@@ -5575,7 +5570,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;
@@ -5595,11 +5590,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);
@@ -5908,12 +5903,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++) {
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

Re: [PATCH v2 07/13] ata: libata-core: Remove support for decreasing the number of ports

[Niklas Cassel]

On Wed, Jun 26, 2024 at 08:00:37PM +0200, Niklas Cassel wrote:
> @@ -5908,12 +5903,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]);

Nit:
Even though we replace the kfree() with a WARN_ON() here, the strictly
correct thing would have been for the earlier patch in this series:
"ata,scsi: libata-core: Add ata_port_free()" to have replaced the kfree()
with ata_port_free(), and then for this patch to replace the ata_port_free()
with a WARN_ON().


Kind regards,
Niklas

Re: [PATCH v2 07/13] ata: libata-core: Remove support for decreasing the number of ports

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> 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>

With your own nit applied,

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 07/13] ata: libata-core: Remove support for decreasing the number of ports

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> 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 ++++++++++--------------
>   include/linux/libata.h    |  2 +-
>   2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 591020ea8989..a213a9c0d0a5 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5550,24 +5550,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;
> @@ -5575,7 +5570,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;
> @@ -5595,11 +5590,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);
> @@ -5908,12 +5903,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]);
>   
What a patently ugly check.
So you are relying on the caller to have zeroed the memory upfront.
But what happens if the caller allocated n_ports, zeroed the memory up 
to that point, and then filled in all 'ports' slots?
ports[n_ports - 1] is set to a pointer, but ports[n_ports] is _not_ 
allocated, and there is no guarantee it'll be zero.
Causing a memory overrun and all sorts of things.

This needs to go, as it's now pointless anyway.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 07/13] ata: libata-core: Remove support for decreasing the number of ports

[Niklas Cassel]

On Thu, Jun 27, 2024 at 08:35:49AM +0200, Hannes Reinecke wrote:
> On 6/26/24 20:00, Niklas Cassel wrote:
> > 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 ++++++++++--------------
> >   include/linux/libata.h    |  2 +-
> >   2 files changed, 11 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 591020ea8989..a213a9c0d0a5 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5550,24 +5550,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;
> > @@ -5575,7 +5570,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;
> > @@ -5595,11 +5590,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);
> > @@ -5908,12 +5903,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]);
> What a patently ugly check.
> So you are relying on the caller to have zeroed the memory upfront.
> But what happens if the caller allocated n_ports, zeroed the memory up to
> that point, and then filled in all 'ports' slots?
> ports[n_ports - 1] is set to a pointer, but ports[n_ports] is _not_
> allocated, and there is no guarantee it'll be zero.
> Causing a memory overrun and all sorts of things.
> 
> This needs to go, as it's now pointless anyway.

For what it is worth, this ugly code was there before this patch :)

However, it seems that ata_host_alloc() allocates max_ports + 1:
https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-core.c#L5568-L5570

So I think this should be safe....

But yes, super ugly...


Kind regards,
Niklas

[PATCH v2 08/13] 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 to ata_sas_port_alloc() to assign it again.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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

Re: [PATCH v2 08/13] ata: libata-sata: Remove superfluous assignment in ata_sas_port_alloc()

[Damien Le Moal]

On 6/27/24 03:00, 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.

Nit: s/need to/need for

> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 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;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 08/13] ata: libata-sata: Remove superfluous assignment in ata_sas_port_alloc()

[Hannes Reinecke]

On 6/26/24 20:00, 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.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 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;

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 09/13] 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.

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 a213a9c0d0a5..ceee4b6ba3dd 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5464,7 +5464,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;
 
@@ -5912,10 +5911,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++) {
+	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

Re: [PATCH v2 09/13] ata: libata-core: Remove local_port_no struct member

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> 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.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 09/13] ata: libata-core: Remove local_port_no struct member

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> 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.
> 
> 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 a213a9c0d0a5..ceee4b6ba3dd 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5464,7 +5464,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;
>   
> @@ -5912,10 +5911,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++) {
> +	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

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 10/13] 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>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 6 +-----
 drivers/ata/libata-sata.c | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ceee4b6ba3dd..52c1f0915aef 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->host = host;
 	ap->dev = host->dev;
 
@@ -5910,10 +5910,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]->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;
 }
-- 
2.45.2

Re: [PATCH v2 10/13] ata: libata: Assign print_id at port allocation time

[Hannes Reinecke]

On 6/26/24 20:00, 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.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c | 6 +-----
>   drivers/ata/libata-sata.c | 1 -
>   2 files changed, 1 insertion(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 11/13] 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.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 52c1f0915aef..846ab99e0cd3 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 {
@@ -5463,7 +5463,11 @@ 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);
+	ap->print_id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL);
+	if (ap->print_id < 0) {
+		kfree(ap);
+		return NULL;
+	}
 	ap->host = host;
 	ap->dev = host->dev;
 
@@ -5497,6 +5501,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

Re: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> 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.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good. But maybe it would make sense to squash this together with patch 10 ?

Either way,

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids

[Niklas Cassel]

On Thu, Jun 27, 2024 at 10:37:40AM +0900, Damien Le Moal wrote:
> On 6/27/24 03:00, Niklas Cassel wrote:
> > 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.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Looks good. But maybe it would make sense to squash this together with patch 10 ?

Patch 10 initializes the print_ids earlier (which is a perfectly
fine change on its own, even with the old way to assign print_ids),
while this patch changes for the print_ids to be reusable.
I think that logically, it is two different logical changes
so I will keep them as separate patches in v3.


Kind regards,
Niklas

Re: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> 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.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids

[kernel test robot]

Hi Niklas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc5 next-20240627]
[cannot apply to mkp-scsi/for-next jejb-scsi/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Niklas-Cassel/ata-libata-core-Fix-null-pointer-dereference-on-error/20240627-123023
base:   linus/master
patch link:    https://lore.kernel.org/r/20240626180031.4050226-26-cassel%40kernel.org
patch subject: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids
config: i386-randconfig-141-20240628 (https://download.01.org/0day-ci/archive/20240629/202406290027.cdgsPQAF-lkp@intel.com/config)
compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406290027.cdgsPQAF-lkp@intel.com/

smatch warnings:
drivers/ata/libata-core.c:5467 ata_port_alloc() warn: unsigned 'ap->print_id' is never less than zero.

vim +5467 drivers/ata/libata-core.c

  5443	
  5444	/**
  5445	 *	ata_port_alloc - allocate and initialize basic ATA port resources
  5446	 *	@host: ATA host this allocated port belongs to
  5447	 *
  5448	 *	Allocate and initialize basic ATA port resources.
  5449	 *
  5450	 *	RETURNS:
  5451	 *	Allocate ATA port on success, NULL on failure.
  5452	 *
  5453	 *	LOCKING:
  5454	 *	Inherited from calling layer (may sleep).
  5455	 */
  5456	struct ata_port *ata_port_alloc(struct ata_host *host)
  5457	{
  5458		struct ata_port *ap;
  5459	
  5460		ap = kzalloc(sizeof(*ap), GFP_KERNEL);
  5461		if (!ap)
  5462			return NULL;
  5463	
  5464		ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN;
  5465		ap->lock = &host->lock;
  5466		ap->print_id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL);
> 5467		if (ap->print_id < 0) {
  5468			kfree(ap);
  5469			return NULL;
  5470		}
  5471		ap->host = host;
  5472		ap->dev = host->dev;
  5473	
  5474		mutex_init(&ap->scsi_scan_mutex);
  5475		INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug);
  5476		INIT_DELAYED_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan);
  5477		INIT_LIST_HEAD(&ap->eh_done_q);
  5478		init_waitqueue_head(&ap->eh_wait_q);
  5479		init_completion(&ap->park_req_pending);
  5480		timer_setup(&ap->fastdrain_timer, ata_eh_fastdrain_timerfn,
  5481			    TIMER_DEFERRABLE);
  5482	
  5483		ap->cbl = ATA_CBL_NONE;
  5484	
  5485		ata_link_init(ap, &ap->link, 0);
  5486	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids

[Niklas Cassel]

On Sat, Jun 29, 2024 at 12:31:57AM +0800, kernel test robot wrote:
> Hi Niklas,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.10-rc5 next-20240627]
> [cannot apply to mkp-scsi/for-next jejb-scsi/for-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Niklas-Cassel/ata-libata-core-Fix-null-pointer-dereference-on-error/20240627-123023
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20240626180031.4050226-26-cassel%40kernel.org
> patch subject: [PATCH v2 11/13] ata: libata-core: Reuse available ata_port print_ids
> config: i386-randconfig-141-20240628 (https://download.01.org/0day-ci/archive/20240629/202406290027.cdgsPQAF-lkp@intel.com/config)
> compiler: gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202406290027.cdgsPQAF-lkp@intel.com/
> 
> smatch warnings:
> drivers/ata/libata-core.c:5467 ata_port_alloc() warn: unsigned 'ap->print_id' is never less than zero.
> 
> vim +5467 drivers/ata/libata-core.c
> 
>   5443	
>   5444	/**
>   5445	 *	ata_port_alloc - allocate and initialize basic ATA port resources
>   5446	 *	@host: ATA host this allocated port belongs to
>   5447	 *
>   5448	 *	Allocate and initialize basic ATA port resources.
>   5449	 *
>   5450	 *	RETURNS:
>   5451	 *	Allocate ATA port on success, NULL on failure.
>   5452	 *
>   5453	 *	LOCKING:
>   5454	 *	Inherited from calling layer (may sleep).
>   5455	 */
>   5456	struct ata_port *ata_port_alloc(struct ata_host *host)
>   5457	{
>   5458		struct ata_port *ap;
>   5459	
>   5460		ap = kzalloc(sizeof(*ap), GFP_KERNEL);
>   5461		if (!ap)
>   5462			return NULL;
>   5463	
>   5464		ap->pflags |= ATA_PFLAG_INITIALIZING | ATA_PFLAG_FROZEN;
>   5465		ap->lock = &host->lock;
>   5466		ap->print_id = ida_alloc_min(&ata_ida, 1, GFP_KERNEL);
> > 5467		if (ap->print_id < 0) {

Well, the check is correct, but since ap->print_id is unsigned int,
we will need to use a temporary (signed) variable to check for errors
from ida_alloc_min(). Will fix in next revision.


>   5468			kfree(ap);
>   5469			return NULL;
>   5470		}
>   5471		ap->host = host;
>   5472		ap->dev = host->dev;
>   5473	
>   5474		mutex_init(&ap->scsi_scan_mutex);
>   5475		INIT_DELAYED_WORK(&ap->hotplug_task, ata_scsi_hotplug);
>   5476		INIT_DELAYED_WORK(&ap->scsi_rescan_task, ata_scsi_dev_rescan);
>   5477		INIT_LIST_HEAD(&ap->eh_done_q);
>   5478		init_waitqueue_head(&ap->eh_wait_q);
>   5479		init_completion(&ap->park_req_pending);
>   5480		timer_setup(&ap->fastdrain_timer, ata_eh_fastdrain_timerfn,
>   5481			    TIMER_DEFERRABLE);
>   5482	
>   5483		ap->cbl = ATA_CBL_NONE;
>   5484	
>   5485		ata_link_init(ap, &ap->link, 0);
>   5486	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

[PATCH v2 12/13] 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.

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 846ab99e0cd3..11ecfdea5959 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5492,6 +5492,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 38ce13b55474..e930ac948eac 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -82,7 +82,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 e8987dce585f..eecdd1525a18 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 v2 12/13] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Damien Le Moal]

On 6/27/24 03:00, 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.

Same comment as for patch 4: not a fan.

But I do like the fact that the port additional initialization is moved to
libsas, as that code is completely dependent on libsas.

What about this cleanup, which would make more sense:

1) Keep the ata_sas_xxx() exported symbols, even if they are trivial.
2) Move all these wrappers to a new file (libata-sas.c) and make this file
compilation dependend on CONFIG_SATA_HOST and CONFIG_SCSI_SAS_LIBSAS.

That has the benefit of keeping all the libsas wrappers together and to reduce
the binary size for configs that do not enable libsas.

Thoughts ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 12/13] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Niklas Cassel]

On Thu, Jun 27, 2024 at 10:46:11AM +0900, Damien Le Moal wrote:
> On 6/27/24 03:00, 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.
> 
> Same comment as for patch 4: not a fan.
> 
> But I do like the fact that the port additional initialization is moved to
> libsas, as that code is completely dependent on libsas.
> 
> What about this cleanup, which would make more sense:
> 
> 1) Keep the ata_sas_xxx() exported symbols, even if they are trivial.
> 2) Move all these wrappers to a new file (libata-sas.c) and make this file
> compilation dependend on CONFIG_SATA_HOST and CONFIG_SCSI_SAS_LIBSAS.
> 
> That has the benefit of keeping all the libsas wrappers together and to reduce
> the binary size for configs that do not enable libsas.
> 
> Thoughts ?

I think that:

1) These wrappers are like a virus... they are completely useless and
   having them will force us to keep adding new wrappers, e.g. we would
   need to add a new wrapper for ata_port_free().

2) Having a wrapper that simply does an EXPORT_SYMBOL is not only useless,
it also makes it harder to know that the function (called by the wrapper)
is actually non-internal, since the function will be defined in the libata
internal header in drivers/ata/libata.h, so you might think that it is an
internal function... but it isn't, since there is a wrapper exporting it :)

3) The naming prefix argument does not hold up.
   If you do a:

$ git grep -E "\s+ata_\S+\(" v6.10-rc5 drivers/scsi/libsas/
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_qc_complete(qc);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_tf_to_fis(&qc->tf, qc->dev->link->pmp, 1, (u8 *)&task->ata_task.fis);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        task->ata_task.dma_xfer = ata_is_dma(qc->tf.protocol);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_tf_from_fis(dev->sata_dev.fis, &qc->result_tf);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_tf_from_fis(dev->frame_rcvd, &tf);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        return ata_dev_classify(&tf);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ret = ata_wait_after_reset(link, deadline, check_ready);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_std_sched_eh(ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_host_init(ata_host, ha->dev, &sas_sata_ops);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_sas_tport_add(ata_host->dev, ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_host_put(ata_host);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                ata_port_probe(dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                ata_sas_port_suspend(sata->ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                ata_sas_port_resume(sata->ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_scsi_port_error_handler(ha->shost, ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                        ata_scsi_cmd_error_handler(shost, ap, &sata_q);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                         * action will be ata_port_error_handler()
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_port_schedule_eh(ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_port_wait_eh(ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_link_abort(link);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_ncq_prio_supported(ddev->sata_dev.ap, sdev, &supported);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_ncq_prio_enabled(ddev->sata_dev.ap, sdev, &enabled);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_ncq_prio_enable(ddev->sata_dev.ap, sdev, enable);
v6.10-rc5:drivers/scsi/libsas/sas_discover.c:           ata_sas_tport_delete(dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_discover.c:           ata_host_put(dev->sata_dev.ata_host);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          return ata_sas_scsi_ioctl(dev->sata_dev.ap, sdev, cmd, arg);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          ata_sas_device_configure(scsi_dev, lim, dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          return ata_change_queue_depth(dev->sata_dev.ap, sdev, depth);

You will see that far from all libata functions have ata_sas_*() wrapper,
so all the ata_* functions that do not not have ata_sas_*() wrapper are
already exported using EXPORT_SYMBOL_GPL(), i.e.:

ata_qc_complete(), ata_tf_to_fis(), ata_tf_from_fis(), ata_dev_classify(),
ata_wait_after_reset(), ata_std_sched_eh(), ata_host_init(), ata_host_put(),
ata_port_probe(), ata_scsi_port_error_handler(), ata_scsi_cmd_error_handler(),
ata_port_schedule_eh(), ata_link_abort(), ata_ncq_prio_supported(),
ata_ncq_prio_enabled(), ata_ncq_prio_enable(), ata_change_queue_depth()
are already exported using EXPORT_SYMBOL_GPL().

(And yes, some of these exported functions not used by any libata SATA driver
(compiled as a separate .ko) other than libsas.)


TL;DR: I really think that we should kill these wrappers.


Kind regards,
Niklas

Re: [PATCH v2 12/13] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Damien Le Moal]

On 6/27/24 6:48 PM, Niklas Cassel wrote:
> You will see that far from all libata functions have ata_sas_*() wrapper,
> so all the ata_* functions that do not not have ata_sas_*() wrapper are
> already exported using EXPORT_SYMBOL_GPL(), i.e.:

OK. I thought things where a little more consistent than that.
Let's kill the wrappers then. Keep your patch as it is !


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 12/13] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Hannes Reinecke]

On 6/26/24 20:00, 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.
> 
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v2 13/13] 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>
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 v2 13/13] ata: ahci: Add debug print for external port

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> 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>
> 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) &&

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[John Garry]

On 26/06/2024 19:00, Niklas Cassel wrote:
> Hello all,
> 
> This patch series was orginally meant to simply assign a unique id used
> for printing earlier (ap->print_id), but has since grown to also include
> cleanups related to ata_port_alloc() (since ap->print_id is now assigned
> in ata_port_alloc()).
> 

There's no real problem statement wrt print_id, telling how and why 
things are like they are, how it is a problem, and how it is improved in 
this series.

> Patch 1-3 fixes incorrect cleanups in the error paths.
> Patch 4,12 removes a useless libata wrappers only used for libsas.
> Patch 5 introduces a ata_port_free(), in order to avoid duplicated code.
> Patch 6 removes a unused function declaration in include/linux/libata.h.
> Patch 7 remove support for decreasing the number of ports, as it is never
>          used by any libata driver (including libsas and ipr).
> Patch 8 removes a superfluous assignment in ata_sas_port_alloc().
> Patch 9 removes the unnecessary local_port_no struct member in ata_port.
> Patch 10 performs the ata_port print_id assignment earlier, so that the
>           ata_port_* print functions can be used even before the ata_host
> 	 has been registered.
> Patch 11 changes the print_id assignment to use an ida_alloc(), such that
>           we will reuse IDs that are no longer in use, rather than keep
> 	 increasing the print_id forever.
> Patch 13 adds a debug print in case the port is marked as external, this
>           code runs before the ata_host has been registered, so it depends
> 	 on patch 10.

Re: [PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel]

On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote:
> On 26/06/2024 19:00, Niklas Cassel wrote:
> > Hello all,
> > 
> > This patch series was orginally meant to simply assign a unique id used
> > for printing earlier (ap->print_id), but has since grown to also include
> > cleanups related to ata_port_alloc() (since ap->print_id is now assigned
> > in ata_port_alloc()).
> > 
> 
> There's no real problem statement wrt print_id, telling how and why things
> are like they are, how it is a problem, and how it is improved in this
> series.

You are right, it is missing from the cover-letter.

It was there in v1:
https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/

"""
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.
"""

Will re-add it in v3.


Kind regards,
Niklas

Re: [PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[John Garry]

On 27/06/2024 13:32, Niklas Cassel wrote:
> On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote:
>> On 26/06/2024 19:00, Niklas Cassel wrote:
>>> Hello all,
>>>
>>> This patch series was orginally meant to simply assign a unique id used
>>> for printing earlier (ap->print_id), but has since grown to also include
>>> cleanups related to ata_port_alloc() (since ap->print_id is now assigned
>>> in ata_port_alloc()).
>>>
>>
>> There's no real problem statement wrt print_id, telling how and why things
>> are like they are, how it is a problem, and how it is improved in this
>> series.
> 
> You are right, it is missing from the cover-letter.
> 
> It was there in v1:
> https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/
> 
> """
> 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.
> """

OK, fine.

I see code which checks vs ap->print_id, like:

static void ata_force_link_limits(struct ata_link *link)
{
...
		if (fe->port != -1 && fe->port != link->ap->print_id)
			continue;


Is this all ok to deal with this print_id assignment change?

To me, it seems natural to assign a valid print_id from the alloc time, 
so I can't help but wonder it was done the current way.

Thanks,
John

Re: [PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel]

On Thu, Jun 27, 2024 at 01:54:34PM +0100, John Garry wrote:
> On 27/06/2024 13:32, Niklas Cassel wrote:
> > On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote:
> > > On 26/06/2024 19:00, Niklas Cassel wrote:
> > > > Hello all,
> > > > 
> > > > This patch series was orginally meant to simply assign a unique id used
> > > > for printing earlier (ap->print_id), but has since grown to also include
> > > > cleanups related to ata_port_alloc() (since ap->print_id is now assigned
> > > > in ata_port_alloc()).
> > > > 
> > > 
> > > There's no real problem statement wrt print_id, telling how and why things
> > > are like they are, how it is a problem, and how it is improved in this
> > > series.
> > 
> > You are right, it is missing from the cover-letter.
> > 
> > It was there in v1:
> > https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/
> > 
> > """
> > 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.
> > """
> 
> OK, fine.
> 
> I see code which checks vs ap->print_id, like:
> 
> static void ata_force_link_limits(struct ata_link *link)
> {
> ...
> 		if (fe->port != -1 && fe->port != link->ap->print_id)
> 			continue;
> 
> 
> Is this all ok to deal with this print_id assignment change?
> 
> To me, it seems natural to assign a valid print_id from the alloc time, so I
> can't help but wonder it was done the current way.

ap->print_id was assigned after calling ata_host_register(), because libata
allowed a driver that did not know how many ports it had, to initially call
ata_alloc_host() with a big number of ports, and then reduce the host->n_ports
variable once it knew the actually number of ports, before calling
ata_host_register(), which would then free the "excess" ports.

This feature has actually never been used by and driver, and I remove support
for this in this series:
https://lore.kernel.org/linux-ide/20240626180031.4050226-22-cassel@kernel.org/


However, you do raise a good point...
ap->print_id is just supposed to be used for printing, but it appears that
ata_force_link_limits() and some other ata_force_*() functions make use of
it for other things... sigh...

Hopefully I can just change them from:
	if (fe->port != -1 && fe->port != link->ap->print_id)
to
	if (fe->port != -1)

but I will need to look in to this further...

Thank you for noticing this (ab)use of print_id!


Kind regards,
Niklas

Re: [PATCH v2 00/13] ata,libsas: Assign the unique id used for printing earlier

[Niklas Cassel]

On Thu, Jun 27, 2024 at 05:07:43PM +0200, Niklas Cassel wrote:
> On Thu, Jun 27, 2024 at 01:54:34PM +0100, John Garry wrote:
> > On 27/06/2024 13:32, Niklas Cassel wrote:
> > > On Thu, Jun 27, 2024 at 01:26:04PM +0100, John Garry wrote:
> > > > On 26/06/2024 19:00, Niklas Cassel wrote:
> > > > > Hello all,
> > > > > 
> > > > > This patch series was orginally meant to simply assign a unique id used
> > > > > for printing earlier (ap->print_id), but has since grown to also include
> > > > > cleanups related to ata_port_alloc() (since ap->print_id is now assigned
> > > > > in ata_port_alloc()).
> > > > > 
> > > > 
> > > > There's no real problem statement wrt print_id, telling how and why things
> > > > are like they are, how it is a problem, and how it is improved in this
> > > > series.
> > > 
> > > You are right, it is missing from the cover-letter.
> > > 
> > > It was there in v1:
> > > https://lore.kernel.org/linux-ide/20240618153537.2687621-7-cassel@kernel.org/
> > > 
> > > """
> > > 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.
> > > """
> > 
> > OK, fine.
> > 
> > I see code which checks vs ap->print_id, like:
> > 
> > static void ata_force_link_limits(struct ata_link *link)
> > {
> > ...
> > 		if (fe->port != -1 && fe->port != link->ap->print_id)
> > 			continue;
> > 
> > 
> > Is this all ok to deal with this print_id assignment change?
> > 
> > To me, it seems natural to assign a valid print_id from the alloc time, so I
> > can't help but wonder it was done the current way.
> 
> ap->print_id was assigned after calling ata_host_register(), because libata
> allowed a driver that did not know how many ports it had, to initially call
> ata_alloc_host() with a big number of ports, and then reduce the host->n_ports
> variable once it knew the actually number of ports, before calling
> ata_host_register(), which would then free the "excess" ports.
> 
> This feature has actually never been used by and driver, and I remove support
> for this in this series:
> https://lore.kernel.org/linux-ide/20240626180031.4050226-22-cassel@kernel.org/
> 
> 
> However, you do raise a good point...
> ap->print_id is just supposed to be used for printing, but it appears that
> ata_force_link_limits() and some other ata_force_*() functions make use of
> it for other things... sigh...
> 
> Hopefully I can just change them from:
> 	if (fe->port != -1 && fe->port != link->ap->print_id)
> to
> 	if (fe->port != -1)
> 
> but I will need to look in to this further...

So, looking more closely at this, the code is actually not abusing print_id.

Looking at libata.force in Documentation/admin-guide/kernel-parameters.txt:

[LIBATA] Force configurations.  The format is a comma-
                        separated list of "[ID:]VAL" where ID is PORT[.DEVICE].
                        PORT and DEVICE are decimal numbers matching port, link
                        or device.  Basically, it matches the ATA ID string
                        printed on console by libata.


While this seems a bit fragile, since it relies on the probe ordering
of the SATA controller drivers, which could change, it does still work
as designed after this series:

I added the following to my kernel command line:
"libata.force=5:nolpm"

which yielded:
[    1.811464] ata3.00: FORCE: horkage modified (nolpm)
[    1.811466] ata3.00: LPM support broken, forcing max_power
[    1.811468] ata3.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100
[    1.811470] ata3.00: 2097152 sectors, multi 16: LBA48 NCQ (depth 32)
[    1.811474] ata3.00: applying bridge limits
[    1.811535] ata3.00: LPM support broken, forcing max_power
[    1.811537] ata3.00: configured for UDMA/100

And considering that all checks against ap->print_id is for libata.force
related parameters, I think that we are all good.


Kind regards,
Niklas