Re: [PATCH v2] ata: ahci: use hweight_long() to count port_map bits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Signed integer overflow and undefined behavior in bit shift operations.
- [High] Strict validation zeroes out `port_map` for non-compliant devices without fallback for AHCI 1.3+, completely disabling the controller.
--

commit 3665ac64e718a0f079a1778f11d8d96122c49025
Author: TanZheng <tanzheng@kylinos.cn>

ata: ahci: use hweight_long() to count port_map bits

This commit simplifies the calculation of set bits in the port mapping
by replacing an open loop with the hweight_long() function.

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index c79abdfcd7a9b..15d44266c9df8 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -552,11 +552,7 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
>  
>  	/* cross check port_map and cap.n_ports */
>  	if (port_map) {
> -		int map_ports = 0;
> -
> -		for (i = 0; i < AHCI_MAX_PORTS; i++)
> -			if (port_map & (1 << i))
> -				map_ports++;
> +		int map_ports = hweight_long(port_map);
>  
>  		/* If PI has more ports than n_ports, whine, clear
>  		 * port_map and let it be generated from n_ports.

[Severity: High]
This is a pre-existing issue, but zeroing out the port map based on this
check can permanently disable the controller for newer devices.

If a device reports extra unimplemented ports in its port map, the code
zeroes out port_map. The inline comment indicates it will let it be generated
from n_ports. However, the subsequent fallback logic that regenerates
port_map is conditionally guarded for older versions:

ahci_save_initial_config() {
    ...
    if (!port_map && vers < 0x10300) {
        port_map = (1 << ahci_nr_ports(cap)) - 1;
    ...
}

For AHCI 1.3+ devices, port_map is never regenerated and remains 0.

Does this cause all legitimate ports to be assigned ata_dummy_port_ops,
breaking otherwise functional devices?

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing
undefined behavior regression with the port mask generation nearby.

When regenerating the port map for devices prior to AHCI 1.3, the fallback
logic uses a signed 32-bit integer:

ahci_save_initial_config() {
    ...
    if (!port_map && vers < 0x10300) {
        port_map = (1 << ahci_nr_ports(cap)) - 1;
    ...
}

Since ahci_nr_ports(cap) can return up to 32, shifting a 32-bit integer by 32
is undefined behavior. On architectures like x86, the shift count is masked
to 5 bits, so 1 << 32 evaluates to 1 << 0 = 1.

This results in 1 - 1 = 0, causing port_map to be incorrectly cleared to 0.

Could this unintentionally disable all ports on a fully populated 32-port
AHCI controller? Is it possible to use 1UL instead of 1 to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528060050.743599-1-kensanya@163.com?part=1