[PATCH i-g-t] lib/igt_pm: Skip LPM policy control on unsupported SATA hosts

[Borah, Chaitanya Kumar]

On 7/31/2025 1:34 PM, Damien Le Moal wrote:
> On 7/31/25 4:41 PM, Chaitanya Kumar Borah wrote:
>> With [1], LPM policy control is now disabled for external SATA ports.
>> This led to a regression in our CI runs [2]. Discussions on the regression
>> resulted in patch [3] which introduces a new sysfs entry, querying
>> which we can determine if LPM policy control is allowed on a host or not.
>>
>> Store/Re-store LPM policy only when it is allowed. If the sysfs entry is
>> not present assume LPM policy control is allowed. This  maintains backward
>> compatibility.
> 
> Note: allowed, yes, but that does not mean it will actually work or do
> anything, since the port or the device may not be supporting LPN at all.
> 
>>
>> [1] https://lore.kernel.org/linux-ide/20250701125321.69496-8-dlemoal@kernel.org/
>> [2] https://lore.kernel.org/linux-ide/07563042-6576-41cd-9a95-de83cfc95de1@intel.com/
>> [3] https://lore.kernel.org/linux-ide/20250730001947.332661-1-dlemoal@kernel.org/
>>
>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah at intel.com>
> 
> Patch looks OK to me (nit below though).
> 
> Reviewed-by: Damien Le Moal <dlemoal at kernel.org>
> 
> 
>> ---
>>   lib/igt_pm.c | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
>> index cf1c442f8..1ffcdcef3 100644
>> --- a/lib/igt_pm.c
>> +++ b/lib/igt_pm.c
>> @@ -321,6 +321,35 @@ void igt_pm_enable_audio_runtime_pm(void)
>>   		igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
>>   }
>>   
>> +static bool lpm_supported_by_sata_host(int host_num)
>> +{
>> +	int fd;
>> +	ssize_t len;
>> +	char buf[2];
>> +	char file_name[PATH_MAX];
>> +
>> +	snprintf(file_name, PATH_MAX,
>> +		 "/sys/class/scsi_host/host%d/link_power_management_supported",
>> +		 host_num);
>> +
>> +	fd = open(file_name, O_RDONLY);
>> +
>> +	/* assume LPM is supported if sysfs entry is absent. This preserves default behaviour */
>> +	if (fd < 0)
>> +		return true;
>> +
>> +	len = read(fd, buf, 1);
>> +
>> +	close(fd);
> 
> Nit: this would all be a lot simpler using scanf:
> 	
> 	snprintf(file_name, PATH_MAX,
> 		 "/sys/class/scsi_host/host%d/link_power_management_supported",
> 		 host_num);
> 
> 	/*
> 	 * If sysfs entry is absent, assume that LPM is supported to
> 	 * preserve the default behaviour.
> 	 */
> 	if (scanf(file_name, "%d", &supported) != 1)
> 		return true;
> 
> 	return supported;
> 


Thank you for the review Damien! This seems like a good idea. However, I 
am considering if I should keep using the POSIX calls to keep it 
consistent with the rest of the file.

I also wonder if the original approach was chosen intentionally to avoid 
stale data from sysfs — though in this specific case, it may not make 
much of a difference.

Regards

Chaitanya


>> +
>> +	if (len <= 0)
>> +		return true;
>> +
>> +	buf[len] = '\0';
>> +
>> +	return buf[0] == '1';
>> +}
>> +
>>   static void __igt_pm_enable_sata_link_power_management(void)
>>   {
>>   	int fd, i;
>> @@ -372,6 +401,9 @@ static void __igt_pm_enable_sata_link_power_management(void)
>>   	igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
>>   
>>   	for (i = 0; i < __scsi_host_cnt; i++) {
>> +		if (!lpm_supported_by_sata_host(i))
>> +			continue;
>> +
>>   		snprintf(file_name, PATH_MAX,
>>   			 "/sys/class/scsi_host/host%d/link_power_management_policy",
>>   			 i);
>> @@ -412,6 +444,9 @@ static void __igt_pm_restore_sata_link_power_management(void)
>>   	for (i = 0; i < __scsi_host_cnt; i++) {
>>   		int8_t policy;
>>   
>> +		if (!lpm_supported_by_sata_host(i))
>> +			continue;
>> +
>>   		if (__sata_pm_policies[i] == POLICY_UNKNOWN)
>>   			continue;
>>   		else
> 
>