[PATCH] ahci: print the number of implemented ports

[PATCH] ahci: print the number of implemented ports

[Niklas Cassel]

We are currently printing the CAP.NP field.
CAP.NP is a 0's based value indicating the maximum number of ports
supported by the HBA silicon. Note that the number of ports indicated
in this field may be more than the number of ports indicated in the
PI (ports implemented) register. (See AHCI 1.3.1, section 3.1.1 -
Offset 00h: CAP – HBA Capabilities.)

Print the port_map instead, which is the value read by the PI (ports
implemented) register (after fixups).

PI (ports implemented) register is a field that has a bit set to '1'
if that specific port is implemented. This register is allowed to have
zeroes mixed with ones, i.e. a port in the middle is allowed to be
unimplemented. (See AHCI 1.3.1, section 3.1.4 - Offset 0Ch: PI – Ports
Implemented.)

Fix the libata print to only print the number of implemented ports,
instead of the theoretical number of ports supported by the HBA.

Suggested-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.h    | 11 +++++++++++
 drivers/ata/libahci.c |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index df8f8a1a3a34..92d29a059763 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -455,4 +455,15 @@ static inline int ahci_nr_ports(u32 cap)
 	return (cap & 0x1f) + 1;
 }
 
+static inline int ahci_nr_ports_in_map(u32 map)
+{
+	unsigned long port_map = map;
+	int i, n = 0;
+
+	for_each_set_bit(i, &port_map, AHCI_MAX_PORTS)
+		n++;
+
+	return n;
+}
+
 #endif /* _AHCI_H */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 1a63200ea437..82ebe911a327 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2637,7 +2637,7 @@ void ahci_print_info(struct ata_host *host, const char *scc_s)
 		vers & 0xff,
 
 		((cap >> 8) & 0x1f) + 1,
-		(cap & 0x1f) + 1,
+		ahci_nr_ports_in_map(impl),
 		speed_s,
 		impl,
 		scc_s);
-- 
2.43.0

Re: [PATCH] ahci: print the number of implemented ports

[Sergei Shtylyov]

On 2/14/24 9:20 PM, Niklas Cassel wrote:

> We are currently printing the CAP.NP field.
> CAP.NP is a 0's based value indicating the maximum number of ports
> supported by the HBA silicon. Note that the number of ports indicated
> in this field may be more than the number of ports indicated in the
> PI (ports implemented) register. (See AHCI 1.3.1, section 3.1.1 -
> Offset 00h: CAP – HBA Capabilities.)
> 
> Print the port_map instead, which is the value read by the PI (ports
> implemented) register (after fixups).
> 
> PI (ports implemented) register is a field that has a bit set to '1'
> if that specific port is implemented. This register is allowed to have
> zeroes mixed with ones, i.e. a port in the middle is allowed to be
> unimplemented. (See AHCI 1.3.1, section 3.1.4 - Offset 0Ch: PI – Ports
> Implemented.)
> 
> Fix the libata print to only print the number of implemented ports,
> instead of the theoretical number of ports supported by the HBA.
> 
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.h    | 11 +++++++++++
>  drivers/ata/libahci.c |  2 +-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index df8f8a1a3a34..92d29a059763 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -455,4 +455,15 @@ static inline int ahci_nr_ports(u32 cap)
>  	return (cap & 0x1f) + 1;
>  }
>  
> +static inline int ahci_nr_ports_in_map(u32 map)
> +{
> +	unsigned long port_map = map;

   Why cast to potentially 64-bit type?

> +	int i, n = 0;
> +
> +	for_each_set_bit(i, &port_map, AHCI_MAX_PORTS)
> +		n++;

   There's hweight32() for that, IIUC.

> +
> +	return n;
> +}
> +
>  #endif /* _AHCI_H */
[...]

MBR, Sergey

Re: [PATCH] ahci: print the number of implemented ports

[Sergei Shtylyov]

On 2/14/24 10:33 PM, Sergei Shtylyov wrote:
[...]

>> We are currently printing the CAP.NP field.
>> CAP.NP is a 0's based value indicating the maximum number of ports
>> supported by the HBA silicon. Note that the number of ports indicated
>> in this field may be more than the number of ports indicated in the
>> PI (ports implemented) register. (See AHCI 1.3.1, section 3.1.1 -
>> Offset 00h: CAP – HBA Capabilities.)
>>
>> Print the port_map instead, which is the value read by the PI (ports
>> implemented) register (after fixups).
>>
>> PI (ports implemented) register is a field that has a bit set to '1'
>> if that specific port is implemented. This register is allowed to have
>> zeroes mixed with ones, i.e. a port in the middle is allowed to be
>> unimplemented. (See AHCI 1.3.1, section 3.1.4 - Offset 0Ch: PI – Ports
>> Implemented.)
>>
>> Fix the libata print to only print the number of implemented ports,
>> instead of the theoretical number of ports supported by the HBA.
>>
>> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>> ---
>>  drivers/ata/ahci.h    | 11 +++++++++++
>>  drivers/ata/libahci.c |  2 +-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>> index df8f8a1a3a34..92d29a059763 100644
>> --- a/drivers/ata/ahci.h
>> +++ b/drivers/ata/ahci.h
>> @@ -455,4 +455,15 @@ static inline int ahci_nr_ports(u32 cap)
>>  	return (cap & 0x1f) + 1;
>>  }
>>  
>> +static inline int ahci_nr_ports_in_map(u32 map)
>> +{
>> +	unsigned long port_map = map;
> 
>    Why cast to potentially 64-bit type?

  Ah, I figured it's for find_next_bit()....

> 
>> +	int i, n = 0;
>> +
>> +	for_each_set_bit(i, &port_map, AHCI_MAX_PORTS)
>> +		n++;
> 
>    There's hweight32() for that, IIUC.

   Yeah, it replaces this whole function... :-)

