Re: [PATCH v2 2/2] ata: Use ACPI methods to power on ata ports

[Markus Probst <markus.probst@posteo.de>]

On Thu, 2025-10-09 at 13:59 +0200, Niklas Cassel wrote:
> On Thu, Oct 09, 2025 at 11:24:49AM +0000, Markus Probst wrote:
> > Some embedded devices, including many Synology NAS devices, have
> > the
> > ability to control whether a ATA port has power or not.
> 
> In V1, you mentioned that it was to control the SATA power supply,
> now you mention the ATA port. I am confused.
The power supply associated with the ata port (or to be more precise,
it controls the power gate to the SATA Power connector for the specific
disk, at least in the synology example). It sets the power state
defined in ACPI on the ata port. I might need to update the wording
used in the patches to make it more clear.

> 
> If it is for the ATA port, then SATA already has support for this,
> using PxSCTL.
> 
> How does this ACPI way to control power interact with the regular
> way to control power for a port using PxSCTL?
> 
> 
> > 
> > Add a new function, ata_acpi_dev_manage_restart(), that will be
> > used to
> > determine if a disk should be stopped before restarting the system.
> > If a
> > usable ACPI power resource has been found, it is assumed that the
> > disk
> > will lose power after a restart and should be stopped to avoid a
> > power
> > failure. Also add a new function, ata_acpi_port_set_power_state(),
> > that
> > will be used to power on an ata port if usable ACPI power resources
> > are
> > found. It will be called right before probing the port, therefore
> > the port
> > will be powered on just in time.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  drivers/ata/libata-acpi.c | 70
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/ata/libata-core.c |  2 ++
> >  drivers/ata/libata-scsi.c |  1 +
> >  drivers/ata/libata.h      |  4 +++
> >  4 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index f2140fc06ba0..bba5ef49f055 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -245,6 +245,76 @@ void ata_acpi_bind_dev(struct ata_device *dev)
> >  				   ata_acpi_dev_uevent);
> >  }
> >  
> > +/**
> > + * ata_acpi_dev_manage_restart - if the disk should be stopped
> > (spin down) on
> > + * system restart.
> > + * @dev: target ATA device
> > + *
> > + * RETURNS:
> > + * true if the disk should be stopped, otherwise false
> > + */
> > +bool ata_acpi_dev_manage_restart(struct ata_device *dev)
> > +{
> > +	// If the device is power manageable and we assume the
> > disk loses power
> > +	// on reboot.
> 
> Like Damien mentioned earlier, please no C++ style comments.
> 
> 
> Kind regards,
> Niklas

Also for the questions sent to the v1 patch:

> Since this patch implements something similar to DevSleep, but
> rather,
> IIUC, for the SATA power itself?

Yes


> How is SATA power supplied tied to a port in ACPI? If you have a
desktop
> you have a PSU, and don't really know which supply is for which port.

It is not for desktop computers, but for embedded devices.


> So, considering how many ways we already have to disable/power off a
port,
> you might understand why I think it is extra important that you
document
> exactly how, and why we need yet another way to disable/power on/off
a port.

In this case,
- According to ACPI spec, if a device has a power resource defined it
has to be turned on before we are able send commands to the device
- Because there is hardware out there, that is perfectly capable of
running upstream linux, which doesn't use one of the methods you
mentioned for controlling the power. Same did apply for the USB VBus
power, despite there being "USB_PORT_FEAT_POWER" in the spec and it
also got in the kernel, see commit
f7ac7787ad361e31a7972e2854ed8dc2eedfac3b.

I will try to add a short version of the reason written above in the
commit message.


Thanks,
- Markus Probst