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

[Niklas Cassel <cassel@kernel.org>]

On Tue, Jul 28, 2026 at 02:16:30PM +0200, Niklas Cassel wrote:
> On Tue, Jul 28, 2026 at 11:53:36AM +0000, sashiko-bot@kernel.org 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 <cassel@kernel.org>
> > 
> > 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/20260728112200.B99F21F000E9@smtp.kernel.org/
> > 
> > > 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.

I guess theoretically, if someone uses a:

sata_sis or sata_via, and forces LPM enabled via sysfs, even though it is
not supported/initialized by any driver other than AHCI... I guess the
drive might become unresponsive.

But, looking at these drivers, they don't provide a .set_lpm() callback,
so I don't see how that could actually happen.


There is another driver however, that does have a slave_link:
drivers/ata/ata_piix.c:                 rc = ata_slave_link_init(ap);

And which does have a .set_lpm() callback:
drivers/ata/ata_piix.c: .set_lpm                = piix_sidpr_set_lpm,


So I guess perhaps this problem could happen on ata_piix - which seems to
be a driver for Intel PATA/SATA controllers.

From a quick look, it seems like
ata_piix.c calls ata_slave_link_init() only if:

if (ap->flags & ATA_FLAG_SLAVE_POSS) {
	rc = ata_slave_link_init(ap);
}



And:
PIIX_PATA_FLAGS         = ATA_FLAG_SLAVE_POSS,
PIIX_SATA_FLAGS         = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,


And all board definitions using PIIX_PATA_FLAGS seems to have pata_* in
their name, so I still think we can ignore this Sashiko comment.


Kind regards,
Niklas