[...]

MBR, Sergey

Re: [PATCH] ahci: print the number of implemented ports

[Andrey Melnikov]

ср, 14 февр. 2024 г. в 21:20, Niklas Cassel <cassel@kernel.org>:
>
> We are currently printing the CAP.NP field.
> CAP.NP is a 0's based value indicating the maximum number of ports
> supported by the HBA silicon. Note that the number of ports indicated
> in this field may be more than the number of ports indicated in the
> PI (ports implemented) register. (See AHCI 1.3.1, section 3.1.1 -
> Offset 00h: CAP – HBA Capabilities.)
>
> Print the port_map instead, which is the value read by the PI (ports
> implemented) register (after fixups).
>
> PI (ports implemented) register is a field that has a bit set to '1'
> if that specific port is implemented. This register is allowed to have
> zeroes mixed with ones, i.e. a port in the middle is allowed to be
> unimplemented. (See AHCI 1.3.1, section 3.1.4 - Offset 0Ch: PI – Ports
> Implemented.)
>
> Fix the libata print to only print the number of implemented ports,
> instead of the theoretical number of ports supported by the HBA.

NAK.
Kernel must report what it got from silicone/addon-board. Different
revisions may implement different numbers of ports.
If you want to report real (usable) ports - add it after the mask.

> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.h    | 11 +++++++++++
>  drivers/ata/libahci.c |  2 +-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index df8f8a1a3a34..92d29a059763 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -455,4 +455,15 @@ static inline int ahci_nr_ports(u32 cap)
>         return (cap & 0x1f) + 1;
>  }
>
> +static inline int ahci_nr_ports_in_map(u32 map)
> +{
> +       unsigned long port_map = map;
> +       int i, n = 0;
> +
> +       for_each_set_bit(i, &port_map, AHCI_MAX_PORTS)
> +               n++;
> +
> +       return n;
> +}
> +
>  #endif /* _AHCI_H */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1a63200ea437..82ebe911a327 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2637,7 +2637,7 @@ void ahci_print_info(struct ata_host *host, const char *scc_s)
>                 vers & 0xff,
>
>                 ((cap >> 8) & 0x1f) + 1,
> -               (cap & 0x1f) + 1,
> +               ahci_nr_ports_in_map(impl),
>                 speed_s,
>                 impl,
>                 scc_s);
> --
> 2.43.0
>

Re: [PATCH] ahci: print the number of implemented ports

[Damien Le Moal]

On 2/15/24 17:01, Andrey Melnikov wrote:
> ср, 14 февр. 2024 г. в 21:20, Niklas Cassel <cassel@kernel.org>:
>>
>> We are currently printing the CAP.NP field.
>> CAP.NP is a 0's based value indicating the maximum number of ports
>> supported by the HBA silicon. Note that the number of ports indicated
>> in this field may be more than the number of ports indicated in the
>> PI (ports implemented) register. (See AHCI 1.3.1, section 3.1.1 -
>> Offset 00h: CAP – HBA Capabilities.)
>>
>> Print the port_map instead, which is the value read by the PI (ports
>> implemented) register (after fixups).
>>
>> PI (ports implemented) register is a field that has a bit set to '1'
>> if that specific port is implemented. This register is allowed to have
>> zeroes mixed with ones, i.e. a port in the middle is allowed to be
>> unimplemented. (See AHCI 1.3.1, section 3.1.4 - Offset 0Ch: PI – Ports
>> Implemented.)
>>
>> Fix the libata print to only print the number of implemented ports,
>> instead of the theoretical number of ports supported by the HBA.
> 
> NAK.
> Kernel must report what it got from silicone/addon-board. Different
> revisions may implement different numbers of ports.
> If you want to report real (usable) ports - add it after the mask.

Or print the mask *and* the number of ports.

E.g., on my machine, I see:

ahci 0000:00:17.0: AHCI 0001.0301 32 slots 8 ports 6 Gbps 0xff impl SATA mode

Which would be a lot more human friendly as something like:

ahci 0000:00:17.0: AHCI 0001.0301 32 slots 8/8 ports impl (0xff mask) 6 Gbps
SATA mode

