Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

On 7/25/2025 4:03 PM, Damien Le Moal wrote:
> 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 at 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.
> 

Atleast a case can be made for debugging and testing use-cases.

>> 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 ?
> 

Something like this should solve our problem.

Regards

Chaitanya