Message-ID: <48eff111d66156fd0bd8eb2418570db4aa62f392.camel@posteo.de>
Date: Mon, 13 Oct 2025 12:29:46 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Niklas Cassel <cassel@...nel.org>
Cc: Damien Le Moal <dlemoal@...nel.org>, "James E.J. Bottomley"
	 <James.Bottomley@...senpartnership.com>, "Martin K. Petersen"
	 <martin.petersen@...cle.com>, linux-ide@...r.kernel.org, 
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] ata: Use ACPI methods to power on disks

On Mon, 2025-10-13 at 10:20 +0200, Niklas Cassel wrote:
> On Fri, Oct 10, 2025 at 10:38:35PM +0000, Markus Probst wrote:
> > Some embedded devices have the ability to control whether power is
> > provided to the disks via the sata power connector or not. If power
> > resources are defined on ata ports / devices in ACPI, we should try
> > to set
> > the power state to D0 before probing the disk to ensure that any
> > power
> > supply or power gate that may exist is providing power to the disk.
> > 
> > An example for such devices would be newer synology nas devices.
> > Every
> > disk slot has its own sata power connector. Whether the connector
> > is
> > providing power is controlled via an gpio, which is *off by
> > default*.
> > Also the disk loses power on reboots.
> > 
> > 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 the sata power connector if usable ACPI
> > power
> > resources on the associated ata port are found. It will be called
> > right
> > before probing the port, therefore the disk will be powered on just
> > in
> > time.
> 
> s/sata/SATA/
> s/nas/NAS/
> s/ata/ATA/ (except for function names of course)
> 
> Since this patch is basically doing two logical changes
> 1) Calling ata_acpi_dev_manage_restart() on restart, which calls
> acpi_bus_power_manageable() to disable power on shutdown.
> 
> 2) Calling ata_acpi_port_set_power_state() during ata_port_probe(),
> to enable power.
> 
> Please also split this patch into two, so that we have one commit per
> logical change. That would make things easier to understand, as your
> commit message your just describe one behavior instead of two
> completely
> different behaviors.
> 
> 
> Your commit message mentions that you want to spin down the disk on
> restart to avoid "avoid a power failure".
> 
> Is there a reason why you call acpi_bus_power_manageable() to spin
> down
> the disk instead of the regular function: ata_dev_power_set_standby()
> which spins down the disk?
I don't use acpi_bus_power_manageable() to spin down the disk. It just
checks if there is a power resource present in acpi for the ata port /
device. If thats the case, scsi should spin the disk down on
SYSTEM_RESTART.
> 
> 
> > 
> > Signed-off-by: Markus Probst <markus.probst@...teo.de>
> > ---
> >  drivers/ata/libata-acpi.c | 71
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/ata/libata-core.c |  2 ++
> >  drivers/ata/libata-scsi.c |  1 +
> >  drivers/ata/libata.h      |  4 +++
> >  4 files changed, 78 insertions(+)
> > 
> > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> > index f2140fc06ba0..4a72a98b922c 100644
> > --- a/drivers/ata/libata-acpi.c
> > +++ b/drivers/ata/libata-acpi.c
> > @@ -245,6 +245,77 @@ void ata_acpi_bind_dev(struct ata_device *dev)
> >  				   ata_acpi_dev_uevent);
> >  }
> >  
> > +/**
> > + * ata_acpi_dev_manage_restart - if the disk should be stopped
> > (spun down) on
> > + * system restart.
> > + * @dev: target ATA device
> > + *
> > + * RETURNS:
> > + * 1 if the disk should be stopped, otherwise 0
> > + */
> > +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.
> 
> Please no C++ style comments.
> 
> Also "If the device is power manageable and we assume"
> should this not be
> "If the device is power manageable, we assume"
> 
> Because your commit message says:
> "If a usable ACPI power resource has been found, it is assumed that
> the disk
> will lose power after a restart"
> 
> so I think the word "and" here is wrong.
> 
> 
> > +	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA) {
> > +		if (!is_acpi_device_node(dev->tdev.fwnode))
> > +			return 0;
> > +		return acpi_bus_power_manageable(ACPI_HANDLE(&dev-
> > >tdev));
> > +	}
> > +
> 
> Please add a commend here explaining the difference between the
> two cases, because you call either:
> return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
> or
> return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
> 
> At least the difference is not obvious to me, from just looking at
> this
> function.
if ATA_FLAG_ACPI_SATA is set, the acpi fwnode with the power resources
is not set on the ata_port->tdev, but on the ata_device->tdev. See
ata_acpi_bind_port (return if ATA_FLAG_ACPI_SATA is set) and
ata_acpi_bind_dev (return if ATA_FLAG_ACPI_SATA is not set).
I will add a comment for this.

Thanks
- Markus Probst

> 
> 
> > +	if (!is_acpi_device_node(dev->link->ap->tdev.fwnode))
> > +		return 0;
> > +	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link-
> > >ap->tdev));
> > +}
> > +
> > +/**
> > + * ata_acpi_port_set_power_state - set the power state of the ata
> > port
> > + * @ap: target ATA port
> > + * @enable: power state to be set
> > + *
> > + * This function is called at the beginning of ata_port_probe.
> > + */
> > +void ata_acpi_port_set_power_state(struct ata_port *ap, bool
> > enable)
> 
> This function is never called with enable==false, so let's please
> remove
> this parameter and rename the function to something like
> ata_acpi_port_enable_power() or similar.
> If someone a future patch ever wants to refactor this to also handle
> disable, then that patch can also create a parameter for this
> function.
> Otherwise we are just adding dead code.
> 
> 
> Kind regards,
> Niklas

Powered by blists - more mailing lists