Re: [PATCH v2 3/6] ata: libata-sata: Improve link_power_management_supported sysfs attribute

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

On 1/12/26 13:20, Niklas Cassel wrote:
> The link_power_management_supported sysfs attribute is currently set as
> true even for ata ports that lack a .set_lpm() callback, e.g. dummy ports.
> 
> This is a bit silly, because while writing to the
> link_power_management_policy sysfs attribute will make ata_scsi_lpm_store()
> update ap->target_lpm_policy (thus sysfs will reflect the new value) and
> call ata_port_schedule_eh() for the port, it is essentially a no-op.
> 
> This is because for a port without a .set_lpm() callback, once EH gets to
> run, the ata_eh_link_set_lpm() will simply return, since the port does not
> provide a .set_lpm() callback.
> 
> Thus, make sure that the link_power_management_supported sysfs attribute
> is set to false for ports that lack a .set_lpm() callback. This way the
> link_power_management_policy sysfs attribute will no longer be writable,
> so we will no longer be misleading users to think that their sysfs write
> actually does something.
> 
> Fixes: 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-sata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b2817a2995d6..04e1e774645e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -909,7 +909,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
>  	struct ata_link *link;
>  	struct ata_device *dev;
>  
> -	if (ap->flags & ATA_FLAG_NO_LPM)
> +	if ((ap->flags & ATA_FLAG_NO_LPM) || !ap->ops->set_lpm)

Can't we set ATA_FLAG_NO_LPM for ports that do not have set_lpm implemented
earlier when scanning ? That would be safer.


>  		return false;
>  
>  	ata_for_each_link(link, ap, EDGE) {


-- 
Damien Le Moal
Western Digital Research