Or

ahci 0000:00:17.0: AHCI 0001.0301 32 slots 6 Gbps SATA mode
ahci 0000:00:17.0: AHCI 0001.0301 8/8 ports implemented (port mask 0xff)


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ahci: print the number of implemented ports

[Niklas Cassel]

Hello Andrey,

On Thu, Feb 15, 2024 at 11:01:23AM +0300, Andrey Melnikov wrote:
> ср, 14 февр. 2024 г. в 21:20, Niklas Cassel <cassel@kernel.org>:
> >
> > We are currently printing the CAP.NP field.
> > CAP.NP is a 0's based value indicating the maximum number of ports
> > supported by the HBA silicon. Note that the number of ports indicated
> > in this field may be more than the number of ports indicated in the
> > PI (ports implemented) register. (See AHCI 1.3.1, section 3.1.1 -
> > Offset 00h: CAP – HBA Capabilities.)
> >
> > Print the port_map instead, which is the value read by the PI (ports
> > implemented) register (after fixups).
> >
> > PI (ports implemented) register is a field that has a bit set to '1'
> > if that specific port is implemented. This register is allowed to have
> > zeroes mixed with ones, i.e. a port in the middle is allowed to be
> > unimplemented. (See AHCI 1.3.1, section 3.1.4 - Offset 0Ch: PI – Ports
> > Implemented.)
> >
> > Fix the libata print to only print the number of implemented ports,
> > instead of the theoretical number of ports supported by the HBA.
> 
> NAK.
> Kernel must report what it got from silicone/addon-board. Different
> revisions may implement different numbers of ports.

Strong words here... "NAK", "must"...
Could you please give some more constructive criticism?


This is only a print at boot, so it is simply meant to provide information
to the user.

Currently this print already contains the mask, printed as impl 0x%x.
This value is after fixups, so this value does not necessarily report
"what the silcone reports".


From AHCI 1.3.1 - 3.1.4 Offset 0Ch: PI – Ports Implemented:
"""
This register indicates which ports are exposed by the HBA.
It is loaded by the BIOS. It indicates which ports that the HBA supports
are available for software to use. For example, on an HBA that supports
6 ports as indicated in CAP.NP, only ports 1 and 3 could be available,
with ports 0, 2, 4, and 5 being unavailable.

Software must not read or write to registers within unavailable ports.
"""


So
1) the impl value (the port mask) is the value read by PI register,
and additionally after fixups.
2) Software must not read or write ports that are unimplemented.


This is with current mainline (without my patch) on Intel Tiger Lake:
[    0.379525] ahci 0000:00:17.0: AHCI 0001.0301 32 slots 3 ports 6 Gbps 0x31 impl SATA mode
[    0.379531] ahci 0000:00:17.0: flags: 64bit ncq sntf stag pm clo only pio slum part ems sxs deso sadm sds apst
[    0.399005] ata1: SATA max UDMA/133 abar m2048@0x42233000 port 0x42233100 irq 125 lpm-pol 0
[    0.399009] ata2: DUMMY
[    0.399010] ata3: DUMMY
[    0.399011] ata4: DUMMY
[    0.399012] ata5: SATA max UDMA/133 abar m2048@0x42233000 port 0x42233300 irq 125 lpm-pol 0
[    0.399015] ata6: SATA max UDMA/133 abar m2048@0x42233000 port 0x42233380 irq 125 lpm-pol 0

Here, CAP.NP reports that the maximum supported number of ports is 3.

So for this specific platform CAP.NP (maximum) is set to the same as
the number of implemented ports.

But as we can see for the description of the CAP.NP field:
"Note that the number of ports indicated in this field may be more than
the number of ports indicated in the PI register."

So the case that "maximum number of ports == number implemented" may not be
the case for all HBAs.

Another HBA could have CAP.NP set at 6.
But PI reports that mask 0x31.

Why should this platform not print number of ports as 3?
In other words, what value does knowing the maximum number of ports
theoretically supported by the HBA provide the average user?

I would expect that the average user would rather see the number of implemented
ports, because that is the number of ports that Linux will be able to use.
The device driver is not allowed to touch unimplemented ports, so why should
the print by the device driver include these?

If you want to see that, I think you can use tools to dump the HBA regs.

What caused me to write this patch in the first place was that someone was
surprised to see:

> before:
> ahci 0000:04:00.0: AHCI 0001.0301 32 slots 24 ports 6 Gbps 0xffff0f impl SATA mode
>
> after:
> ahci 0000:04:00.0: forcing port_map 0xffff0f -> 0xf
> ahci 0000:04:00.0: AHCI 0001.0301 32 slots 24 ports 6 Gbps 0xf impl SATA mode

And I have to agree. Why report 24 ports?
We had a fixup that corrected the incorrect PI register to 0xf.
The print is there to provide information, but reporting that we have 24 ports
(instead of reporting 4 ports), causes more confusion to the user, rather than
providing userful information IMO.


Kind regards,
Niklas