Re: Regression on linux-next (next-20250708)

[Damien Le Moal <dlemoal@kernel.org>]

On 7/25/25 15:43, Borah, Chaitanya Kumar wrote:
> Hello Damien,
> 
> Hope you are doing well. I am Chaitanya from the linux graphics team in 
> Intel.
> 
> This mail is regarding a regression we are seeing in our CI runs[1] on
> linux-next repository.
> 
> Since the version next-20250708 [2], we are seeing the following regression
> 
> `````````````````````````````````````````````````````````````````````````````````
> (kms_pm_rpm:5821) igt_pm-CRITICAL: Test assertion failure function 
> __igt_pm_enable_sata_link_power_management, file ../lib/igt_pm.c:392:
> 
> (kms_pm_rpm:5821) igt_pm-CRITICAL: Failed assertion: write(fd, 
> "min_power\n", strlen("min_power\n")) == strlen("min_power\n")
> 
> (kms_pm_rpm:5821) igt_pm-CRITICAL: Last errno: 95, Operation not supported
> 
> (kms_pm_rpm:5821) igt_pm-CRITICAL: error: -1 != 10
> 
> Test kms_pm_rpm failed.
> `````````````````````````````````````````````````````````````````````````````````
> Details log can be found in [3].
> 
> After bisecting the tree, the following patch [4] seems to be the first 
> "bad" commit
> 
> `````````````````````````````````````````````````````````````````````````````````````````````````````````
> commit 4edf1505b76d30e1e1e283d431e4f84ad01ddcef
> 
> Author: Damien Le Moal dlemoal@kernel.org
> 
> Date:   Tue Jul 1 21:53:18 2025 +0900
> 
> 
>      ata: ahci: Disallow LPM policy control for external ports
> `````````````````````````````````````````````````````````````````````````````````````````````````````````
> 
> For some context in our kms_pm_rpm tests, we enable min_power policy for 
> SATA so that we can reach deep runtime power states and restore the 
> original policy after finishing. [5][6]
> 
> IIUC, the above change is based on spec and not something which can be 
> reverted. So as I see it, we have to drop this code path for external 
> ports. However I am not sure if we can achieve deep power states without 
> enforcing it through the sysfs entry.

I am not entirely sure what you mean with the last sentence above, but for
external ports, LPM cannot be used if you want to keep the port hotplug
capability alive and working. Without keeping such port at max power state, we
cannot detect hotplug events (which is super annoying when you have e.g. a
server with front loading drive bays allowing swapping drives without shutting
the machine down).

> Atleast for the basic-rte subtest, the test passes if we comment out the 
> functions controlling the SATA ports. We will need more testing to 
> determine if this approach work. Any thoughts on it?

Niklas and I actually suspected that we would be getting "complaints" about this
change. Well... We did :)

The problem really is that external ports have never been properly handled by
libata so SATA hot-plugging never really worked reliably. Patches queued up for
6.17 before this patch prevent the kernel from changing the power state of
external port. And this patch was introduced after seeing systemd.udevd setting
external ports power state to min_power or lower states, thus breaking again the
hotplug capability.

The error you are seeing is thus entirely correct and expected.

The question is though: do we want the user to "ignore" hotplug capability and
instead priviledge low power states. I guess we should have such capability.

> Also, are there other ways to detect a port is external other than 
> receiving EOPNOTSUPP on the sysfs write?

There is not. But it would be easy to add a sysfs port attribute, e.g.
/sys/class/ata_port/ata1/external which says "0" for regular ports and "1" for
external ports. We could also make this attribute writable in the case of
external port so that doing:

echo 0 > /sys/class/ata_port/ata1/external

forces the kernel to ignore the external nature of the port and allow user
control of the port/device LPM state.

Would that work for your case ?

-- 
Damien Le Moal
Western Digital Research