Re: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

On Tue, Jul 28, 2026 at 11:53:36AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] ata_scsi_lpm_supported() skips checking devices on the slave link because it uses the ATA_LITER_EDGE iterator. This allows userspace to enable Link Power Management (LPM) on a port where the slave device does not support it (has ATA_QUIRK_NOLPM).
> --
> 
> --- Patch [1]: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration ---
> commit 32bd728de2e1b6025504205f3ade16813231e703
> Author: Niklas Cassel <[email protected]>
> 
> ata: libata-sata: fix ata_scsi_lpm_supported() iteration
> 
> This commit fixes the inner loop of ata_scsi_lpm_supported() to iterate
> over the specific link being evaluated rather than always using the host
> link. This prevents the LPM policy from being incorrectly applied or denied.
> 
> Link: https://lore.kernel.org/linux-ide/[email protected]/
> 
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index b0706c30da052..ad40f516c6d43 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -913,7 +913,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
> >  		return false;
> >  
> >  	ata_for_each_link(link, ap, EDGE) {
> > -		ata_for_each_dev(dev, &ap->link, ENABLED) {
> > +		ata_for_each_dev(dev, link, ENABLED) {
> 
> [Severity: High]
> This is a pre-existing issue, but does using the ATA_LITER_EDGE iterator
> here in ata_scsi_lpm_supported() skip checking devices on the slave link?
> 
> Since the EDGE iterator skips ap->slave_link, it appears this loop misses
> devices on the slave link that might have the ATA_QUIRK_NOLPM quirk.

Well,

1) slave_link is only for PATA.

Yes, I know that two sata drivers abuse this:
drivers/ata/sata_sis.c:                 rc = ata_slave_link_init(ap);
drivers/ata/sata_via.c:         ata_slave_link_init(host->ports[i]);


2) The call to ata_eh_link_set_lpm() is protected by:

libata-eh.c:ata_eh_recover()

if (link->lpm_policy != ap->target_lpm_policy) {
	rc = ata_eh_link_set_lpm(link, ap->target_lpm_policy, &dev);
}

And the only driver that sets ap->target_lpm_policy is AHCI.

So, I don't think we need to care about the slave link in this case.


Kind regards,
Niklas